Solid `IStakingModule` interface #574

Psirex · 2023-02-08T13:51:25Z

Main changes in the IStakingModule interface:

~~Solidity's structs are used for return values. In the contracts with the 0.4.24 Solidity version returned struct is encoded using assembly.~~ (Back to tuples)
the __Keys__ suffix in the method and variable names was removed.

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

skozin · 2023-02-08T16:27:19Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

-        activeValidatorsKeysCount = depositedSigningKeysCount - exitedValidatorsCount;
-        readyToDepositValidatorsKeysCount = vettedSigningKeysCount - depositedSigningKeysCount;
+        bytes memory validatorsReportEncoded = abi.encode(
+            uint256(totalSigningKeysStats.exitedSigningKeysCount),


the cast is prob not needed here as ABI encoder pads anything to 32 bytes anyway

Changed the encoding a bit. Now it just returns the struct from the memory instead of slicing it from the abi.encode() result.

skozin · 2023-02-08T16:28:49Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

+    //     uint256 totalStuck;
+    //     uint256 totalRefunded;
+    //     uint256 targetLimit;
+    //     uint256 excessCount;


what is excessCount? can it be calc from the other values?

It's a new field introduced by #557 PR. According to the description: "excess validators count for operator that will be forced to exit"

I thought it's done via targetLimit, prob I'm misunderstanding smth

skozin · 2023-02-08T16:33:53Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

+    // struct ValidatorsReport {
+    //     uint256 totalExited;
+    //     uint256 totalDeposited;
+    //     uint256 totalVetted;


I see an inconsistency in naming totalVetted vs targetLimit. maybe currentLimit and targetLimit?

Such naming was introduced in the #557 PR and was preserved for consistency.

skozin · 2023-02-08T16:35:34Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

+            0 // targetLimit
+        );
+        assembly {
+            return(add(validatorsReportEncoded, 32), mul(32, 7))


a comment explaining the magic is needed here

Added comment with an explanation of what data is returned

skozin · 2023-02-08T16:36:18Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

+            0 // targetLimit
+        );
+        assembly {
+            return(add(validatorsReportEncoded, 32), mul(32, 7))


a comment explaining the magic is needed here

Added comment with an explanation of what data is returned

skozin · 2023-02-08T16:37:10Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

    ///     4. a node operator was activated/deactivated
-    function getValidatorsKeysNonce() external view returns (uint256) {
+    ///     5. a node operator's validator(s) is used for the deposit
+    function getDepositsDataNonce() external view returns (uint256) {


the commonly used term is "deposit data", in the singular form

I'd rename to getDepositNonce()

Renamed to getDepositNonce()

skozin · 2023-02-08T16:40:35Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

-    ///     1. a node operator's key(s) is added
-    ///     2. a node operator's key(s) is removed
-    ///     3. a node operator's ready to deposit keys count is changed
+    ///     1. a node operator's validator(s) is added


validator => deposit data

…eratorsRegistry

…-staking-module-interface

ujenjt · 2023-02-09T12:05:54Z

contracts/0.8.9/interfaces/IStakingModule.sol

-    ///     5. a node operator's key(s) is used for the deposit
-    function getValidatorsKeysNonce() external view returns (uint256);
+    ///     5. a node operator's deposit data is used for the deposit
+    function getDepositNonce() external view returns (uint256);


getNonce()

Nonce MUST change in any changes in validator deposit data set

ujenjt · 2023-02-09T12:17:22Z

contracts/0.8.9/interfaces/IStakingModule.sol

+    uint256 totalExited;
+    uint256 totalDeposited;
+    uint256 totalVetted;
+    uint256 totalStuck;


ujenjt · 2023-02-09T12:17:30Z

contracts/0.8.9/interfaces/IStakingModule.sol

+    uint256 totalDeposited;
+    uint256 totalVetted;
+    uint256 totalStuck;
+    uint256 totalRefunded;


ujenjt · 2023-02-09T12:20:01Z

contracts/0.8.9/interfaces/IStakingModule.sol

+    uint256 totalStuck;
+    uint256 totalRefunded;
+    uint256 targetLimit;
+    uint256 excessCount;


I thought it's over-limit but from what I've been told it cannot be calculated as deposited - targetLimit (which the overLimit name would imply). and if I'm wrong and it can I'd remove the field altogether

ujenjt · 2023-02-09T12:21:29Z

contracts/0.8.9/interfaces/IStakingModule.sol

    /// @return number of exited validators across all node operators
-    function updateExitedValidatorsKeysCount(
+    function updateExitedValidatorsCount(


also add updateRefundedValidators

ujenjt · 2023-02-09T12:28:26Z

contracts/0.8.9/interfaces/IStakingModule.sol

-    /// @notice Called by StakingRouter after oracle finishes updating exited keys counts for all operators.
-    function finishUpdatingExitedValidatorsKeysCount() external;
+    /// @notice Called by StakingRouter after oracle finishes updating exited validators counts for all operators.
+    function finishUpdatingExitedValidatorsCount() external;


finishUpdatingNodeOperatorsData()
onNodeOperatorsDataFinalized()

ujenjt · 2023-02-09T12:31:46Z

contracts/0.8.9/interfaces/IStakingModule.sol

-    /// @notice Invalidates all unused validators keys for all node operators
-    function invalidateReadyToDepositKeys() external;
+    /// @notice Invalidates all unused validators for all node operators
+    function invalidateDepositData() external;


rename somehow

ujenjt · 2023-02-09T12:35:57Z

contracts/0.8.9/interfaces/IStakingModule.sol

+        bytes memory signatures
+    );
+
+    event DepositNonceChanged(uint256 depositNonce);


event StakingModuleNonceChanged(uint256 depositNonce);

ujenjt · 2023-02-09T12:45:04Z

contracts/0.8.9/interfaces/IStakingModule.sol

+struct ValidatorsReport {
+    uint256 totalExited;
+    uint256 totalDeposited;
+    uint256 totalVetted;


depositable

that's not correct, though. a key that's already deposited is counted towards totalVetted but is not depositable

…ed()

…-staking-module-interface

…iving

TheDZhon · 2023-02-10T13:42:53Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

+    /// @param _refundedValidatorsCount New number of refunded validators of the node operator
+    function updateRefundedValidatorsCount(uint256 _nodeOperatorId, uint256 _refundedValidatorsCount) external {
+        _onlyExistedNodeOperator(_nodeOperatorId);
+        _auth(STAKING_ROUTER_ROLE);


need a lever from StakingRouter

TheDZhon · 2023-02-10T13:46:22Z

contracts/0.4.24/nos/NodeOperatorsRegistry.sol

@@ -507,7 +505,8 @@ contract NodeOperatorsRegistry is AragonApp, Versioned {
        emit RefundedValidatorsCountChanged(_nodeOperatorId, _refundedValidatorsCount);
    }

-    function invalidateReadyToDepositKeys() external {
+    /// @notice Invalidates all unused validators for all node operators
+    function onWithdrawalCredentialsChanged() external {
        uint256 operatorsCount = getNodeOperatorsCount();
        if (operatorsCount > 0) {
            invalidateReadyToDepositKeysRange(0, operatorsCount - 1);


DepositData?

TheDZhon · 2023-02-10T13:57:24Z

contracts/0.8.9/StakingRouter.sol

-
-        // FIXME: get current value from the staking module
-        uint256 nodeOpStuckKeysCount;
+        (,,uint256 stuckValidatorsCount,,,uint256 totalExitedValidatorsCount,,) =


maybe add /* varName */ placeholders

TheDZhon

hooray 👍

Psirex added 4 commits February 8, 2023 16:10

Use structs in StakingModule. Rework getValidatorsKeysStats method

9adb386

Refactor IStakingModule interface

579be0b

Update tests

5931b3f

Rename invalidateReadyToDepositValidators

df2e0e1

skozin reviewed Feb 8, 2023

View reviewed changes

contracts/0.4.24/nos/NodeOperatorsRegistry.sol Outdated Show resolved Hide resolved

skozin reviewed Feb 8, 2023

View reviewed changes

Psirex added 11 commits February 9, 2023 02:10

Rename "keys" -> "validators" across the codebase

6258912

Update NodeOperatorsRegistry ValidatorsReport views

47578d7

Add getNodeOperatorIds to IStakingModule interface

4dc1218

Add StakingModulesDataAggregator contract

b2b3727

Update ABIs

7a8ae1b

rename provideDepositsData to obtainDepositData

fd30a72

Add more detailed comments to getValidatorsReport views in the NodeOp…

f4e6615

…eratorsRegistry

Rename depositsDataNonce to depositNonce

c9a3e62

Rename invalidateDepositsData to invalidateDepositData

c952094

Merge branch 'feature/shapella-upgrade' into feature/shapella-upgrade…

92864f0

…-staking-module-interface

Use new ValidatorsReport fields in StakingRouter

d98395b

Psirex marked this pull request as ready for review February 9, 2023 03:23

Remove StakingModulesDataAggregator contract

17227a7

ujenjt reviewed Feb 9, 2023

View reviewed changes

Rename getDepositNonce() to getNonce()

2aa991d

Psirex added 15 commits February 10, 2023 05:36

Update ValidatorsReport fields

6a1ed2a

Add updateRefundedValidatorsCount() method

44111a3

finishUpdatingExitedValidatorsCount() -> onAllValidatorsCountersUpdat…

9ddeaf5

…ed()

Rename invalidateDepositData() to onWithdrawalCredentialsChanged()

0d59b8f

Add missing doc comments to the IStakinModule

4aa1299

Merge branch 'feature/shapella-upgrade' into feature/shapella-upgrade…

01fad86

…-staking-module-interface

Remove FIXME from NodeOperatorsRegistry

d795ffc

After-merge node-operators-registry test fixes

eb38e3b

Replace getValidatorsReport() methods

ea278f3

Rename onAllValidatorsCountersUpdated to onAllValidatorCountersUpdated

47fdf2d

Merge branch 'feature/shapella-upgrade' into feature/shapella-upgrade…

0e42d35

…-staking-module-interface

Fix typo in NodeOperatorRegistry

aab9e44

Rename keys to validators in var names in the StakingRouter

500019d

Update getNonce() comment in NodeOperatorsRegistry

6232e97

Add methods for batched node operator and staking module reports rece…

9279f11

…iving

TheDZhon reviewed Feb 10, 2023

View reviewed changes

Psirex added 3 commits February 10, 2023 18:09

Add updateRefundedValidatorsCount to StakingRouter

888eed7

Add commented var names into tuple decoding

d5cf565

Use uint256 type in external/public methods

1f7ad2c

TheDZhon approved these changes Feb 10, 2023

View reviewed changes

TheDZhon merged commit f34273c into feature/shapella-upgrade Feb 10, 2023

TheDZhon deleted the feature/shapella-upgrade-staking-module-interface branch February 10, 2023 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solid `IStakingModule` interface #574

Solid `IStakingModule` interface #574

Psirex commented Feb 8, 2023 •

edited

Loading

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 9, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 8, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

skozin Feb 8, 2023

Psirex Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

skozin Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

ujenjt Feb 9, 2023

skozin Feb 9, 2023 •

edited

Loading

TheDZhon Feb 10, 2023

TheDZhon Feb 10, 2023

TheDZhon Feb 10, 2023

TheDZhon left a comment

Solid IStakingModule interface #574

Solid IStakingModule interface #574

Conversation

Psirex commented Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skozin Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheDZhon left a comment

Choose a reason for hiding this comment

Solid `IStakingModule` interface #574

Solid `IStakingModule` interface #574

Psirex commented Feb 8, 2023 •

edited

Loading

skozin Feb 9, 2023 •

edited

Loading