From e7da52643b3c004683978363a5a75154fbd23f6b Mon Sep 17 00:00:00 2001 From: "clandestine.eth" <96172957+0xClandestine@users.noreply.github.com> Date: Tue, 1 Oct 2024 13:34:57 -0400 Subject: [PATCH] refactor: review changes --- src/contracts/core/AllocationManager.sol | 146 +++++++----------- .../core/AllocationManagerStorage.sol | 2 +- .../interfaces/IAllocationManager.sol | 2 +- src/test/events/IRewardsCoordinatorEvents.sol | 30 ++-- src/test/mocks/DelegationManagerMock.sol | 4 +- src/test/mocks/EigenPodManagerMock.sol | 4 +- src/test/mocks/StrategyManagerMock.sol | 4 +- src/test/unit/EigenPodManagerUnit.t.sol | 16 +- src/test/unit/StrategyManagerUnit.t.sol | 12 +- 9 files changed, 97 insertions(+), 123 deletions(-) diff --git a/src/contracts/core/AllocationManager.sol b/src/contracts/core/AllocationManager.sol index 674452dbe..b3dc3ace4 100644 --- a/src/contracts/core/AllocationManager.sol +++ b/src/contracts/core/AllocationManager.sol @@ -128,14 +128,9 @@ contract AllocationManager is } require(delegation.isOperator(operator), OperatorNotRegistered()); - (bool isSet, uint32 delay) = allocationDelay(operator); + (bool isSet, uint32 operatorAllocationDelay) = allocationDelay(operator); require(isSet, UninitializedAllocationDelay()); - // effect timestamp for allocations to take effect. This is configurable by operators - uint32 allocationEffectTimestamp = uint32(block.timestamp) + delay; - // completable timestamp for deallocations - uint32 deallocationEffectTimestamp = uint32(block.timestamp) + DEALLOCATION_DELAY; - for (uint256 i = 0; i < allocations.length; ++i) { // 1. For the given (operator,strategy) clear all pending free magnitude for the strategy and update freeMagnitude _updateFreeMagnitude({ @@ -148,15 +143,60 @@ contract AllocationManager is // where an operator gets slashed from an operatorSet and as a result all the configured allocations have larger // proprtional magnitudes relative to each other. uint64 currentTotalMagnitude = _getLatestTotalMagnitude(operator, allocations[i].strategy); + require(currentTotalMagnitude == allocations[i].expectedTotalMagnitude, InvalidExpectedTotalMagnitude()); // 3. set allocations for the strategy after updating freeMagnitude - _modifyAllocations({ - operator: operator, - allocation: allocations[i], - allocationEffectTimestamp: allocationEffectTimestamp, - deallocationEffectTimestamp: deallocationEffectTimestamp - }); + MagnitudeAllocation calldata allocation = allocations[i]; + + require(allocation.operatorSets.length == allocation.magnitudes.length, InputArrayLengthMismatch()); + + for (uint256 j = 0; j < allocation.operatorSets.length; ++j) { + require( + avsDirectory.isOperatorSet(allocation.operatorSets[j].avs, allocation.operatorSets[j].operatorSetId), + InvalidOperatorSet() + ); + + bytes32 operatorSetKey = _encodeOperatorSet(allocation.operatorSets[j]); + + // Check that there are no pending allocations & deallocations for the operator, operatorSet, strategy + MagnitudeInfo memory mInfo = _getCurrentMagnitudeWithPending(operator, allocation.strategy, operatorSetKey); + require( + block.timestamp >= mInfo.effectTimestamp, + ModificationAlreadyPending() + ); + + // Calculate the new pending diff with this modification + mInfo.pendingMagnitudeDiff = int128(uint128(allocation.magnitudes[j])) - int128(uint128(mInfo.currentMagnitude)); + require(mInfo.pendingMagnitudeDiff != 0, SameMagnitude()); + + // Handle deallocation/allocation and modification effect timestamp + if (mInfo.pendingMagnitudeDiff < 0) { + // This is a deallocation + + // 1. push PendingFreeMagnitude and respective array index into (op,opSet,Strategy) queued deallocations + _pendingDeallocationOperatorSets[operator][allocation.strategy].push(operatorSetKey); + + // 2. Update the effect timestamp for the deallocation + mInfo.effectTimestamp = uint32(block.timestamp) + DEALLOCATION_DELAY; + } else if (mInfo.pendingMagnitudeDiff > 0) { + // This is an allocation + + // 1. decrement free magnitude by incremented amount + uint64 magnitudeToAllocate = uint64(uint128(mInfo.pendingMagnitudeDiff)); + FreeMagnitudeInfo memory freeInfo = operatorFreeMagnitudeInfo[operator][allocation.strategy]; + require(freeInfo.freeMagnitude >= magnitudeToAllocate, InsufficientAllocatableMagnitude()); + freeInfo.freeMagnitude -= magnitudeToAllocate; + + // 2. Update the effectTimestamp for the allocation + mInfo.effectTimestamp = uint32(block.timestamp) + operatorAllocationDelay; + + operatorFreeMagnitudeInfo[operator][allocation.strategy] = freeInfo; + } + + // Allocate magnitude which will take effect at the `effectTimestamp` + _operatorMagnitudeInfo[operator][allocation.strategy][operatorSetKey] = mInfo; + } } } @@ -254,70 +294,6 @@ contract AllocationManager is operatorFreeMagnitudeInfo[operator][strategy] = freeInfo; } - /** - * @notice For a single strategy, modify magnitude allocations for each of the specified operatorSets - * @param operator address to modify allocations for - * @param allocation the magnitude allocations to modify for a single strategy - * @param allocationEffectTimestamp the timestamp when the allocations will take effect - * @param deallocationEffectTimestamp the timestamp when the deallocations will be completable - * @dev For each allocation, allocation.operatorSets MUST be ordered in ascending order according to the - * encoding of the operatorSet. This is to prevent duplicate operatorSets being passed in. The easiest way to ensure - * ordering is to sort allocated operatorSets by address first, and then sort for each avs by ascending operatorSetIds. - */ - function _modifyAllocations( - address operator, - MagnitudeAllocation calldata allocation, - uint32 allocationEffectTimestamp, - uint32 deallocationEffectTimestamp - ) internal { - require(allocation.operatorSets.length == allocation.magnitudes.length, InputArrayLengthMismatch()); - - for (uint256 i = 0; i < allocation.operatorSets.length; ++i) { - require( - avsDirectory.isOperatorSet(allocation.operatorSets[i].avs, allocation.operatorSets[i].operatorSetId), - InvalidOperatorSet() - ); - - bytes32 operatorSetKey = _encodeOperatorSet(allocation.operatorSets[i]); - - // Check that there are no pending allocations & deallocations for the operator, operatorSet, strategy - MagnitudeInfo memory mInfo = _getCurrentMagnitudeWithPending(operator, allocation.strategy, operatorSetKey); - require( - block.timestamp >= mInfo.effectTimestamp, - PendingAllocationOrDeallocation() - ); - - // Calculate the new pending diff with this modification - mInfo.pendingMagnitudeDiff = int128(uint128(allocation.magnitudes[i])) - int128(uint128(mInfo.currentMagnitude)); - require(mInfo.pendingMagnitudeDiff != 0, SameMagnitude()); - - // Handle deallocation/allocation and modification effect timestamp - if (mInfo.pendingMagnitudeDiff < 0) { - // This is a deallocation - - // 1. push PendingFreeMagnitude and respective array index into (op,opSet,Strategy) queued deallocations - _pendingDeallocationOperatorSets[operator][allocation.strategy].push(operatorSetKey); - - // 2. Update the effect timestamp for the deallocation - mInfo.effectTimestamp = deallocationEffectTimestamp; - } else if (mInfo.pendingMagnitudeDiff > 0) { - // This is an allocation - - // 1. decrement free magnitude by incremented amount - uint64 magnitudeToAllocate = uint64(uint128(mInfo.pendingMagnitudeDiff)); - FreeMagnitudeInfo storage freeInfo = operatorFreeMagnitudeInfo[operator][allocation.strategy]; - require(freeInfo.freeMagnitude >= magnitudeToAllocate, InsufficientAllocatableMagnitude()); - freeInfo.freeMagnitude -= magnitudeToAllocate; - - // 2. Update the effectTimestamp for the allocation - mInfo.effectTimestamp = allocationEffectTimestamp; - } - - // Allocate magnitude which will take effect at the `effectTimestamp` - _operatorMagnitudeInfo[operator][allocation.strategy][operatorSetKey] = mInfo; - } - } - /// @dev gets the latest total magnitude or overwrites it if it is not set function _getLatestTotalMagnitude(address operator, IStrategy strategy) internal returns (uint64) { (bool exists,, uint224 totalMagnitude) = _totalMagnitudeUpdate[operator][strategy].latestSnapshot(); @@ -525,19 +501,19 @@ contract AllocationManager is * @param operator the operator to get the pending allocations for * @param strategy the strategy to get the pending allocations for * @param operatorSets the operatorSets to get the pending allocations for - * @return pendingMagnitude the pending allocations for each operatorSet + * @return pendingMagnitudes the pending allocations for each operatorSet * @return timestamps the timestamps for each pending allocation */ function getPendingAllocations( address operator, IStrategy strategy, OperatorSet[] calldata operatorSets - ) external view returns (uint64[] memory, uint32[] memory) { - uint64[] memory pendingMagnitudes = new uint64[](operatorSets.length); - uint32[] memory timestamps = new uint32[](operatorSets.length); + ) external view returns (uint64[] memory pendingMagnitudes, uint32[] memory timestamps) { + pendingMagnitudes = new uint64[](operatorSets.length); + timestamps = new uint32[](operatorSets.length); for (uint256 i = 0; i < operatorSets.length; ++i) { MagnitudeInfo memory opsetMagnitudeInfo = _operatorMagnitudeInfo[operator][strategy][_encodeOperatorSet(operatorSets[i])]; - + if (opsetMagnitudeInfo.effectTimestamp < block.timestamp && opsetMagnitudeInfo.pendingMagnitudeDiff > 0) { pendingMagnitudes[i] = opsetMagnitudeInfo.currentMagnitude + uint64(uint128(opsetMagnitudeInfo.pendingMagnitudeDiff)); timestamps[i] = opsetMagnitudeInfo.effectTimestamp; @@ -546,7 +522,6 @@ contract AllocationManager is timestamps[i] = 0; } } - return (pendingMagnitudes, timestamps); } /** @@ -563,9 +538,9 @@ contract AllocationManager is address operator, IStrategy strategy, OperatorSet[] calldata operatorSets - ) external view returns (uint64[] memory, uint32[] memory) { - uint64[] memory pendingMagnitudeDiffs = new uint64[](operatorSets.length); - uint32[] memory timestamps = new uint32[](operatorSets.length); + ) external view returns (uint64[] memory pendingMagnitudeDiffs, uint32[] memory timestamps) { + pendingMagnitudeDiffs = new uint64[](operatorSets.length); + timestamps = new uint32[](operatorSets.length); for (uint256 i = 0; i < operatorSets.length; ++i) { MagnitudeInfo memory opsetMagnitudeInfo = _operatorMagnitudeInfo[operator][strategy][_encodeOperatorSet(operatorSets[i])]; @@ -578,7 +553,6 @@ contract AllocationManager is timestamps[i] = 0; } } - return (pendingMagnitudeDiffs, timestamps); } /// @dev Verify operator's signature and spend salt diff --git a/src/contracts/core/AllocationManagerStorage.sol b/src/contracts/core/AllocationManagerStorage.sol index df3b24c9b..c6e34f2dc 100644 --- a/src/contracts/core/AllocationManagerStorage.sol +++ b/src/contracts/core/AllocationManagerStorage.sol @@ -78,5 +78,5 @@ abstract contract AllocationManagerStorage is IAllocationManager { * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ - uint256[42] private __gap; + uint256[43] private __gap; } diff --git a/src/contracts/interfaces/IAllocationManager.sol b/src/contracts/interfaces/IAllocationManager.sol index b4835b504..8d082d3c2 100644 --- a/src/contracts/interfaces/IAllocationManager.sol +++ b/src/contracts/interfaces/IAllocationManager.sol @@ -25,7 +25,7 @@ interface IAllocationManager is ISignatureUtils { /// @dev Thrown when an operator attempts to set their allocation for an operatorSet to the same value error SameMagnitude(); /// @dev Thrown when an allocation is attempted for a given operator when they have pending allocations or deallocations. - error PendingAllocationOrDeallocation(); + error ModificationAlreadyPending(); /// @dev Thrown when an allocation is attempted that exceeds a given operators total allocatable magnitude. error InsufficientAllocatableMagnitude(); /// @dev Thrown when attempting to use an expired eip-712 signature. diff --git a/src/test/events/IRewardsCoordinatorEvents.sol b/src/test/events/IRewardsCoordinatorEvents.sol index 30894a91b..94a24aefc 100644 --- a/src/test/events/IRewardsCoordinatorEvents.sol +++ b/src/test/events/IRewardsCoordinatorEvents.sol @@ -36,14 +36,14 @@ interface IRewardsCoordinatorEvents { ); event ActivationDelaySet(uint32 oldActivationDelay, uint32 newActivationDelay); event GlobalCommissionBipsSet(uint16 oldGlobalCommissionBips, uint16 newGlobalCommissionBips); - /// @notice emitted when an operator commission is set for a specific OperatorSet - event OperatorCommissionUpdated( - address indexed operator, - OperatorSet indexed operatorSet, - IRewardsCoordinator.RewardType rewardType, - uint16 newCommissionBips, - uint32 effectTimestamp - ); + // /// @notice emitted when an operator commission is set for a specific OperatorSet + // event OperatorCommissionUpdated( + // address indexed operator, + // OperatorSet indexed operatorSet, + // IRewardsCoordinator.RewardType rewardType, + // uint16 newCommissionBips, + // uint32 effectTimestamp + // ); event ClaimerForSet(address indexed earner, address indexed oldClaimer, address indexed claimer); /// @notice rootIndex is the specific array index of the newly created root in the storage array event DistributionRootSubmitted( @@ -62,13 +62,13 @@ interface IRewardsCoordinatorEvents { IERC20 token, uint256 claimedAmount ); - /// @notice emitted when an AVS creates a valid OperatorSetRewardsSubmission - event OperatorSetRewardCreated( - address indexed avs, - uint256 indexed submissionNonce, - bytes32 indexed rewardsSubmissionHash, - IRewardsCoordinator.OperatorSetRewardsSubmission rewardsSubmission - ); + // /// @notice emitted when an AVS creates a valid OperatorSetRewardsSubmission + // event OperatorSetRewardCreated( + // address indexed avs, + // uint256 indexed submissionNonce, + // bytes32 indexed rewardsSubmissionHash, + // IRewardsCoordinator.OperatorSetRewardsSubmission rewardsSubmission + // ); diff --git a/src/test/mocks/DelegationManagerMock.sol b/src/test/mocks/DelegationManagerMock.sol index 4af0c239b..483bf7b5f 100644 --- a/src/test/mocks/DelegationManagerMock.sol +++ b/src/test/mocks/DelegationManagerMock.sol @@ -179,7 +179,7 @@ contract DelegationManagerMock is IDelegationManager, Test { IStrategy strategy, OwnedShares shares ) external { - strategyManager.addShares(staker, token, strategy, shares); + strategyManager.addOwnedShares(staker, strategy, token, shares); } function removeShares( @@ -198,6 +198,6 @@ contract DelegationManagerMock is IDelegationManager, Test { OwnedShares shares, IERC20 token ) external { - strategyManager.withdrawSharesAsTokens(recipient, strategy, shares, token); + strategyManager.withdrawSharesAsTokens(recipient, strategy, token, shares); } } diff --git a/src/test/mocks/EigenPodManagerMock.sol b/src/test/mocks/EigenPodManagerMock.sol index 607186959..3641fd24f 100644 --- a/src/test/mocks/EigenPodManagerMock.sol +++ b/src/test/mocks/EigenPodManagerMock.sol @@ -19,9 +19,9 @@ contract EigenPodManagerMock is IEigenPodManager, Test, Pausable { function removeShares(address staker, IStrategy strategy, Shares shares) external {} - function addShares(address staker, IERC20 token, IStrategy strategy, OwnedShares shares) external {} + function addOwnedShares(address staker, IStrategy strategy, IERC20 token, OwnedShares shares) external {} - function withdrawSharesAsTokens(address recipient, IStrategy strategy, OwnedShares shares, IERC20 token) external {} + function withdrawSharesAsTokens(address recipient, IStrategy strategy, IERC20 token, OwnedShares shares) external {} function stakerStrategyShares(address user, IStrategy strategy) external view returns (Shares shares) {} diff --git a/src/test/mocks/StrategyManagerMock.sol b/src/test/mocks/StrategyManagerMock.sol index 0c34951df..01d1bf734 100644 --- a/src/test/mocks/StrategyManagerMock.sol +++ b/src/test/mocks/StrategyManagerMock.sol @@ -114,9 +114,9 @@ contract StrategyManagerMock is function removeShares(address staker, IStrategy strategy, Shares shares) external {} - function addShares(address staker, IERC20 token, IStrategy strategy, OwnedShares shares) external {} + function addOwnedShares(address staker, IStrategy strategy, IERC20 token, OwnedShares shares) external {} - function withdrawSharesAsTokens(address recipient, IStrategy strategy, OwnedShares shares, IERC20 token) external {} + function withdrawSharesAsTokens(address recipient, IStrategy strategy, IERC20 token, OwnedShares shares) external {} /// @notice returns the enshrined beaconChainETH Strategy function beaconChainETHStrategy() external view returns (IStrategy) {} diff --git a/src/test/unit/EigenPodManagerUnit.t.sol b/src/test/unit/EigenPodManagerUnit.t.sol index 8fe8c14c7..c64bc1583 100644 --- a/src/test/unit/EigenPodManagerUnit.t.sol +++ b/src/test/unit/EigenPodManagerUnit.t.sol @@ -194,28 +194,28 @@ contract EigenPodManagerUnitTests_ShareUpdateTests is EigenPodManagerUnitTests { cheats.assume(notDelegationManager != address(delegationManagerMock)); cheats.prank(notDelegationManager); cheats.expectRevert("EigenPodManager.onlyDelegationManager: not the DelegationManager"); - eigenPodManager.addShares(defaultStaker, 0); + eigenPodManager.addOwnedShares(defaultStaker, 0); } function test_addShares_revert_podOwnerInputAddressZero() public { cheats.prank(address(delegationManagerMock)); - cheats.expectRevert("EigenPodManager.addShares: podOwner cannot be zero address"); - eigenPodManager.addShares(address(0), 0); + cheats.expectRevert("EigenPodManager.addOwnedShares: podOwner cannot be zero address"); + eigenPodManager.addOwnedShares(address(0), 0); } function testFuzz_addShares_revert_sharesNegative(int256 shares) public { cheats.assume(shares < 0); cheats.prank(address(delegationManagerMock)); - cheats.expectRevert("EigenPodManager.addShares: shares cannot be negative"); - eigenPodManager.addShares(defaultStaker, uint256(shares)); + cheats.expectRevert("EigenPodManager.addOwnedShares: shares cannot be negative"); + eigenPodManager.addOwnedShares(defaultStaker, uint256(shares)); } function testFuzz_addShares_revert_sharesNotWholeGwei(uint256 shares) public { cheats.assume(int256(shares) >= 0); cheats.assume(shares % GWEI_TO_WEI != 0); cheats.prank(address(delegationManagerMock)); - cheats.expectRevert("EigenPodManager.addShares: shares must be a whole Gwei amount"); - eigenPodManager.addShares(defaultStaker, shares); + cheats.expectRevert("EigenPodManager.addOwnedShares: shares must be a whole Gwei amount"); + eigenPodManager.addOwnedShares(defaultStaker, shares); } function testFuzz_addShares(uint256 shares) public { @@ -226,7 +226,7 @@ contract EigenPodManagerUnitTests_ShareUpdateTests is EigenPodManagerUnitTests { // Add shares cheats.prank(address(delegationManagerMock)); - eigenPodManager.addShares(defaultStaker, shares); + eigenPodManager.addOwnedShares(defaultStaker, shares); // Check storage update assertEq(eigenPodManager.podOwnerShares(defaultStaker), int256(shares), "Incorrect number of shares added"); diff --git a/src/test/unit/StrategyManagerUnit.t.sol b/src/test/unit/StrategyManagerUnit.t.sol index 0ec6b8858..54e62ed76 100644 --- a/src/test/unit/StrategyManagerUnit.t.sol +++ b/src/test/unit/StrategyManagerUnit.t.sol @@ -979,18 +979,18 @@ contract StrategyManagerUnitTests_addShares is StrategyManagerUnitTests { function test_Revert_DelegationManagerModifier() external { DelegationManagerMock invalidDelegationManager = new DelegationManagerMock(); cheats.expectRevert("StrategyManager.onlyDelegationManager: not the DelegationManager"); - invalidDelegationManager.addShares(strategyManager, address(this), dummyToken, dummyStrat, 1); + invalidDelegationManager.addOwnedShares(strategyManager, address(this), dummyToken, dummyStrat, 1); } function testFuzz_Revert_StakerZeroAddress(uint256 amount) external { cheats.expectRevert("StrategyManager._addShares: staker cannot be zero address"); - delegationManagerMock.addShares(strategyManager, address(0), dummyToken, dummyStrat, amount); + delegationManagerMock.addOwnedShares(strategyManager, address(0), dummyToken, dummyStrat, amount); } function testFuzz_Revert_ZeroShares(address staker) external filterFuzzedAddressInputs(staker) { cheats.assume(staker != address(0)); cheats.expectRevert("StrategyManager._addShares: shares should not be zero!"); - delegationManagerMock.addShares(strategyManager, staker, dummyToken, dummyStrat, 0); + delegationManagerMock.addOwnedShares(strategyManager, staker, dummyToken, dummyStrat, 0); } function testFuzz_AppendsStakerStrategyList( @@ -1003,7 +1003,7 @@ contract StrategyManagerUnitTests_addShares is StrategyManagerUnitTests { assertEq(sharesBefore, 0, "Staker has already deposited into this strategy"); assertFalse(_isDepositedStrategy(staker, dummyStrat), "strategy should not be deposited"); - delegationManagerMock.addShares(strategyManager, staker, dummyToken, dummyStrat, amount); + delegationManagerMock.addOwnedShares(strategyManager, staker, dummyToken, dummyStrat, amount); uint256 stakerStrategyListLengthAfter = strategyManager.stakerStrategyListLength(staker); uint256 sharesAfter = strategyManager.stakerStrategyShares(staker, dummyStrat); assertEq( @@ -1028,7 +1028,7 @@ contract StrategyManagerUnitTests_addShares is StrategyManagerUnitTests { assertEq(sharesBefore, initialAmount, "Staker has not deposited amount into strategy"); assertTrue(_isDepositedStrategy(staker, strategy), "strategy should be deposited"); - delegationManagerMock.addShares(strategyManager, staker, dummyToken, dummyStrat, sharesAmount); + delegationManagerMock.addOwnedShares(strategyManager, staker, dummyToken, dummyStrat, sharesAmount); uint256 stakerStrategyListLengthAfter = strategyManager.stakerStrategyListLength(staker); uint256 sharesAfter = strategyManager.stakerStrategyShares(staker, dummyStrat); assertEq( @@ -1077,7 +1077,7 @@ contract StrategyManagerUnitTests_addShares is StrategyManagerUnitTests { cheats.prank(staker); cheats.expectRevert("StrategyManager._addShares: deposit would exceed MAX_STAKER_STRATEGY_LIST_LENGTH"); - delegationManagerMock.addShares(strategyManager, staker, dummyToken, strategy, amount); + delegationManagerMock.addOwnedShares(strategyManager, staker, dummyToken, strategy, amount); cheats.expectRevert("StrategyManager._addShares: deposit would exceed MAX_STAKER_STRATEGY_LIST_LENGTH"); strategyManager.depositIntoStrategy(strategy, token, amount);