Fix event values #2060

sparrowDom · 2024-05-13T21:12:36Z

adds tests to detect the new Withdrawal events
allows for the manuallyFixAccounting function to wrap ETH into WETH and send it to the vault to correct a possible state where the accounting function needs to classify a blown fuse as a full (slashed) withdrawal of the validator and send its assets as WETH to the Vault
add better comments
improve accounting for depositAll to communicate correct values

github-actions · 2024-05-13T21:13:09Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against e71897d

codecov · 2024-05-13T21:24:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.29%. Comparing base (e8c15b5) to head (e71897d).

Additional details and impacted files

@@                     Coverage Diff                      @@
##           sparrowDom/nativeStaking    #2060      +/-   ##
============================================================
+ Coverage                     62.15%   62.29%   +0.14%     
============================================================
  Files                            65       65              
  Lines                          3216     3228      +12     
  Branches                        626      833     +207     
============================================================
+ Hits                           1999     2011      +12     
  Misses                         1214     1214              
  Partials                          3        3

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

…ually fixing accounting

naddison36

I've made a few small changes to Native Staking Strategy's depositAll function

naddison36 · 2024-05-22T02:45:13Z

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

@@ -154,8 +188,16 @@ contract NativeStakingSSVStrategy is
        uint256 wethBalance = IERC20(WETH_TOKEN_ADDRESS).balanceOf(
            address(this)
        );
+        uint256 newWeth = wethBalance - depositedWethAccountedFor;
+        depositedWethAccountedFor += newWeth;


isn't the new depositedWethAccountedFor just the wethBalance? It'll be cheaper gas wise to assign storage variable depositedWethAccountedFor the wethBalance. The alternative is it reads the value from storage again (100 gas), adds it and then stores it back.

naddison36 · 2024-05-22T02:47:58Z

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

+        depositedWethAccountedFor += newWeth;
+
+        require(
+            depositedWethAccountedFor == wethBalance,


I can't see how this could be anything but true given the previous two lines

naddison36 · 2024-05-22T03:09:02Z

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

+            depositedWethAccountedFor == wethBalance,
+            "Unexpected depositedWethAccountedFor amount"
+        );
+
        if (wethBalance > 0) {


Should this be if (wethBalance > 0) {?

The wethBalance could be > 0 while newWeth is equal to zero. This will then fail in _deposit with Must deposit something. This could be ok, but when wethBalance is zero we don't fail so it seems inconsistent.

Looking at the other strategies, they don't fail if depositAll is called without any assets being transferred.

…parrowDom/eventFixes

naddison36 · 2024-05-22T06:21:08Z

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

+    /// It is important to note that this variable is not concerned with WETH that is a result of full/partial
+    /// withdrawal of the validators. It is strictly concerned with WETH that has been deposited and is waiting to
+    /// be staked.
+    uint256 depositedWethAccountedFor;


nit storage variables should explicitly be public

* add fuse interval logic and deploy script * add accounting logic * add manually fixing accounting * implement the collect rewards function and add tests for it * implement and test checkBalance * add functions to register and exit/remove the ssv validator * Implemented depositSSV * Moved MAX_STAKE on ValidatorAccountant to a constant * Removed strategist from strategy as its already maintained in the Vault * Native staking changes (#2024) * 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 * Native staking changes and unit tests (#2029) * 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 * allow for WETH to send ETH to the contract * Holesky deploy (#2026) * add basic steps to deploy OETH to holesky * add holesky deployment files * make the fork tests run on Holesky * testing SSV staking on Holesky * add deposit to validator deployment files * SSV cluster info (#2036) * add ability to fetch SSV cluster information * manuallyFixAccounting changes (#2034) * 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 * mainnet native staking fork tests (#2037) * 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 * Added Defender Relayer for validator registrator Added ssv utils to get cluster data Added native staking fork tests * Removed now redundant IWETH9 import * moved more logic into native staking fixture * Removed unused imports * fix native staking unit tests * Fail accounting if activeDepositedValidators < fullyWithdrawnValidators Changed Harvester to transfer WETH to dripper Added more mainnet fork tests for native staking * Updated the OETH value flows * Added governable Hardhat tasks Created a resolveContract util * deconstruct params for Hardhat tasks * WIP Hardhat tasks for validator registration * Added depositSSV HH task * Updated OETH contract dependency diagram * Update to diagrams * mini fixes * fix bug and minor test improvement * update yarn fulie * unify the holesky and the mainnet fork tests * re-deploy holesky native staking strategy (#2046) * also re-deploy the harvester * upgrade harvester as well * fix upgrade script and correct the bug in deploy actions * Deployed new Native Staking strategy including the proxy * Added Hardhat tasks for generic strategy functions * remove nativeStakingSSVStrategyProxy from js addresses file --------- Co-authored-by: Domen Grabec <grabec@gmail.com> * fix unit tests * fix global hooks filter * add github workflow * another workflow fix * another workflow fix * fix holesky tests * remove the .only * prettier * Sparrow dom/native staking defender action (#2051) * 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 * Fix fork tests (#2050) * 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) * add payable to fee accumulator * remove zero initializers * some gas savings * fix linter * Fix balancer fork test * Fixed resolveContract for none proxied contracts * simplified woethCcipZapperFixture * Regenerated latest contract diagrams * Resolved Slither divide-before-multiply warning * Native staking deployment to reduceQueueTime * Fixed fork test harvesting CRV rewards * gas optimisation * make verification automatic * removed wethToVault from manuallyFixAccounting * native staking strategy redeployed * adjust the comment * add rate limit to calling manually fix accounting (#2057) * add rate limit to calling manually fix accounting * fix comment * Correct deployName in Native Staking deploy script * Fix linter * Remove approveAssets on Swapper contract in 095_ogn_buyback script as it has already been done * Fix Holesky fork tests * Fix mainnet fork tests * skip deploy 095_ogn_buyback for now * fix some Slither errors * Fix Slither warnings * Upgrade the CI to use node.js 20.x * Fix CI * Fix CI * Upgrade to node.js 20 * Still upgrade the buyback contracts but not approveAssets on the swapper * Upgrade actions so they don't use node.js 16 * attempt to fix holesky fork run * Generated latest contract diagram * fix codecov upload * Fix OETH process diagram for native staking * add a withdrawal event when withdrawing WETH to vault (#2059) * Regenerated latest native staking diagram * add a util contract that is able to recalculate a valid deposit data root with an invalid signature * Prettier native staking test * Renamed native staking deploy script * Fix event values (#2060) * add withdrawal events to the tests * add WETH accounting * add better documentation * add sending WETH to the Vault back to fix manual accounting function * actually wrap the ETH to WETH before sending it to the Vault when manually fixing accounting * add withdrawal event to manually fix accounting * fix bugs and add tests with WETH accounting * add test to verify correct deposits * Gas changes to Native Staking's depositAll --------- Co-authored-by: Nicholas Addison <nick@addisonbrown.com.au> * Deploy latest Native Staking Strategy to Holesky (#2073) * Deployed new NativeStakingSSVStrategy * Generated latest Native Staking Strategy diagrams * Deploy native staking Proxy via Relayer (#2066) * deploy native staking proxy and add auxiliary functions to transfer its governance * Shortened error message in InitializeGovernedUpgradeabilityProxy.initialize --------- Co-authored-by: Nicholas Addison <nick@addisonbrown.com.au> * Setup basic defender action (#2072) * deploy native staking proxy and add auxiliary functions to transfer its governance * Setup basic defender action * update defender-sdk version * add defender client as an external export * configuration update --------- Co-authored-by: Nicholas Addison <nick@addisonbrown.com.au> * Output more contract details in 097 deploy script * Fix Native Staking fork tests * Changed ssv util getClusterInfo to use the SSV API instead of the ssv-scanner Moved duplicate getClusterInfo in tasks to utils * Prettier * ValidatorRegistrator.stakeEth gas improvement when multiple validators * Corrected stakeEth call in OETH processes diagram * Added fork test for registering a validator twice * Stake funds with confirmations for front-running protection of Beacon Deposits (#2074) * contract changes and tests for gated protection against front running * Update off-chain validator registration process to also consider stake threshold * front run protection changes (#2076) * Added Holesky deploy script * Fixed OUSD metapool fork test * Generated latest NativeStakingSSVStrategy contract diagrams * Deployed new NativeStakingSSVStrategy to Holesky --------- Co-authored-by: Nicholas Addison <nick@addisonbrown.com.au> * Updated Buyback value flow for OETH * Updated Native staking process diagram * Updated OETH Vault diagrams * Add process for pausing the Native Staking Strategy * Removed the redundant condition in the fuse interval check (#2082) * OZ - Native Staking - M-01 All Addresses Are Registered Validators by Default (#2081) * Added default NON_REGISTERED to VALIDATOR_STATE * Added explicit check that a validator has not already been registered * Added new Holesky deploy script * OZ - Native Staking - N-07 Lack of Indexed Event Parameters (#2083) * Indexed the RegistratorChanged and StakingMonitorChanged events * Added indexed pubKeyHash to validator events * Deployed new Native Staking Strategy * generated latest NativeStakingSSVStrategy docs * P2P API changes (#2084) * Updated Natspec * Updated native staking process diagrams * Fixed signer for Holesky * split operateValidators into registerValidators and stakeValidators * Fix vault mint HH task. It now waits for the approve tx to be mined * resolveAsset to handle local Holesky fork * upgrade signer to use new defender-relay-client package * added approve option to vault mint HH task * Added stakeValidators HH task * update git ignore file * Added depositWETH and withdrawWETH HH tasks * Added resetStakeETHTally and setStakeETHThreshold HH tasks * generated latest Native Staking Strategy docs * Hardhat tasks for Native Staking (#2085) * Fixed depositSSV HH tasks * Added exitValidator and removeValidator HH tasks * Added doAccounting Defender Action * Added harvest HH task * Added fixAccounting and pauseStaking HH tasks * Fixed stakeValidators when run without uuid option * Cap the validators a Native Staking Strategy can hold (#2087) * Added max validators check to Native Staking Strategy * don't format Defender Action code in dist folder * Deployed latest NativeStakingSSVStrategy contract to Holesky * Capitalised error messages in Native Staking Strategy * Fix hot deploy of Native Staking Strategy * moved when event it emitted * remove unchecked when calculating fullyWithdrawnValidators * Renamed MAX_STAKE to FULL_STAKE * Renamed withdrawal_credentials to withdrawalCredentials * Renamed WETH_TOKEN_ADDRESS to WETH * renamed SSV_TOKEN_ADDRESS to SSV_TOKEN renamed SSV_NETWORK_ADDRESS to SSV_NETWORK * replaced 32 ether with FULL_STAKE * moved the internal function to the bottom of NativeStakingSSVStrategy * fixed formatting of if statements * Moved emits to after state changes * Removed validatorsLength variable as we aren't dealing with a storage array * removed unchecked iteration in for loop * consistent check of currentState * Added new Holesky deploy script * Removed Governable from FeeAccumulator --------- Co-authored-by: Nick Addison <nick@addisonbrown.com.au> Co-authored-by: Shahul Hameed <10547529+shahthepro@users.noreply.github.com>

add withdrawal events to the tests

bf2e6d4

sparrowDom requested review from DanielVF, naddison36 and shahthepro as code owners May 13, 2024 21:12

sparrowDom marked this pull request as draft May 13, 2024 22:00

sparrowDom added 12 commits May 14, 2024 00:00

add WETH accounting

1be5598

add better documentation

4158272

add sending WETH to the Vault back to fix manual accounting function

54b7f5e

actually wrap the ETH to WETH before sending it to the Vault when man…

3474eae

…ually fixing accounting

lint

3a54d03

add withdrawal event to manually fix accounting

38005b5

prettier

3e338b6

slither

04fcc9e

fix fork tests

f30ca4a

fix bugs and add tests with WETH accounting

8545a27

add comment

a6584d2

add test to verify correct deposits

5afef62

sparrowDom self-assigned this May 15, 2024

sparrowDom marked this pull request as ready for review May 15, 2024 11:38

sparrowDom and others added 2 commits May 15, 2024 13:46

lint

95fe981

Gas changes to Native Staking's depositAll

73fa09b

naddison36 approved these changes May 22, 2024

View reviewed changes

Merge remote-tracking branch 'origin/sparrowDom/nativeStaking' into s…

e71897d

…parrowDom/eventFixes

naddison36 merged commit 638bf30 into sparrowDom/nativeStaking May 22, 2024
14 of 16 checks passed

naddison36 deleted the sparrowDom/eventFixes branch May 22, 2024 05:01

naddison36 reviewed May 22, 2024

View reviewed changes

naddison36 mentioned this pull request May 22, 2024

Deploy latest Native Staking Strategy to Holesky #2073

Merged

naddison36 mentioned this pull request May 31, 2024

SSV Native staking #2015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix event values #2060

Fix event values #2060

sparrowDom commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited

naddison36 left a comment

naddison36 May 22, 2024

naddison36 May 22, 2024

naddison36 May 22, 2024

naddison36 May 22, 2024

Fix event values #2060

Fix event values #2060

Conversation

sparrowDom commented May 13, 2024 • edited

github-actions bot commented May 13, 2024 • edited

codecov bot commented May 13, 2024 • edited

Codecov Report

naddison36 left a comment

Choose a reason for hiding this comment

naddison36 May 22, 2024

Choose a reason for hiding this comment

naddison36 May 22, 2024

Choose a reason for hiding this comment

naddison36 May 22, 2024

Choose a reason for hiding this comment

naddison36 May 22, 2024

Choose a reason for hiding this comment

sparrowDom commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited