Skip to content

Commit c13eb6e

Browse files
ypatil12ypatil12
authored
EigenPodManager Unit Test Refactor (#290)
* skeleton refactor * finished EPM unit tests * fix tree diagram typos * fix _checkPodDeployed function * split max pod revert tests * fuzz removeShares tests * update initializePodWithShares to not use stdStorage * add error messages on asserts * fix tree file name * add share adjustment tests * create temp file for pod and pod manager unit tests * remove unused constant in EPM unit test
1 parent 814e850 commit c13eb6e

File tree

8 files changed

+838
-256
lines changed

8 files changed

+838
-256
lines changed

‎src/contracts/pods/EigenPodManager.sol‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,13 +142,13 @@ contract EigenPodManager is
142142
* result in the `podOwner` incurring a "share deficit". This behavior prevents a Staker from queuing a withdrawal which improperly removes excessive
143143
* shares from the operator to whom the staker is delegated.
144144
* @dev Reverts if `shares` is not a whole Gwei amount
145+
* @dev The delegation manager validates that the podOwner is not address(0)
145146
*/
146147
function removeShares(
147148
address podOwner,
148149
uint256 shares
149150
) external onlyDelegationManager {
150-
require(podOwner != address(0), "EigenPodManager.removeShares: podOwner cannot be zero address");
151-
require(int256(shares) >= 0, "EigenPodManager.removeShares: shares amount is negative");
151+
require(int256(shares) >= 0, "EigenPodManager.removeShares: shares cannot be negative");
152152
require(shares % GWEI_TO_WEI == 0, "EigenPodManager.removeShares: shares must be a whole Gwei amount");
153153
int256 updatedPodOwnerShares = podOwnerShares[podOwner] - int256(shares);
154154
require(updatedPodOwnerShares >= 0, "EigenPodManager.removeShares: cannot result in pod owner having negative shares");
@@ -180,6 +180,8 @@ contract EigenPodManager is
180180
* @notice Used by the DelegationManager to complete a withdrawal, sending tokens to some destination address
181181
* @dev Prioritizes decreasing the podOwner's share deficit, if they have one
182182
* @dev Reverts if `shares` is not a whole Gwei amount
183+
* @dev This function assumes that `removeShares` has already been called by the delegationManager, hence why
184+
* we do not need to update the podOwnerShares if `currentPodOwnerShares` is positive
183185
*/
184186
function withdrawSharesAsTokens(
185187
address podOwner,

‎src/test/EigenPod.t.sol‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2126,3 +2126,140 @@ contract Relayer is Test {
21262126
BeaconChainProofs.verifyWithdrawal(beaconStateRoot, withdrawalFields, proofs);
21272127
}
21282128
}
2129+
2130+
2131+
//TODO: Integration Tests from old EPM unit tests:
2132+
// queues a withdrawal of "beacon chain ETH shares" from this address to itself
2133+
// fuzzed input amountGwei is sized-down, since it must be in GWEI and gets sized-up to be WEI
2134+
// TODO: reimplement similar test
2135+
// function testQueueWithdrawalBeaconChainETHToSelf(uint128 amountGwei)
2136+
// public returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory, bytes32 /*withdrawalRoot*/)
2137+
// {
2138+
// // scale fuzzed amount up to be a whole amount of GWEI
2139+
// uint256 amount = uint256(amountGwei) * 1e9;
2140+
// address staker = address(this);
2141+
// address withdrawer = staker;
2142+
2143+
// testRestakeBeaconChainETHSuccessfully(staker, amount);
2144+
2145+
// (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) =
2146+
// _createQueuedWithdrawal(staker, amount, withdrawer);
2147+
2148+
// return (queuedWithdrawal, withdrawalRoot);
2149+
// }
2150+
// TODO: reimplement similar test
2151+
// function testQueueWithdrawalBeaconChainETHToDifferentAddress(address withdrawer, uint128 amountGwei)
2152+
// public
2153+
// filterFuzzedAddressInputs(withdrawer)
2154+
// returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory, bytes32 /*withdrawalRoot*/)
2155+
// {
2156+
// // scale fuzzed amount up to be a whole amount of GWEI
2157+
// uint256 amount = uint256(amountGwei) * 1e9;
2158+
// address staker = address(this);
2159+
2160+
// testRestakeBeaconChainETHSuccessfully(staker, amount);
2161+
2162+
// (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) =
2163+
// _createQueuedWithdrawal(staker, amount, withdrawer);
2164+
2165+
// return (queuedWithdrawal, withdrawalRoot);
2166+
// }
2167+
// TODO: reimplement similar test
2168+
2169+
// function testQueueWithdrawalBeaconChainETHFailsNonWholeAmountGwei(uint256 nonWholeAmount) external {
2170+
// // this also filters out the zero case, which will revert separately
2171+
// cheats.assume(nonWholeAmount % GWEI_TO_WEI != 0);
2172+
// cheats.expectRevert(bytes("EigenPodManager._queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH for an non-whole amount of gwei"));
2173+
// eigenPodManager.queueWithdrawal(nonWholeAmount, address(this));
2174+
// }
2175+
2176+
// function testQueueWithdrawalBeaconChainETHFailsZeroAmount() external {
2177+
// cheats.expectRevert(bytes("EigenPodManager._queueWithdrawal: amount must be greater than zero"));
2178+
// eigenPodManager.queueWithdrawal(0, address(this));
2179+
// }
2180+
2181+
// TODO: reimplement similar test
2182+
// function testCompleteQueuedWithdrawal() external {
2183+
// address staker = address(this);
2184+
// uint256 withdrawalAmount = 1e18;
2185+
2186+
// // withdrawalAmount is converted to GWEI here
2187+
// (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) =
2188+
// testQueueWithdrawalBeaconChainETHToSelf(uint128(withdrawalAmount / 1e9));
2189+
2190+
// IEigenPod eigenPod = eigenPodManager.getPod(staker);
2191+
// uint256 eigenPodBalanceBefore = address(eigenPod).balance;
2192+
2193+
// uint256 middlewareTimesIndex = 0;
2194+
2195+
// // actually complete the withdrawal
2196+
// cheats.startPrank(staker);
2197+
// cheats.expectEmit(true, true, true, true, address(eigenPodManager));
2198+
// emit BeaconChainETHWithdrawalCompleted(
2199+
// queuedWithdrawal.podOwner,
2200+
// queuedWithdrawal.shares,
2201+
// queuedWithdrawal.nonce,
2202+
// queuedWithdrawal.delegatedAddress,
2203+
// queuedWithdrawal.withdrawer,
2204+
// withdrawalRoot
2205+
// );
2206+
// eigenPodManager.completeQueuedWithdrawal(queuedWithdrawal, middlewareTimesIndex);
2207+
// cheats.stopPrank();
2208+
2209+
// // TODO: make EigenPodMock do something so we can verify that it gets called appropriately?
2210+
// uint256 eigenPodBalanceAfter = address(eigenPod).balance;
2211+
2212+
// // verify that the withdrawal root does bit exist after queuing
2213+
// require(!eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is true!");
2214+
// }
2215+
2216+
// TODO: reimplement similar test
2217+
// // creates a queued withdrawal of "beacon chain ETH shares", from `staker`, of `amountWei`, "to" the `withdrawer`
2218+
// function _createQueuedWithdrawal(address staker, uint256 amountWei, address withdrawer)
2219+
// internal
2220+
// returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot)
2221+
// {
2222+
// // create the struct, for reference / to return
2223+
// queuedWithdrawal = IEigenPodManager.BeaconChainQueuedWithdrawal({
2224+
// shares: amountWei,
2225+
// podOwner: staker,
2226+
// nonce: eigenPodManager.cumulativeWithdrawalsQueued(staker),
2227+
// startBlock: uint32(block.number),
2228+
// delegatedTo: delegationManagerMock.delegatedTo(staker),
2229+
// withdrawer: withdrawer
2230+
// });
2231+
2232+
// // verify that the withdrawal root does not exist before queuing
2233+
// require(!eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is true!");
2234+
2235+
// // get staker nonce and shares before queuing
2236+
// uint256 nonceBefore = eigenPodManager.cumulativeWithdrawalsQueued(staker);
2237+
// int256 sharesBefore = eigenPodManager.podOwnerShares(staker);
2238+
2239+
// // actually create the queued withdrawal, and check for event emission
2240+
// cheats.startPrank(staker);
2241+
2242+
// cheats.expectEmit(true, true, true, true, address(eigenPodManager));
2243+
// emit BeaconChainETHWithdrawalQueued(
2244+
// queuedWithdrawal.podOwner,
2245+
// queuedWithdrawal.shares,
2246+
// queuedWithdrawal.nonce,
2247+
// queuedWithdrawal.delegatedAddress,
2248+
// queuedWithdrawal.withdrawer,
2249+
// eigenPodManager.calculateWithdrawalRoot(queuedWithdrawal)
2250+
// );
2251+
// withdrawalRoot = eigenPodManager.queueWithdrawal(amountWei, withdrawer);
2252+
// cheats.stopPrank();
2253+
2254+
// // verify that the withdrawal root does exist after queuing
2255+
// require(eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is false!");
2256+
2257+
// // verify that staker nonce incremented correctly and shares decremented correctly
2258+
// uint256 nonceAfter = eigenPodManager.cumulativeWithdrawalsQueued(staker);
2259+
// int256 sharesAfter = eigenPodManager.podOwnerShares(staker);
2260+
// require(nonceAfter == nonceBefore + 1, "nonce did not increment correctly on queuing withdrawal");
2261+
// require(sharesAfter + amountWei == sharesBefore, "shares did not decrement correctly on queuing withdrawal");
2262+
2263+
// return (queuedWithdrawal, withdrawalRoot);
2264+
// }
2265+

‎src/test/events/IEigenPodManagerEvents.sol‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity =0.8.12;
3+
4+
interface IEigenPodManagerEvents {
5+
/// @notice Emitted to notify the update of the beaconChainOracle address
6+
event BeaconOracleUpdated(address indexed newOracleAddress);
7+
8+
/// @notice Emitted to notify the deployment of an EigenPod
9+
event PodDeployed(address indexed eigenPod, address indexed podOwner);
10+
11+
/// @notice Emitted when `maxPods` value is updated from `previousValue` to `newValue`
12+
event MaxPodsUpdated(uint256 previousValue, uint256 newValue);
13+
}

‎src/test/harnesses/EigenPodManagerWrapper.sol‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity =0.8.12;
3+
4+
import "../../contracts/pods/EigenPodManager.sol";
5+
6+
///@notice This contract exposed the internal `_calculateChangeInDelegatableShares` function for testing
7+
contract EigenPodManagerWrapper is EigenPodManager {
8+
9+
constructor(
10+
IETHPOSDeposit _ethPOS,
11+
IBeacon _eigenPodBeacon,
12+
IStrategyManager _strategyManager,
13+
ISlasher _slasher,
14+
IDelegationManager _delegationManager
15+
) EigenPodManager(_ethPOS, _eigenPodBeacon, _strategyManager, _slasher, _delegationManager) {}
16+
17+
function calculateChangeInDelegatableShares(int256 sharesBefore, int256 sharesAfter) external pure returns (int256) {
18+
return _calculateChangeInDelegatableShares(sharesBefore, sharesAfter);
19+
}
20+
}

‎src/test/tree/EigenPodManagerUnit.tree‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
├── EigenPodManager Tree (*** denotes that integrationt tests are needed to validate path)
2+
├── when contract is deployed and initialized
3+
│ └── it should properly set storage
4+
├── when initialize called again
5+
│ └── it should revert
6+
├── when createPod called
7+
│ ├── given the user has already created a pod
8+
│ │ └── it should revert
9+
│ ├── given that the max number of pods has been deployed
10+
│ │ └── it should revert
11+
│ └── given the user has not created a pod
12+
│ └── it should deploy a pod
13+
├── when stake is called
14+
│ ├── given the user has not created a pod
15+
│ │ └── it should deploy a pod
16+
│ └── given the user has already created a pod
17+
│ └── it should call stake on the eigenPod
18+
├── when setMaxPods is called
19+
│ ├── given the user is not the pauser
20+
│ │ └── it should revert
21+
│ └── given the user is the pauser
22+
│ └── it should set the max pods
23+
├── when updateBeaconChainOracle is called
24+
│ ├── given the user is not the owner
25+
│ │ └── it should revert
26+
│ └── given the user is the owner
27+
│ └── it should set the beacon chain oracle
28+
├── when addShares is called
29+
│ ├── given that the caller is not the delegationManager
30+
│ │ └── it should revert
31+
│ ├── given that the podOwner address is 0
32+
│ │ └── it should revert
33+
│ ├── given that the shares amount is negative
34+
│ │ └── it should revert
35+
│ ├── given that the shares is not a whole gwei amount
36+
│ │ └── it should revert
37+
│ └── given that all of the above conditions are satisfied
38+
│ └── it should update the podOwnerShares
39+
├── when removeShares is called
40+
│ ├── given that the caller is not the delegationManager
41+
│ │ └── it should revert
42+
│ ├── given that the shares amount is negative
43+
│ │ └── it should revert
44+
│ ├── given that the shares is not a whole gwei amount
45+
│ │ └── it should revert
46+
│ ├── given that removing shares results in the pod owner having negative shares
47+
│ │ └── it should revert
48+
│ └── given that all of the above conditions are satisfied
49+
│ └── it should update the podOwnerShares
50+
├── when withdrawSharesAsTokens is called
51+
│ ├── given that the podOwner is address 0
52+
│ │ └── it should revert
53+
│ ├── given that the destination is address 0
54+
│ │ └── it should revert
55+
│ ├── given that the shares amount is negative
56+
│ │ └── it should revert
57+
│ ├── given that the shares is not a whole gwei amount
58+
│ │ └── it should revert
59+
│ ├── given that the current podOwner shares are negative
60+
│ │ ├── given that the shares to withdraw are larger in magnitude than the shares of the podOwner
61+
│ │ │ └── it should set the podOwnerShares to 0 and decrement shares to withdraw by the share deficit
62+
│ │ └── given that the shares to withdraw are smaller in magnitude than shares of the podOwner
63+
│ │ └── it should increment the podOwner shares by the shares to withdraw
64+
│ └── given that the pod owner shares are positive
65+
│ └── it should withdraw restaked ETH from the eigenPod
66+
├── when shares are adjusted
67+
│ ├── given that sharesBefore is negative or 0
68+
│ │ ├── given that sharesAfter is negative or zero
69+
│ │ │ └── the change in delegateable shares should be 0
70+
│ │ └── given that sharesAfter is positive
71+
│ │ └── the change in delegateable shares should be positive
72+
│ └── given that sharesBefore is positive
73+
│ ├── given that sharesAfter is negative or zero
74+
│ │ └── the change in delegateable shares is negative sharesBefore
75+
│ └── given that sharesAfter is positive
76+
│ └── the change in delegateable shares is the difference between sharesAfter and sharesBefore
77+
└── when recordBeaconChainETHBalanceUpdate is called
78+
├── given that the podOwner's eigenPod is not the caller
79+
│ └── it should revert
80+
├── given that the podOwner is a zero address
81+
│ └── it should revert
82+
├── given that sharesDelta is not a whole gwei amount
83+
│ ├── it should revert
84+
│ └── given that the shares delta is valid
85+
│ └── it should update the podOwnerShares
86+
├── given that the change in delegateable shares is positive ***
87+
│ └── it should increase delegated shares on the delegationManager
88+
├── given that the change in delegateable shares is negative ***
89+
│ └── it should decrease delegated shares on the delegationManager
90+
├── given that the change in delegateable shares is 0 ***
91+
│ └── it should only update the podOwnerShares
92+
└── given that the function is reentered ***
93+
└── it should revert

‎src/test/unit/EigenPod-PodManagerUnit.t.sol‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
///@notice Placeholder for future unit tests that combine interaction between the EigenPod & EigenPodManager
2+
3+
// TODO: salvage / re-implement a check for reentrancy guard on functions, as possible
4+
// function testRecordBeaconChainETHBalanceUpdateFailsWhenReentering() public {
5+
// uint256 amount = 1e18;
6+
// uint256 amount2 = 2e18;
7+
// address staker = address(this);
8+
// uint256 beaconChainETHStrategyIndex = 0;
9+
10+
// _beaconChainReentrancyTestsSetup();
11+
12+
// testRestakeBeaconChainETHSuccessfully(staker, amount);
13+
14+
// address targetToUse = address(strategyManager);
15+
// uint256 msgValueToUse = 0;
16+
17+
// int256 amountDelta = int256(amount2 - amount);
18+
// // reference: function recordBeaconChainETHBalanceUpdate(address podOwner, uint256 beaconChainETHStrategyIndex, uint256 sharesDelta, bool isNegative)
19+
// bytes memory calldataToUse = abi.encodeWithSelector(StrategyManager.recordBeaconChainETHBalanceUpdate.selector, staker, beaconChainETHStrategyIndex, amountDelta);
20+
// reenterer.prepare(targetToUse, msgValueToUse, calldataToUse, bytes("ReentrancyGuard: reentrant call"));
21+
22+
// cheats.startPrank(address(reenterer));
23+
// eigenPodManager.recordBeaconChainETHBalanceUpdate(staker, amountDelta);
24+
// cheats.stopPrank();
25+
// }
26+
27+
// function _beaconChainReentrancyTestsSetup() internal {
28+
// // prepare EigenPodManager with StrategyManager and Delegation replaced with a Reenterer contract
29+
// reenterer = new Reenterer();
30+
// eigenPodManagerImplementation = new EigenPodManager(
31+
// ethPOSMock,
32+
// eigenPodBeacon,
33+
// IStrategyManager(address(reenterer)),
34+
// slasherMock,
35+
// IDelegationManager(address(reenterer))
36+
// );
37+
// eigenPodManager = EigenPodManager(
38+
// address(
39+
// new TransparentUpgradeableProxy(
40+
// address(eigenPodManagerImplementation),
41+
// address(proxyAdmin),
42+
// abi.encodeWithSelector(
43+
// EigenPodManager.initialize.selector,
44+
// type(uint256).max /*maxPods*/,
45+
// IBeaconChainOracle(address(0)) /*beaconChainOracle*/,
46+
// initialOwner,
47+
// pauserRegistry,
48+
// 0 /*initialPausedStatus*/
49+
// )
50+
// )
51+
// )
52+
// );
53+
// }

0 commit comments

Comments
 (0)