SSV Native staking #2015

sparrowDom · 2024-04-04T20:45:06Z

Contracts

Changes

Added new NativeStakingSSVStrategy that takes WETH from the Vault and stakes ETH into SSV validators.
Added FeeAccumulator to receive ETH execution rewards.
Changes BaseHarvester so it can collect WETH and send it straight to the Dripper

Diagrams

NativeStakingSSVStrategy

FeeAccumulator

Processes

Value Flows

The flow of native currency (ETH) and tokens between the wallets and contracts.

Contract calls

Testing

Unit Tests

Unit tests are in contracts/test/strategies/nativeSSVStaking.js

yarn run test

Holesky fork tests

Holesky fork tests are in contracts/test/strategies/nativeSsvStaking.holesky.fork-test.js

yarn run node:hol
yarn run test:hol-fork

Mainnet fork tests

Mainnet fork tests are in contracts/test/strategies/nativeSsvStaking.fork-test.js

yarn run node
yarn run test:fork

Deployment

Operations

Hardhat tasks

github-actions · 2024-04-04T20:45:42Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 7eb135b

codecov · 2024-04-04T21:00:12Z

Codecov Report

Attention: Patch coverage is 94.94382% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 62.24%. Comparing base (fdb0ba0) to head (e1ff728).
Report is 22 commits behind head on master.

❗ Current head e1ff728 differs from pull request most recent head 579a93c. Consider uploading reports for the commit 579a93c to get more accurate results

Files	Patch %	Lines
contracts/contracts/oracle/OETHFixedOracle.sol	0.00%	4 Missing ⚠️
.../strategies/NativeStaking/ValidatorRegistrator.sol	94.44%	3 Missing ⚠️
...ategies/NativeStaking/NativeStakingSSVStrategy.sol	98.07%	1 Missing ⚠️
...s/strategies/NativeStaking/ValidatorAccountant.sol	98.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2015      +/-   ##
==========================================
+ Coverage   60.92%   62.24%   +1.31%     
==========================================
  Files          58       64       +6     
  Lines        2974     3181     +207     
  Branches      774      829      +55     
==========================================
+ Hits         1812     1980     +168     
- Misses       1159     1198      +39     
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

naddison36 · 2024-04-22T06:58:06Z

contracts/contracts/strategies/NativeStaking/ValidatorRegistrator.sol

+        EXIT_COMPLETE // validator has funds withdrawn to the EigenPod and is removed from the SSV
+    }
+
+    event RegistratorAddressChanged(address oldAddress, address newAddress);


I know this has been a pattern used for a while in Origin dollar, but do you need both the old and new addresses in change events? Can we just have the new address?

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

naddison36 · 2024-04-22T11:18:27Z

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

+
+        balance = activeDepositedValidators * 32 ether;
+        balance += beaconChainRewardWETH;
+        balance += FEE_ACCUMULATOR_ADDRESS.balance;


I don't think we want to include the MEV rewards in the FeeAccumulator if those are going to be harvested and sent to through the dripper.

Thanks this is a nice catch. I see I haven't given it enough though as to how (W)ETH should be handled.
Lets continue discussion here so we have it all in 1 place: https://www.notion.so/originprotocol/How-should-accounting-function-handle-ETH-after-accounting-4ed3a9352c0f4fc8aa6bb866022a25cb

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol

naddison36 · 2024-04-22T11:46:18Z

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol

+    /// @dev Governor that can manually correct the accounting
+    address public accountingGovernor;
+    /// @dev Strategist that can pause the accounting
+    address public strategist;


Best to read the strategist from the Vault rather than duplicating the maintenance in the strategy. This is what OETH AMO strategy does

modifier onlyStrategist() { require( msg.sender == IVault(vaultAddress).strategistAddr(), "Caller is not the Strategist" ); _; }

* Update Natspec * Generated docs for native eth strategy * Prettier and linter Fixed spelling of ValidatorAccountant events Implemented depositSSV * Updated Natspec Moved MAX_STAKE on ValidatorAccountant to a constant * Removed strategist from strategy as its already maintained in the Vault * Fix compilation error * Fix unit tests * fix linter

