EigenPodManager Unit Test Refactor

src/contracts/pods/EigenPodManager.sol

@@ -142,13 +142,13 @@ contract EigenPodManager is
      * result in the `podOwner` incurring a "share deficit". This behavior prevents a Staker from queuing a withdrawal which improperly removes excessive
      * shares from the operator to whom the staker is delegated.
      * @dev Reverts if `shares` is not a whole Gwei amount
+     * @dev The delegation manager validates that the podOwner is not address(0)
      */
     function removeShares(
         address podOwner, 
         uint256 shares
     ) external onlyDelegationManager {
-        require(podOwner != address(0), "EigenPodManager.removeShares: podOwner cannot be zero address");
-        require(int256(shares) >= 0, "EigenPodManager.removeShares: shares amount is negative");
+        require(int256(shares) >= 0, "EigenPodManager.removeShares: shares cannot be negative");
         require(shares % GWEI_TO_WEI == 0, "EigenPodManager.removeShares: shares must be a whole Gwei amount");
         int256 updatedPodOwnerShares = podOwnerShares[podOwner] - int256(shares);
         require(updatedPodOwnerShares >= 0, "EigenPodManager.removeShares: cannot result in pod owner having negative shares");
@@ -180,6 +180,8 @@ contract EigenPodManager is
      * @notice Used by the DelegationManager to complete a withdrawal, sending tokens to some destination address
      * @dev Prioritizes decreasing the podOwner's share deficit, if they have one
      * @dev Reverts if `shares` is not a whole Gwei amount
+     * @dev This function assumes that `removeShares` has already been called by the delegationManager, hence why
+     *      we do not need to update the podOwnerShares if `currentPodOwnerShares` is positive
      */
     function withdrawSharesAsTokens(
         address podOwner, 

src/test/EigenPod.t.sol

@@ -2126,3 +2126,140 @@ contract Relayer is Test {
         BeaconChainProofs.verifyWithdrawal(beaconStateRoot, withdrawalFields, proofs);
     }
 }
+
+
+//TODO: Integration Tests from old EPM unit tests:
+    // queues a withdrawal of "beacon chain ETH shares" from this address to itself
+    // fuzzed input amountGwei is sized-down, since it must be in GWEI and gets sized-up to be WEI
+// TODO: reimplement similar test
+    // function testQueueWithdrawalBeaconChainETHToSelf(uint128 amountGwei)
+    //     public returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory, bytes32 /*withdrawalRoot*/) 
+    // {
+    //     // scale fuzzed amount up to be a whole amount of GWEI
+    //     uint256 amount = uint256(amountGwei) * 1e9;
+    //     address staker = address(this);
+    //     address withdrawer = staker;
+
+    //     testRestakeBeaconChainETHSuccessfully(staker, amount);
+
+    //     (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) =
+    //         _createQueuedWithdrawal(staker, amount, withdrawer);
+
+    //     return (queuedWithdrawal, withdrawalRoot);
+    // }
+// TODO: reimplement similar test
+    // function testQueueWithdrawalBeaconChainETHToDifferentAddress(address withdrawer, uint128 amountGwei)
+    //     public
+    //     filterFuzzedAddressInputs(withdrawer)
+    //     returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory, bytes32 /*withdrawalRoot*/) 
+    // {
+    //     // scale fuzzed amount up to be a whole amount of GWEI
+    //     uint256 amount = uint256(amountGwei) * 1e9;
+    //     address staker = address(this);
+
+    //     testRestakeBeaconChainETHSuccessfully(staker, amount);
+
+    //     (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) =
+    //         _createQueuedWithdrawal(staker, amount, withdrawer);
+
+    //     return (queuedWithdrawal, withdrawalRoot);
+    // }
+// TODO: reimplement similar test
+
+    // function testQueueWithdrawalBeaconChainETHFailsNonWholeAmountGwei(uint256 nonWholeAmount) external {
+    //     // this also filters out the zero case, which will revert separately
+    //     cheats.assume(nonWholeAmount % GWEI_TO_WEI != 0);
+    //     cheats.expectRevert(bytes("EigenPodManager._queueWithdrawal: cannot queue a withdrawal of Beacon Chain ETH for an non-whole amount of gwei"));
+    //     eigenPodManager.queueWithdrawal(nonWholeAmount, address(this));
+    // }
+
+    // function testQueueWithdrawalBeaconChainETHFailsZeroAmount() external {
+    //     cheats.expectRevert(bytes("EigenPodManager._queueWithdrawal: amount must be greater than zero"));
+    //     eigenPodManager.queueWithdrawal(0, address(this));
+    // }
+
+// TODO: reimplement similar test
+    // function testCompleteQueuedWithdrawal() external {
+    //     address staker = address(this);
+    //     uint256 withdrawalAmount = 1e18;
+
+    //     // withdrawalAmount is converted to GWEI here
+    //     (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot) = 
+    //         testQueueWithdrawalBeaconChainETHToSelf(uint128(withdrawalAmount / 1e9));
+
+    //     IEigenPod eigenPod = eigenPodManager.getPod(staker);
+    //     uint256 eigenPodBalanceBefore = address(eigenPod).balance;
+
+    //     uint256 middlewareTimesIndex = 0;
+
+    //     // actually complete the withdrawal
+    //     cheats.startPrank(staker);
+    //     cheats.expectEmit(true, true, true, true, address(eigenPodManager));
+    //     emit BeaconChainETHWithdrawalCompleted(
+    //         queuedWithdrawal.podOwner,
+    //         queuedWithdrawal.shares,
+    //         queuedWithdrawal.nonce,
+    //         queuedWithdrawal.delegatedAddress,
+    //         queuedWithdrawal.withdrawer,
+    //         withdrawalRoot
+    //     );
+    //     eigenPodManager.completeQueuedWithdrawal(queuedWithdrawal, middlewareTimesIndex);
+    //     cheats.stopPrank();
+
+    //     // TODO: make EigenPodMock do something so we can verify that it gets called appropriately?
+    //     uint256 eigenPodBalanceAfter = address(eigenPod).balance;
+
+    //     // verify that the withdrawal root does bit exist after queuing
+    //     require(!eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is true!");
+    // }
+
+// TODO: reimplement similar test
+    // // creates a queued withdrawal of "beacon chain ETH shares", from `staker`, of `amountWei`, "to" the `withdrawer`
+    // function _createQueuedWithdrawal(address staker, uint256 amountWei, address withdrawer)
+    //     internal
+    //     returns (IEigenPodManager.BeaconChainQueuedWithdrawal memory queuedWithdrawal, bytes32 withdrawalRoot)
+    // {
+    //     // create the struct, for reference / to return
+    //     queuedWithdrawal = IEigenPodManager.BeaconChainQueuedWithdrawal({
+    //         shares: amountWei,
+    //         podOwner: staker,
+    //         nonce: eigenPodManager.cumulativeWithdrawalsQueued(staker),
+    //         startBlock: uint32(block.number),
+    //         delegatedTo: delegationManagerMock.delegatedTo(staker),
+    //         withdrawer: withdrawer
+    //     });
+
+    //     // verify that the withdrawal root does not exist before queuing
+    //     require(!eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is true!");
+
+    //     // get staker nonce and shares before queuing
+    //     uint256 nonceBefore = eigenPodManager.cumulativeWithdrawalsQueued(staker);
+    //     int256 sharesBefore = eigenPodManager.podOwnerShares(staker);
+
+    //     // actually create the queued withdrawal, and check for event emission
+    //     cheats.startPrank(staker);
+
+    //     cheats.expectEmit(true, true, true, true, address(eigenPodManager));
+    //     emit BeaconChainETHWithdrawalQueued(
+    //         queuedWithdrawal.podOwner,
+    //         queuedWithdrawal.shares,
+    //         queuedWithdrawal.nonce,
+    //         queuedWithdrawal.delegatedAddress,
+    //         queuedWithdrawal.withdrawer,
+    //         eigenPodManager.calculateWithdrawalRoot(queuedWithdrawal)
+    //     );
+    //     withdrawalRoot = eigenPodManager.queueWithdrawal(amountWei, withdrawer);
+    //     cheats.stopPrank();
+
+    //     // verify that the withdrawal root does exist after queuing
+    //     require(eigenPodManager.withdrawalRootPending(withdrawalRoot), "withdrawalRootPendingBefore is false!");
+
+    //     // verify that staker nonce incremented correctly and shares decremented correctly
+    //     uint256 nonceAfter = eigenPodManager.cumulativeWithdrawalsQueued(staker);
+    //     int256 sharesAfter = eigenPodManager.podOwnerShares(staker);
+    //     require(nonceAfter == nonceBefore + 1, "nonce did not increment correctly on queuing withdrawal");
+    //     require(sharesAfter + amountWei == sharesBefore, "shares did not decrement correctly on queuing withdrawal");
+
+    //     return (queuedWithdrawal, withdrawalRoot);
+    // }
+

src/test/events/IEigenPodManagerEvents.sol

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity =0.8.12;
+
+interface IEigenPodManagerEvents {
+    /// @notice Emitted to notify the update of the beaconChainOracle address
+    event BeaconOracleUpdated(address indexed newOracleAddress);
+
+    /// @notice Emitted to notify the deployment of an EigenPod
+    event PodDeployed(address indexed eigenPod, address indexed podOwner);
+
+    /// @notice Emitted when `maxPods` value is updated from `previousValue` to `newValue`
+    event MaxPodsUpdated(uint256 previousValue, uint256 newValue);
+}

src/test/harnesses/EigenPodManagerWrapper.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity =0.8.12;
+
+import "../../contracts/pods/EigenPodManager.sol";
+
+///@notice This contract exposed the internal `_calculateChangeInDelegatableShares` function for testing
+contract EigenPodManagerWrapper is EigenPodManager {
+
+    constructor(
+        IETHPOSDeposit _ethPOS,
+        IBeacon _eigenPodBeacon,
+        IStrategyManager _strategyManager,
+        ISlasher _slasher,
+        IDelegationManager _delegationManager
+    ) EigenPodManager(_ethPOS, _eigenPodBeacon, _strategyManager, _slasher, _delegationManager) {}
+
+    function calculateChangeInDelegatableShares(int256 sharesBefore, int256 sharesAfter) external pure returns (int256) {
+        return _calculateChangeInDelegatableShares(sharesBefore, sharesAfter);
+    }
+}

src/test/tree/EigenPodManagerUnit.tree

@@ -0,0 +1,93 @@
+├── EigenPodManager Tree (*** denotes that integrationt tests are needed to validate path)
+├── when contract is deployed and initialized
+│   └── it should properly set storage
+├── when initialize called again
+│   └── it should revert
+├── when createPod called
+│   ├── given the user has already created a pod
+│   │   └── it should revert
+│   ├── given that the max number of pods has been deployed
+│   │   └── it should revert
+│   └── given the user has not created a pod
+│       └── it should deploy a pod
+├── when stake is called
+│   ├── given the user has not created a pod
+│   │   └── it should deploy a pod
+│   └── given the user has already created a pod
+│       └── it should call stake on the eigenPod
+├── when setMaxPods is called
+│   ├── given the user is not the pauser
+│   │   └── it should revert
+│   └── given the user is the pauser
+│       └── it should set the max pods
+├── when updateBeaconChainOracle is called
+│   ├── given the user is not the owner
+│   │   └── it should revert
+│   └── given the user is the owner
+│       └── it should set the beacon chain oracle
+├── when addShares is called
+│   ├── given that the caller is not the delegationManager
+│   │   └── it should revert
+│   ├── given that the podOwner address is 0
+│   │   └── it should revert
+│   ├── given that the shares amount is negative
+│   │   └── it should revert
+│   ├── given that the shares is not a whole gwei amount
+│   │   └── it should revert
+│   └── given that all of the above conditions are satisfied
+│       └── it should update the podOwnerShares
+├── when removeShares is called
+│   ├── given that the caller is not the delegationManager
+│   │   └── it should revert
+│   ├── given that the shares amount is negative
+│   │   └── it should revert
+│   ├── given that the shares is not a whole gwei amount
+│   │   └── it should revert
+│   ├── given that removing shares results in the pod owner having negative shares
+│   │   └── it should revert
+│   └── given that all of the above conditions are satisfied
+│       └── it should update the podOwnerShares
+├── when withdrawSharesAsTokens is called
+│   ├── given that the podOwner is address 0
+│   │   └── it should revert
+│   ├── given that the destination is address 0
+│   │   └── it should revert
+│   ├── given that the shares amount is negative
+│   │   └── it should revert
+│   ├── given that the shares is not a whole gwei amount
+│   │   └── it should revert
+│   ├── given that the current podOwner shares are negative
+│   │   ├── given that the shares to withdraw are larger in magnitude than the shares of the podOwner
+│   │   │   └── it should set the podOwnerShares to 0 and decrement shares to withdraw by the share deficit
+│   │   └── given that the shares to withdraw are smaller in magnitude than shares of the podOwner
+│   │       └── it should increment the podOwner shares by the shares to withdraw
+│   └── given that the pod owner shares are positive
+│       └── it should withdraw restaked ETH from the eigenPod
+├── when shares are adjusted
+│   ├── given that sharesBefore is negative or 0
+│   │   ├── given that sharesAfter is negative or zero
+│   │   │   └── the change in delegateable shares should be 0
+│   │   └── given that sharesAfter is positive
+│   │       └── the change in delegateable shares should be positive
+│   └── given that sharesBefore is positive
+│       ├── given that sharesAfter is negative or zero
+│       │   └── the change in delegateable shares is negative sharesBefore
+│       └── given that sharesAfter is positive
+│           └── the change in delegateable shares is the difference between sharesAfter and sharesBefore
+└── when recordBeaconChainETHBalanceUpdate is called
+    ├── given that the podOwner's eigenPod is not the caller
+    │   └── it should revert
+    ├── given that the podOwner is a zero address
+    │   └── it should revert
+    ├── given that sharesDelta is not a whole gwei amount
+    │   ├── it should revert
+    │   └── given that the shares delta is valid
+    │       └── it should update the podOwnerShares
+    ├── given that the change in delegateable shares is positive ***
+    │   └── it should increase delegated shares on the delegationManager
+    ├── given that the change in delegateable shares is negative ***
+    │   └── it should decrease delegated shares on the delegationManager
+    ├── given that the change in delegateable shares is 0 ***
+    │   └── it should only update the podOwnerShares
+    └── given that the function is reentered ***
+        └── it should revert

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

@@ -0,0 +1,53 @@
+///@notice Placeholder for future unit tests that combine interaction between the EigenPod & EigenPodManager
+
+// TODO: salvage / re-implement a check for reentrancy guard on functions, as possible
+    // function testRecordBeaconChainETHBalanceUpdateFailsWhenReentering() public {
+    //     uint256 amount = 1e18;
+    //     uint256 amount2 = 2e18;
+    //     address staker = address(this);
+    //     uint256 beaconChainETHStrategyIndex = 0;
+
+    //     _beaconChainReentrancyTestsSetup();
+
+    //     testRestakeBeaconChainETHSuccessfully(staker, amount);        
+
+    //     address targetToUse = address(strategyManager);
+    //     uint256 msgValueToUse = 0;
+
+    //     int256 amountDelta = int256(amount2 - amount);
+    //     // reference: function recordBeaconChainETHBalanceUpdate(address podOwner, uint256 beaconChainETHStrategyIndex, uint256 sharesDelta, bool isNegative)
+    //     bytes memory calldataToUse = abi.encodeWithSelector(StrategyManager.recordBeaconChainETHBalanceUpdate.selector, staker, beaconChainETHStrategyIndex, amountDelta);
+    //     reenterer.prepare(targetToUse, msgValueToUse, calldataToUse, bytes("ReentrancyGuard: reentrant call"));
+
+    //     cheats.startPrank(address(reenterer));
+    //     eigenPodManager.recordBeaconChainETHBalanceUpdate(staker, amountDelta);
+    //     cheats.stopPrank();
+    // }
+
+    // function _beaconChainReentrancyTestsSetup() internal {
+    //     // prepare EigenPodManager with StrategyManager and Delegation replaced with a Reenterer contract
+    //     reenterer = new Reenterer();
+    //     eigenPodManagerImplementation = new EigenPodManager(
+    //         ethPOSMock,
+    //         eigenPodBeacon,
+    //         IStrategyManager(address(reenterer)),
+    //         slasherMock,
+    //         IDelegationManager(address(reenterer))
+    //     );
+    //     eigenPodManager = EigenPodManager(
+    //         address(
+    //             new TransparentUpgradeableProxy(
+    //                 address(eigenPodManagerImplementation),
+    //                 address(proxyAdmin),
+    //                 abi.encodeWithSelector(
+    //                     EigenPodManager.initialize.selector,
+    //                     type(uint256).max /*maxPods*/,
+    //                     IBeaconChainOracle(address(0)) /*beaconChainOracle*/,
+    //                     initialOwner,
+    //                     pauserRegistry,
+    //                     0 /*initialPausedStatus*/
+    //                 )
+    //             )
+    //         )
+    //     );
+    // }