Merge pull request #484 from lidofinance/feature/withdrawal_tests

- restore basic Withdrawal queue tests - fix a bug sneaked from WQv0 on request creation - fix a bug with shares precision calculation on finalization - unify assertReverts for custom error solutions
lidofinance · Jan 13, 2023 · 5680024 · 5680024
2 parents 86d68d2 + da08679
commit 5680024
Show file tree

Hide file tree

Showing 29 changed files with 295 additions and 476 deletions.
diff --git a/contracts/0.4.24/interfaces/IWithdrawalQueue.sol b/contracts/0.4.24/interfaces/IWithdrawalQueue.sol
@@ -11,7 +11,7 @@ interface IWithdrawalQueue {
     function calculateFinalizationParams(
         uint256 _lastIdToFinalize,
         uint256 _shareRate
-    ) external view returns (uint256 sharesToBurn, uint256 etherToLock);
+    ) external view returns (uint128 sharesToBurn, uint128 etherToLock);
 
     function finalize(
         uint256 _lastIdToFinalize,

diff --git a/contracts/0.4.24/test_helpers/StETHMock.sol b/contracts/0.4.24/test_helpers/StETHMock.sol
@@ -13,7 +13,7 @@ import "../StETH.sol";
 contract StETHMock is StETH {
     uint256 private totalPooledEther;
 
-    constructor() public {
+    constructor() public payable{
         _resume();
     }
 

diff --git a/contracts/0.8.9/WithdrawalQueue.sol b/contracts/0.8.9/WithdrawalQueue.sol
@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: 2022 Lido <[email protected]>
+// SPDX-FileCopyrightText: 2023 Lido <[email protected]>
 // SPDX-License-Identifier: GPL-3.0
 
 /* See contracts/COMPILERS.md */
@@ -122,6 +122,8 @@ contract WithdrawalQueue {
      */
     uint256 public constant MAX_STETH_WITHDRAWAL_AMOUNT = 1000 ether;
 
+    uint256 public constant SHARE_RATE_PRECISION = 1e27;
+
     ///! STRUCTURED STORAGE OF THE CONTRACT
     ///! SLOT 0: uint128 lockedEtherAmount
     ///! SLOT 1: uint256 finalizedRequestsCounter
@@ -139,7 +141,7 @@ contract WithdrawalQueue {
     uint256 public finalizedRequestsCounter = 0;
 
     /// @notice queue for withdrawal requests
-    WithdrawalRequest[] public queue;
+    WithdrawalRequest[] internal queue;
 
     /// @notice withdrawal requests mapped to the recipients
     mapping(address => uint256[]) public requestsByRecipient;
@@ -311,7 +313,9 @@ contract WithdrawalQueue {
         if (_lastIdToFinalize < finalizedRequestsCounter || _lastIdToFinalize >= queue.length) {
             revert InvalidFinalizationId();
         }
-        if (lockedEtherAmount + msg.value > address(this).balance) revert NotEnoughEther();
+        (uint128 ethToWithdraw, ) = _calculateDiscountedBatch(finalizedRequestsCounter, _lastIdToFinalize, _shareRate);
+
+        if (msg.value < ethToWithdraw) revert NotEnoughEther();
 
         _updateRateHistory(_shareRate, _lastIdToFinalize);
 
@@ -331,7 +335,7 @@ contract WithdrawalQueue {
     function calculateFinalizationParams(
         uint256 _lastIdToFinalize,
         uint256 _shareRate
-    ) external view returns (uint256 etherToLock, uint256 sharesToBurn) {
+    ) external view returns (uint128 etherToLock, uint128 sharesToBurn) {
         return _calculateDiscountedBatch(finalizedRequestsCounter, _lastIdToFinalize, _shareRate);
     }
 
@@ -413,7 +417,7 @@ contract WithdrawalQueue {
             shares -= queue[_firstId - 1].cumulativeShares;
         }
 
-        eth = _min(eth, _toUint128((shares * _shareRate) / 1e9));
+        eth = _min(eth, _toUint128(shares * _shareRate / SHARE_RATE_PRECISION));
     }
 
     /// @dev checks if provided request included in the rate hint boundaries
@@ -498,7 +502,7 @@ contract WithdrawalQueue {
             WithdrawalRequest memory prevRequest = queue[requestId - 1];
 
             cumulativeShares += prevRequest.cumulativeShares;
-            cumulativeShares += prevRequest.cumulativeEther;
+            cumulativeEther += prevRequest.cumulativeEther;
         }
 
         queue.push(

diff --git a/contracts/0.8.9/test_helpers/Owner.sol b/contracts/0.8.9/test_helpers/Owner.sol
diff --git a/contracts/0.8.9/test_helpers/StETHMockForWithdrawalQueue.sol b/contracts/0.8.9/test_helpers/StETHMockForWithdrawalQueue.sol
diff --git a/lib/abi/WithdrawalQueue.json b/lib/abi/WithdrawalQueue.json
diff --git a/package.json b/package.json
@@ -11,7 +11,7 @@
     "gasprofile"
   ],
   "scripts": {
-    "postinstall": "patch-package && husky install",
+    "postinstall": "husky install",
     "build:apps": "yarn compile && hardhat run --no-compile scripts/build-apps-frontend.js",
     "lint": "yarn lint:sol && yarn lint:js",
     "lint:sol": "solhint \"contracts/**/*.sol\" --ignore-path .solhintignore",
@@ -127,7 +127,6 @@
     "ethers": "^5.1.4",
     "node-gyp": "^8.4.1",
     "openzeppelin-solidity": "2.0.0",
-    "patch-package": "^6.4.7",
     "solhint-plugin-lido": "^0.0.4",
     "yargs": "^16.0.3"
   }

diff --git a/patches/@aragon+contract-helpers-test+0.1.0.patch b/patches/@aragon+contract-helpers-test+0.1.0.patch
diff --git a/test/0.4.24/lido.rewards-distribution.test.js b/test/0.4.24/lido.rewards-distribution.test.js
@@ -1,6 +1,6 @@
 const { assert } = require('chai')
 const { newDao, newApp } = require('./helpers/dao')
-const { assertBn, assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn } = require('@aragon/contract-helpers-test/src/asserts')
 const { ZERO_ADDRESS, bn } = require('@aragon/contract-helpers-test')
 
 const NodeOperatorsRegistry = artifacts.require('NodeOperatorsRegistryMock')

diff --git a/test/0.4.24/lido.test.js b/test/0.4.24/lido.test.js
@@ -3,7 +3,9 @@ const { assert } = require('chai')
 const { artifacts } = require('hardhat')
 
 const { getInstalledApp } = require('@aragon/contract-helpers-test/src/aragon-os')
-const { assertBn, assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
+
 
 const { newDao, newApp } = require('./helpers/dao')
 const { ZERO_ADDRESS, bn, getEventAt } = require('@aragon/contract-helpers-test')
@@ -457,17 +459,17 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor, t
 
     await assertRevert(
       stakingRouter.updateStakingModule(module1.id, 10001, 300, 700, { from: voting }),
-      `ed with custom error 'ErrorValueOver100Percent("_targetShare")`
+      `ErrorValueOver100Percent("_targetShare")`
     )
 
     await assertRevert(
       stakingRouter.updateStakingModule(module1.id, 10000, 10001, 700, { from: voting }),
-      `ed with custom error 'ErrorValueOver100Percent("_moduleFee + _treasuryFee")`
+      `ErrorValueOver100Percent("_moduleFee + _treasuryFee")`
     )
 
     await assertRevert(
       stakingRouter.updateStakingModule(module1.id, 10000, 300, 10001, { from: voting }),
-      `ed with custom error 'ErrorValueOver100Percent("_moduleFee + _treasuryFee")`
+      `ErrorValueOver100Percent("_moduleFee + _treasuryFee")`
     )
 
     // distribution fee calculates on active keys in modules
@@ -608,7 +610,7 @@ contract('Lido', ([appManager, voting, user1, user2, user3, nobody, depositor, t
     // can not deposit with unset withdrawalCredentials
     await assertRevert(
       app.methods['deposit(uint256,uint24,bytes)'](MAX_DEPOSITS, CURATED_MODULE_ID, CALLDATA, { from: depositor }),
-      `ed with custom error 'ErrorEmptyWithdrawalsCredentials()`
+      'ErrorEmptyWithdrawalsCredentials()'
     )
     // set withdrawalCredentials with keys, because they were trimmed
     await stakingRouter.setWithdrawalCredentials(pad('0x0202', 32), { from: voting })

diff --git a/test/0.4.24/lidoHandleOracleReport.test.js b/test/0.4.24/lidoHandleOracleReport.test.js
@@ -1,6 +1,7 @@
 const { assert } = require('chai')
 const { newDao, newApp } = require('./helpers/dao')
-const { assertBn, assertRevert } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 
 const LidoPushableMock = artifacts.require('LidoPushableMock.sol')
 const OracleMock = artifacts.require('OracleMock.sol')

diff --git a/test/0.4.24/lidooracle.test.js b/test/0.4.24/lidooracle.test.js
@@ -1,6 +1,8 @@
 const { assert } = require('chai')
 const { newDao, newApp } = require('./helpers/dao')
-const { assertBn, assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
+
 const { toBN } = require('../helpers/utils')
 const { ZERO_ADDRESS } = require('@aragon/contract-helpers-test')
 const keccak256 = require('js-sha3').keccak_256

diff --git a/test/0.4.24/node-operators-registry.test.js b/test/0.4.24/node-operators-registry.test.js
@@ -4,7 +4,9 @@ const { hexSplit, toBN } = require('../helpers/utils')
 const { newDao, newApp } = require('./helpers/dao')
 const { EvmSnapshot } = require('../helpers/blockchain')
 const { ZERO_ADDRESS, getEventAt } = require('@aragon/contract-helpers-test')
-const { assertBn, assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
+
 const keccak256 = require('js-sha3').keccak_256
 const nodeOperators = require('../helpers/node-operators')
 

diff --git a/test/0.4.24/staking-limit.test.js b/test/0.4.24/staking-limit.test.js
@@ -1,5 +1,6 @@
 const { assert } = require('chai')
-const { assertBn, assertRevert } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 const { bn, MAX_UINT256 } = require('@aragon/contract-helpers-test')
 const { toBN } = require('../helpers/utils')
 const { waitBlocks } = require('../helpers/blockchain')

diff --git a/test/0.4.24/steth.test.js b/test/0.4.24/steth.test.js
@@ -1,5 +1,6 @@
 const { assert } = require('chai')
-const { assertBn, assertRevert, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 const { ZERO_ADDRESS, bn } = require('@aragon/contract-helpers-test')
 
 const StETH = artifacts.require('StETHMock')

diff --git a/test/0.8.9/composite-post-rebase-beacon-receiver.test.js b/test/0.8.9/composite-post-rebase-beacon-receiver.test.js
@@ -1,6 +1,7 @@
 /* eslint no-unmodified-loop-condition: "warn" */
 
-const { assertBn, assertRevert, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent, assertAmountOfEvents } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 const { ZERO_ADDRESS, bn } = require('@aragon/contract-helpers-test')
 
 const CompositePostRebaseBeaconReceiver = artifacts.require('CompositePostRebaseBeaconReceiver.sol')

diff --git a/test/0.8.9/deposit-security-module.test.js b/test/0.8.9/deposit-security-module.test.js
@@ -1,5 +1,6 @@
 const hre = require('hardhat')
-const { assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 const { assert } = require('chai')
 const { DSMAttestMessage, DSMPauseMessage } = require('./helpers/signatures')
 const { ZERO_ADDRESS } = require('@aragon/contract-helpers-test')

diff --git a/test/0.8.9/lido-exec-layer-rewards-vault.js b/test/0.8.9/lido-exec-layer-rewards-vault.js
@@ -1,6 +1,7 @@
 const { assert } = require('chai')
 
-const { assertBn, assertRevert, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertBn, assertEvent } = require('@aragon/contract-helpers-test/src/asserts')
+const { assertRevert } = require('../helpers/assertThrow')
 const { ZERO_ADDRESS, bn } = require('@aragon/contract-helpers-test')
 const { newDao, newApp } = require('../0.4.24/helpers/dao')
 const { StETH, ETH } = require('../helpers/utils')