* Added OETH process diagram with functions calls for native staking * Native Staking Strategy now hold consensus rewards at ETH FeeAccumulator now holds execution rewards as ETH Removed WETH immutable from FeeAccumulator Converted custom errors back to require with string collect rewards now converts ETH to WETH at harvest checkBalance is now validators * 32 plus WETH balance from deposits Renamed beaconChainRewardsWETH to consensusRewards Fixed bug in stakeETH that was converting all WETH to ETH

* Fixed native staking deployment since the strategist is got from the vault * Refactor of some Native Staking events Refactor of Native Staking unit tests * Renamed AccountingBeaconChainRewards to AccountingConsensusRewards Accounting updated to handle zero ETH from the beacon chain * fixed bug not accounting for previous consensus rewards Blow fuse if ETH balance < previous consensus rewards * Pause collectRewardTokens and doAccounting on accounting failure. Validated asset on deposit to Native Staking Strategy. Moved depositSSV from NativeStakingSSVStrategy to ValidatorRegistrator moved onlyStrategist modified and VAULT_ADDRESS immutable from ValidatorAccountant to ValidatorRegistrator manuallyFixAccounting changed to use whenPaused modifier made fuseIntervalEnd inclusive Natspec updates refactoring of native staking unit tests

* add basic steps to deploy OETH to holesky * prettier * minor change * holesky deployment ifles holesky deployment files * add holesky deployment files * minor fix * minor fixes * make the fork tests run on Holesky * add some more tests * testing SSV staking on Holesky * refactor where deployment files are located * more progress on deployment * add deposit to validator deployment files * remove log * prettier * lint * move file * SSV cluster info (#2036) * add ability to fetch SSV cluster information * prettier

* manuallyFixAccounting now uses delta values and only callable by the strategist manuallyFixAccounting calls doAccounting to check the fuse is still not blown Removed accountingGovernor * Added pauseOnFail param to internal _doAccounting Increased the allowed delta values of manuallyFixAccounting * ran prettier

pandadefi · 2024-05-07T15:38:13Z

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol

+    uint256 public constant MAX_STAKE = 32 ether;
+
+    /// @notice Keeps track of the total consensus rewards swept from the beacon chain
+    uint256 public consensusRewards = 0;


In Solidity, variables are automatically initialized to zero. Therefore, explicitly initializing variables to zero is not mandatory and can be skipped.

Good point, fixed here: 8bf9a26

pandadefi · 2024-05-07T15:44:06Z

contracts/contracts/strategies/NativeStaking/ValidatorRegistrator.sol

+                validators[i].depositDataRoot
+            );
+
+            activeDepositedValidators += 1;


You could save on some gas by adding the validators.length once instead of doing a +1 on every interaction..

great gas savings thanks: fd4f2d2

pandadefi · 2024-05-07T15:44:52Z

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol

+
+        // send the ETH that is from fully withdrawn validators to the Vault
+        if (newSweptETH >= MAX_STAKE) {
+            uint256 fullyWithdrawnValidators = newSweptETH / MAX_STAKE;


This safe division could use unchecked

agreed, thanks

pandadefi · 2024-05-07T15:45:42Z

contracts/contracts/strategies/NativeStaking/FeeAccumulator.sol

+        require(msg.sender == STRATEGY, "Caller is not the Strategy");
+
+        eth = address(this).balance;
+        if (eth > 0) {


You have a couple of superior to zero checks, it's more gas-efficient to use a not zero check.

I've tested this with the hardhat-gas-reporter and Remix and didn't see a noticeable difference in gas usage.

pandadefi · 2024-05-07T15:47:45Z

contracts/contracts/strategies/NativeStaking/ValidatorRegistrator.sol

+        IWETH9(WETH_TOKEN_ADDRESS).withdraw(requiredETH);
+
+        // For each validator
+        for (uint256 i = 0; i < validators.length; ) {


validators.length should be cached outside of the loop.

thanks, fixed here: fd4f2d2

* adding defender action task * fix unit tests setup * update the registrator address of the native staking contract * fix up the operate validators script so that it works for native staking on holesky * also add the gitignore * add ability to exit if staking contract is paused

* Fixed resolveAsset * Fixed validator fork tests Added error func sigs to ISSVNetwork * Fixed harvester behaviour tests * Fix global hooks * Fix tooling --------- Co-authored-by: Shahul Hameed <10547529+shahthepro@users.noreply.github.com>

* add payable to fee accumulator * remove zero initializers * some gas savings

…aking

initial commit of the native staking strategy

063d8b2

sparrowDom self-assigned this Apr 4, 2024

fix text

6e8332b

sparrowDom added 12 commits April 10, 2024 15:17

add some more functionality

b933f22

finish up the fuse interval test, update deploy script

0a43d14

some natspec and refactoring

d31fe30

add some more accounting logic

03c544c

finish up accounting and add tests for it

9aa0d73

add some comments

e99f86c

add tests for manually fixing accounting

52ba11e

lint and prettier

e656ecb

undo file delete

fe113a5

implement the collect rewards function and add tests for it

71ad71f

implement and test checkBalance

2c448bb

add functions to register and exit/remove the ssv validator

5d5e9cb

naddison36 reviewed Apr 22, 2024

View reviewed changes

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol Outdated Show resolved Hide resolved

naddison36 reviewed Apr 22, 2024

View reviewed changes

naddison36 and others added 10 commits April 22, 2024 17:35

allow for WETH to send ETH to the contract

87c5254

shorten error message

1d66508

fix depositing bug

00ec878

fix fork tests

da6a7b5

Merge branch 'master' into sparrowDom/nativeStaking

fd17ec9

pandadefi reviewed May 7, 2024

View reviewed changes

fix global hooks filter

fe12d23

sparrowDom force-pushed the sparrowDom/nativeStaking branch from 97e38ec to fe12d23 Compare May 7, 2024 15:49

sparrowDom added 6 commits May 7, 2024 17:57

add github workflow

c61c9bb

another workflow fix

7e2e640

another workflow fix

af8ea03

fix holesky tests

32480dd

remove the .only

0fb09ea

prettier

c7175bb

DanielVF added the contracts Works related to contracts label May 8, 2024

sparrowDom marked this pull request as ready for review May 9, 2024 12:37

sparrowDom requested review from DanielVF and shahthepro as code owners May 9, 2024 12:37

naddison36 and others added 12 commits May 10, 2024 08:16

Fix fork tests (#2050)

2f7a9cd

* Fixed resolveAsset * Fixed validator fork tests Added error func sigs to ISSVNetwork * Fixed harvester behaviour tests * Fix global hooks * Fix tooling --------- Co-authored-by: Shahul Hameed <10547529+shahthepro@users.noreply.github.com>

add payable to fee accumulator (#2053)

0623a47

* add payable to fee accumulator * remove zero initializers * some gas savings

fix linter

f268742

Fix balancer fork test

20d29a1

Merge remote-tracking branch 'origin/master' into sparrowDom/nativeSt…

2a93df2

…aking

Fixed resolveContract for none proxied contracts

b898801

simplified woethCcipZapperFixture

25170aa

Regenerated latest contract diagrams

e1ff728

Resolved Slither divide-before-multiply warning

c09cd46

Native staking deployment to reduceQueueTime

9d83e97

Fixed fork test harvesting CRV rewards

7eb135b

gas optimisation

579a93c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSV Native staking #2015

SSV Native staking #2015

sparrowDom commented Apr 4, 2024 •

edited by naddison36

github-actions bot commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited

naddison36 Apr 22, 2024

naddison36 Apr 22, 2024

sparrowDom Apr 22, 2024

naddison36 Apr 22, 2024

pandadefi May 7, 2024

sparrowDom May 9, 2024

pandadefi May 7, 2024

sparrowDom May 9, 2024

pandadefi May 7, 2024

sparrowDom May 9, 2024

pandadefi May 7, 2024

sparrowDom May 10, 2024

pandadefi May 7, 2024

sparrowDom May 9, 2024

SSV Native staking #2015

Are you sure you want to change the base?

SSV Native staking #2015

Conversation

sparrowDom commented Apr 4, 2024 • edited by naddison36

Contracts

Changes

Diagrams

NativeStakingSSVStrategy

FeeAccumulator

Processes

Value Flows

Contract calls

Testing

Unit Tests

Holesky fork tests

Mainnet fork tests

Deployment

Operations

github-actions bot commented Apr 4, 2024 • edited

codecov bot commented Apr 4, 2024 • edited

Codecov Report

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

sparrowDom commented Apr 4, 2024 •

edited by naddison36

github-actions bot commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited