Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Remove set withdrawal delay #355

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Feat: Remove set withdrawal delay #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83ba2cb
feat: remove setWithdrawalDelayBlocks()
8sunyuan Dec 1, 2023
9bbc4bb
fix: fixed delegation.initialize() calls
8sunyuan Dec 1, 2023
810b2a1
chore: emit WithdrawalDelayBlocksSet
8sunyuan Dec 4, 2023
a152b05
Merge branch 'm2-mainnet' into feat/remove-set-withdrawal-delay
8sunyuan Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion script/middleware/DeployOpenEigenLayer.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ contract DeployOpenEigenLayer is Script, Test {
eigenLayerProxyAdmin.upgradeAndCall(
TransparentUpgradeableProxy(payable(address(delegation))),
address(delegationImplementation),
abi.encodeWithSelector(DelegationManager.initialize.selector, executorMultisig, eigenLayerPauserReg, 0)
abi.encodeWithSelector(DelegationManager.initialize.selector, executorMultisig, eigenLayerPauserReg, 0, 0 /* withdrawalDelayBlocks */)
);
eigenLayerProxyAdmin.upgradeAndCall(
TransparentUpgradeableProxy(payable(address(strategyManager))),
Expand Down
2 changes: 1 addition & 1 deletion script/milestone/M2Deploy.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ contract M2Deploy is Script, Test {
);

cheats.expectRevert(bytes("Initializable: contract is already initialized"));
DelegationManager(address(delegation)).initialize(address(this), PauserRegistry(address(this)), 0);
DelegationManager(address(delegation)).initialize(address(this), PauserRegistry(address(this)), 0, 0);

cheats.expectRevert(bytes("Initializable: contract is already initialized"));
EigenPodManager(address(eigenPodManager)).initialize(
Expand Down
5 changes: 4 additions & 1 deletion script/testing/M2_Deploy_From_Scratch.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ contract Deployer_M2 is Script, Test {
uint256 STRATEGY_MANAGER_INIT_PAUSED_STATUS;
uint256 SLASHER_INIT_PAUSED_STATUS;
uint256 DELEGATION_INIT_PAUSED_STATUS;
uint256 DELEGATION_WITHDRAWAL_DELAY_BLOCKS;
uint256 EIGENPOD_MANAGER_INIT_PAUSED_STATUS;
uint256 EIGENPOD_MANAGER_MAX_PODS;
uint256 DELAYED_WITHDRAWAL_ROUTER_INIT_PAUSED_STATUS;
Expand All @@ -103,6 +104,7 @@ contract Deployer_M2 is Script, Test {
STRATEGY_MANAGER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".strategyManager.init_paused_status");
SLASHER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".slasher.init_paused_status");
DELEGATION_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".delegation.init_paused_status");
DELEGATION_WITHDRAWAL_DELAY_BLOCKS = stdJson.readUint(config_data, ".delegation.init_withdrawal_delay_blocks");
EIGENPOD_MANAGER_MAX_PODS = stdJson.readUint(config_data, ".eigenPodManager.max_pods");
EIGENPOD_MANAGER_INIT_PAUSED_STATUS = stdJson.readUint(config_data, ".eigenPodManager.init_paused_status");
DELAYED_WITHDRAWAL_ROUTER_INIT_PAUSED_STATUS = stdJson.readUint(
Expand Down Expand Up @@ -208,7 +210,8 @@ contract Deployer_M2 is Script, Test {
DelegationManager.initialize.selector,
executorMultisig,
eigenLayerPauserReg,
DELEGATION_INIT_PAUSED_STATUS
DELEGATION_INIT_PAUSED_STATUS,
DELEGATION_WITHDRAWAL_DELAY_BLOCKS
)
);
eigenLayerProxyAdmin.upgradeAndCall(
Expand Down
3 changes: 2 additions & 1 deletion script/testing/M2_deploy_from_scratch.mainnet.config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@
},
"delegation":
{
"init_paused_status": 0
"init_paused_status": 0,
"init_withdrawal_delay_blocks": 50400
},
"ethPOSDepositAddress": "0x00000000219ab540356cBB839Cbe05303d7705Fa"
}
26 changes: 12 additions & 14 deletions src/contracts/core/DelegationManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,18 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg

/**
* @dev Initializes the addresses of the initial owner, pauser registry, and paused status.
* withdrawalDelayBlocks is set only once here
*/
function initialize(
address initialOwner,
IPauserRegistry _pauserRegistry,
uint256 initialPausedStatus
uint256 initialPausedStatus,
uint256 _withdrawalDelayBlocks
) external initializer {
_initializePauser(_pauserRegistry, initialPausedStatus);
_DOMAIN_SEPARATOR = _calculateDomainSeparator();
_transferOwnership(initialOwner);
_initializeWithdrawalDelayBlocks(_withdrawalDelayBlocks);
}

/*******************************************************************************
Expand Down Expand Up @@ -210,19 +213,6 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg
_delegate(staker, operator, approverSignatureAndExpiry, approverSalt);
}

/**
* @notice Owner-only function for modifying the value of the `withdrawalDelayBlocks` variable.
* @param newWithdrawalDelayBlocks new value of `withdrawalDelayBlocks`.
*/
function setWithdrawalDelayBlocks(uint256 newWithdrawalDelayBlocks) external onlyOwner {
require(
newWithdrawalDelayBlocks <= MAX_WITHDRAWAL_DELAY_BLOCKS,
"DelegationManager.setWithdrawalDelayBlocks: newWithdrawalDelayBlocks too high"
);
emit WithdrawalDelayBlocksSet(withdrawalDelayBlocks, newWithdrawalDelayBlocks);
withdrawalDelayBlocks = newWithdrawalDelayBlocks;
}

/**
* Allows the staker, the staker's operator, or that operator's delegationApprover to undelegate
* a staker from their operator. Undelegation immediately removes ALL active shares/strategies from
Expand Down Expand Up @@ -763,6 +753,14 @@ contract DelegationManager is Initializable, OwnableUpgradeable, Pausable, Deleg
}
}

function _initializeWithdrawalDelayBlocks(uint256 _withdrawalDelayBlocks) internal {
require(
_withdrawalDelayBlocks <= MAX_WITHDRAWAL_DELAY_BLOCKS,
"DelegationManager._initializeWithdrawalDelayBlocks: _withdrawalDelayBlocks cannot be > MAX_WITHDRAWAL_DELAY_BLOCKS"
);
withdrawalDelayBlocks = _withdrawalDelayBlocks;
}
8sunyuan marked this conversation as resolved.
Show resolved Hide resolved

/*******************************************************************************
VIEW FUNCTIONS
*******************************************************************************/
Expand Down
4 changes: 2 additions & 2 deletions src/test/Delegation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ contract DelegationTests is EigenLayerTestHelper {
/// cannot be intitialized multiple times
function testCannotInitMultipleTimesDelegation() public cannotReinit {
//delegation has already been initialized in the Deployer test contract
delegation.initialize(address(this), eigenLayerPauserReg, 0);
delegation.initialize(address(this), eigenLayerPauserReg, 0, initializedWithdrawalDelayBlocks);
}

/// @notice This function tests to ensure that a you can't register as a delegate multiple times
Expand Down Expand Up @@ -370,7 +370,7 @@ contract DelegationTests is EigenLayerTestHelper {
//delegation has already been initialized in the Deployer test contract
vm.prank(_attacker);
cheats.expectRevert(bytes("Initializable: contract is already initialized"));
delegation.initialize(_attacker, eigenLayerPauserReg, 0);
delegation.initialize(_attacker, eigenLayerPauserReg, 0, initializedWithdrawalDelayBlocks);
}

/// @notice This function tests that the earningsReceiver cannot be set to address(0)
Expand Down
3 changes: 2 additions & 1 deletion src/test/DepositWithdraw.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,8 @@ contract DepositWithdrawTests is EigenLayerTestHelper {
DelegationManager.initialize.selector,
eigenLayerReputedMultisig,
eigenLayerPauserReg,
0/*initialPausedStatus*/
0 /*initialPausedStatus*/,
initializedWithdrawalDelayBlocks
)
);
eigenLayerProxyAdmin.upgradeAndCall(
Expand Down
4 changes: 3 additions & 1 deletion src/test/EigenLayerDeployer.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ contract EigenLayerDeployer is Operators {
uint256 public constant eigenTotalSupply = 1000e18;
uint256 nonce = 69;
uint256 public gasLimit = 750000;
uint256 initializedWithdrawalDelayBlocks = 0;
uint32 PARTIAL_WITHDRAWAL_FRAUD_PROOF_PERIOD_BLOCKS = 7 days / 12 seconds;
uint256 REQUIRED_BALANCE_WEI = 32 ether;
uint64 MAX_PARTIAL_WTIHDRAWAL_AMOUNT_GWEI = 1 ether / 1e9;
Expand Down Expand Up @@ -268,7 +269,8 @@ contract EigenLayerDeployer is Operators {
DelegationManager.initialize.selector,
eigenLayerReputedMultisig,
eigenLayerPauserReg,
0 /*initialPausedStatus*/
0 /*initialPausedStatus*/,
initializedWithdrawalDelayBlocks
)
);
eigenLayerProxyAdmin.upgradeAndCall(
Expand Down
3 changes: 2 additions & 1 deletion src/test/EigenPod.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ contract EigenPodTests is ProofParsing, EigenPodPausingConstants {
DelegationManager.initialize.selector,
initialOwner,
pauserReg,
0 /*initialPausedStatus*/
0 /*initialPausedStatus*/,
WITHDRAWAL_DELAY_BLOCKS
)
);
eigenLayerProxyAdmin.upgradeAndCall(
Expand Down
5 changes: 3 additions & 2 deletions src/test/integration/IntegrationDeployer.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
DelayedWithdrawalRouter delayedWithdrawalRouterImplementation = new DelayedWithdrawalRouter(eigenPodManager);

// Third, upgrade the proxy contracts to point to the implementations
uint256 withdrawalDelayBlocks = 7 days / 12 seconds;
// DelegationManager
eigenLayerProxyAdmin.upgradeAndCall(
TransparentUpgradeableProxy(payable(address(delegationManager))),
Expand All @@ -197,7 +198,8 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
DelegationManager.initialize.selector,
eigenLayerReputedMultisig, // initialOwner
pauserRegistry,
0 // initialPausedStatus
0 /* initialPausedStatus */,
withdrawalDelayBlocks
)
);
// StrategyManager
Expand Down Expand Up @@ -237,7 +239,6 @@ abstract contract IntegrationDeployer is Test, IUserDeployer {
)
);
// Delayed Withdrawal Router
uint256 withdrawalDelayBlocks = 7 days / 12 seconds;
eigenLayerProxyAdmin.upgradeAndCall(
TransparentUpgradeableProxy(payable(address(delayedWithdrawalRouter))),
address(delayedWithdrawalRouterImplementation),
Expand Down
75 changes: 35 additions & 40 deletions src/test/unit/DelegationUnit.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
address defaultStaker = cheats.addr(uint256(123_456_789));
address defaultOperator = address(this);

uint256 initializedWithdrawalDelayBlocks = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this test or any others set this to a nonzero value?
feels harder to test that the functionality is correct if we are setting this to zero, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as 0 since we never changed it from its default 0 value in the unit tests.
Updated now to a nonzero value but tests still seem to pass because we never added back the unit tests for the withdrawals. Will add them back to DelegationUnit.t.sol in a followup PR


IStrategy public constant beaconChainETHStrategy = IStrategy(0xbeaC0eeEeeeeEEeEeEEEEeeEEeEeeeEeeEEBEaC0);

// Index for flag that pauses new delegations when set.
Expand All @@ -54,6 +56,8 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
// Index for flag that pauses completing existing withdrawals when set.
uint8 internal constant PAUSED_EXIT_WITHDRAWAL_QUEUE = 2;

uint256 public constant MAX_WITHDRAWAL_DELAY_BLOCKS = 50400;

/// @notice mappings used to handle duplicate entries in fuzzed address array input
mapping(address => uint256) public totalSharesForStrategyInArray;
mapping(IStrategy => uint256) public delegatedSharesBefore;
Expand All @@ -69,7 +73,13 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
new TransparentUpgradeableProxy(
address(delegationManagerImplementation),
address(eigenLayerProxyAdmin),
abi.encodeWithSelector(DelegationManager.initialize.selector, address(this), pauserRegistry, 0) // 0 is initialPausedStatus
abi.encodeWithSelector(
DelegationManager.initialize.selector,
address(this),
pauserRegistry,
0, // 0 is initialPausedStatus
initializedWithdrawalDelayBlocks
)
)
)
);
Expand Down Expand Up @@ -240,11 +250,11 @@ contract DelegationManagerUnitTests is EigenLayerUnitTestSetup, IDelegationManag
ERC1271WalletMock wallet = new ERC1271WalletMock(delegationSigner);

IDelegationManager.OperatorDetails memory operatorDetails = IDelegationManager.OperatorDetails({
earningsReceiver: defaultOperator,
earningsReceiver: operator,
delegationApprover: address(wallet),
stakerOptOutWindowBlocks: 0
});
_registerOperator(defaultOperator, operatorDetails, emptyStringForMetadataURI);
_registerOperator(operator, operatorDetails, emptyStringForMetadataURI);
}

function _registerOperator(
Expand Down Expand Up @@ -294,7 +304,7 @@ contract DelegationManagerUnitTests_Initialization_Setters is DelegationManagerU
/// @notice Verifies that the DelegationManager cannot be iniitalized multiple times
function test_initialize_revert_reinitialization() public {
cheats.expectRevert("Initializable: contract is already initialized");
delegationManager.initialize(address(this), pauserRegistry, 0);
delegationManager.initialize(address(this), pauserRegistry, 0, initializedWithdrawalDelayBlocks);
}

/// @notice Verifies that the stakeRegistry cannot be set after it has already been set
Expand All @@ -303,37 +313,27 @@ contract DelegationManagerUnitTests_Initialization_Setters is DelegationManagerU
delegationManager.setStakeRegistry(stakeRegistryMock);
}

function testFuzz_setWithdrawalDelayBlocks_revert_notOwner(
address invalidCaller
) public filterFuzzedAddressInputs(invalidCaller) {
cheats.prank(invalidCaller);
cheats.expectRevert("Ownable: caller is not the owner");
delegationManager.setWithdrawalDelayBlocks(0);
}

function testFuzz_setWithdrawalDelayBlocks_revert_tooLarge(uint256 newWithdrawalDelayBlocks) external {
// filter fuzzed inputs to disallowed amounts
cheats.assume(newWithdrawalDelayBlocks > delegationManager.MAX_WITHDRAWAL_DELAY_BLOCKS());

// attempt to set the `withdrawalDelayBlocks` variable
cheats.expectRevert("DelegationManager.setWithdrawalDelayBlocks: newWithdrawalDelayBlocks too high");
delegationManager.setWithdrawalDelayBlocks(newWithdrawalDelayBlocks);
}

function testFuzz_setWithdrawalDelayBlocks(uint256 newWithdrawalDelayBlocks) public {
cheats.assume(newWithdrawalDelayBlocks <= delegationManager.MAX_WITHDRAWAL_DELAY_BLOCKS());

// set the `withdrawalDelayBlocks` variable
uint256 previousDelayBlocks = delegationManager.withdrawalDelayBlocks();
cheats.expectEmit(true, true, true, true, address(delegationManager));
emit WithdrawalDelayBlocksSet(previousDelayBlocks, newWithdrawalDelayBlocks);
delegationManager.setWithdrawalDelayBlocks(newWithdrawalDelayBlocks);

// Check storage
assertEq(
delegationManager.withdrawalDelayBlocks(),
newWithdrawalDelayBlocks,
"withdrawalDelayBlocks not set correctly"
function testFuzz_initialize_Revert_WhenWithdrawalDelayBlocksTooLarge(uint256 withdrawalDelayBlocks) public {
cheats.assume(withdrawalDelayBlocks > MAX_WITHDRAWAL_DELAY_BLOCKS);
// Deploy DelegationManager implmentation and proxy
delegationManagerImplementation = new DelegationManager(strategyManagerMock, slasherMock, eigenPodManagerMock);
cheats.expectRevert(
"DelegationManager._initializeWithdrawalDelayBlocks: _withdrawalDelayBlocks cannot be > MAX_WITHDRAWAL_DELAY_BLOCKS"
);
delegationManager = DelegationManager(
address(
new TransparentUpgradeableProxy(
address(delegationManagerImplementation),
address(eigenLayerProxyAdmin),
abi.encodeWithSelector(
DelegationManager.initialize.selector,
address(this),
pauserRegistry,
0, // 0 is initialPausedStatus
withdrawalDelayBlocks
)
)
)
);
}
}
Expand Down Expand Up @@ -894,9 +894,6 @@ contract DelegationManagerUnitTests_delegateTo is DelegationManagerUnitTests {
cheats.roll(type(uint256).max / 2);
// filter to only *invalid* `expiry` values
cheats.assume(expiry < block.timestamp);

address delegationApprover = cheats.addr(delegationSignerPrivateKey);

// filter inputs, since this will fail when the staker is already registered as an operator
cheats.assume(staker != defaultOperator);

Expand Down Expand Up @@ -1098,8 +1095,6 @@ contract DelegationManagerUnitTests_delegateTo is DelegationManagerUnitTests {
) public filterFuzzedAddressInputs(staker) {
// filter to only valid `expiry` values
cheats.assume(expiry >= block.timestamp);

address delegationApprover = cheats.addr(delegationSignerPrivateKey);
// filter inputs, since this will fail when the staker is already registered as an operator
cheats.assume(staker != defaultOperator);

Expand Down
Loading