From 444954129ee254c18bf65133c667fcfb8e92b584 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Fri, 6 Dec 2024 12:19:43 +0300 Subject: [PATCH 1/5] Add maximum assets lock duration to Escrow contract --- contracts/Escrow.sol | 11 ++++++++--- contracts/interfaces/IEscrow.sol | 2 ++ contracts/libraries/EscrowState.sol | 10 ++++++++-- scripts/deploy/Config.sol | 2 ++ scripts/deploy/ContractsDeployment.sol | 3 ++- scripts/deploy/JsonConfig.s.sol | 4 ++++ test/mocks/EscrowMock.sol | 9 +++++++++ test/unit/DualGovernance.t.sol | 3 ++- test/unit/Escrow.t.sol | 10 ++++++++-- test/unit/libraries/EscrowState.t.sol | 24 ++++++++++++++++++++---- 10 files changed, 65 insertions(+), 13 deletions(-) diff --git a/contracts/Escrow.sol b/contracts/Escrow.sol index 87c61c97..1c5f9081 100644 --- a/contracts/Escrow.sol +++ b/contracts/Escrow.sol @@ -3,7 +3,7 @@ pragma solidity 0.8.26; import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; -import {Duration} from "./types/Duration.sol"; +import {Durations, Duration} from "./types/Duration.sol"; import {Timestamp} from "./types/Timestamp.sol"; import {ETHValue, ETHValues} from "./types/ETHValue.sol"; import {SharesValue, SharesValues} from "./types/SharesValue.sol"; @@ -58,6 +58,9 @@ contract Escrow is IEscrow { /// the `Escrow.requestNextWithdrawalsBatch(batchSize)` method. uint256 public immutable MIN_WITHDRAWALS_BATCH_SIZE; + /// @notice The maximum duration that can be set as the minimum assets lock duration. + Duration public immutable MAX_ASSETS_LOCK_DURATION; + // --- // Dependencies Immutables // --- @@ -104,7 +107,8 @@ contract Escrow is IEscrow { IWstETH wstETH, IWithdrawalQueue withdrawalQueue, IDualGovernance dualGovernance, - uint256 minWithdrawalsBatchSize + uint256 minWithdrawalsBatchSize, + Duration maxAssetsLockDuration ) { _SELF = address(this); DUAL_GOVERNANCE = dualGovernance; @@ -114,6 +118,7 @@ contract Escrow is IEscrow { WITHDRAWAL_QUEUE = withdrawalQueue; MIN_WITHDRAWALS_BATCH_SIZE = minWithdrawalsBatchSize; + MAX_ASSETS_LOCK_DURATION = maxAssetsLockDuration; } /// @notice Initializes the proxy instance with the specified minimum assets lock duration. @@ -432,7 +437,7 @@ contract Escrow is IEscrow { /// @param newMinAssetsLockDuration The new minimum lock duration to be set. function setMinAssetsLockDuration(Duration newMinAssetsLockDuration) external { _checkCallerIsDualGovernance(); - _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration); + _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration, MAX_ASSETS_LOCK_DURATION); } // --- diff --git a/contracts/interfaces/IEscrow.sol b/contracts/interfaces/IEscrow.sol index a51a5030..a0e8973c 100644 --- a/contracts/interfaces/IEscrow.sol +++ b/contracts/interfaces/IEscrow.sol @@ -32,6 +32,8 @@ interface IEscrow { function initialize(Duration minAssetsLockDuration) external; + function MAX_ASSETS_LOCK_DURATION() external view returns (Duration); + function lockStETH(uint256 amount) external returns (uint256 lockedStETHShares); function unlockStETH() external returns (uint256 unlockedStETHShares); function lockWstETH(uint256 amount) external returns (uint256 lockedStETHShares); diff --git a/contracts/libraries/EscrowState.sol b/contracts/libraries/EscrowState.sol index 9fb30ff3..dbd37c5b 100644 --- a/contracts/libraries/EscrowState.sol +++ b/contracts/libraries/EscrowState.sol @@ -103,8 +103,14 @@ library EscrowState { /// @notice Sets the minimum assets lock duration. /// @param self The context of the Escrow State library. /// @param newMinAssetsLockDuration The new minimum assets lock duration. - function setMinAssetsLockDuration(Context storage self, Duration newMinAssetsLockDuration) internal { - if (self.minAssetsLockDuration == newMinAssetsLockDuration) { + /// @param maxAssetsLockDuration Sanity check for max assets lock duration. + function setMinAssetsLockDuration( + Context storage self, + Duration newMinAssetsLockDuration, + Duration maxAssetsLockDuration + ) internal { + if (self.minAssetsLockDuration == newMinAssetsLockDuration || newMinAssetsLockDuration > maxAssetsLockDuration) + { revert InvalidMinAssetsLockDuration(newMinAssetsLockDuration); } _setMinAssetsLockDuration(self, newMinAssetsLockDuration); diff --git a/scripts/deploy/Config.sol b/scripts/deploy/Config.sol index 4b46f375..bb438f90 100644 --- a/scripts/deploy/Config.sol +++ b/scripts/deploy/Config.sol @@ -31,6 +31,7 @@ uint256 constant DEFAULT_MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT = 255; uint256 constant DEFAULT_FIRST_SEAL_RAGE_QUIT_SUPPORT = 3_00; // 3% uint256 constant DEFAULT_SECOND_SEAL_RAGE_QUIT_SUPPORT = 15_00; // 15% uint256 constant DEFAULT_MIN_ASSETS_LOCK_DURATION = 5 hours; +uint256 constant DEFAULT_MAX_ASSETS_LOCK_DURATION = 365 days; uint256 constant DEFAULT_VETO_SIGNALLING_MIN_DURATION = 3 days; uint256 constant DEFAULT_VETO_SIGNALLING_MAX_DURATION = 30 days; uint256 constant DEFAULT_VETO_SIGNALLING_MIN_ACTIVE_DURATION = 5 hours; @@ -80,6 +81,7 @@ struct DeployConfig { PercentD16 FIRST_SEAL_RAGE_QUIT_SUPPORT; PercentD16 SECOND_SEAL_RAGE_QUIT_SUPPORT; Duration MIN_ASSETS_LOCK_DURATION; + Duration MAX_ASSETS_LOCK_DURATION; Duration VETO_SIGNALLING_MIN_DURATION; Duration VETO_SIGNALLING_MAX_DURATION; Duration VETO_SIGNALLING_MIN_ACTIVE_DURATION; diff --git a/scripts/deploy/ContractsDeployment.sol b/scripts/deploy/ContractsDeployment.sol index dcd84c3e..f5a1b33d 100644 --- a/scripts/deploy/ContractsDeployment.sol +++ b/scripts/deploy/ContractsDeployment.sol @@ -228,7 +228,8 @@ library DGContractsDeployment { minWithdrawalsBatchSize: dgDeployConfig.MIN_WITHDRAWALS_BATCH_SIZE, minTiebreakerActivationTimeout: dgDeployConfig.MIN_TIEBREAKER_ACTIVATION_TIMEOUT, maxTiebreakerActivationTimeout: dgDeployConfig.MAX_TIEBREAKER_ACTIVATION_TIMEOUT, - maxSealableWithdrawalBlockersCount: dgDeployConfig.MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT + maxSealableWithdrawalBlockersCount: dgDeployConfig.MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT, + maxAssetsLockDuration: dgDeployConfig.MAX_ASSETS_LOCK_DURATION }) }); } diff --git a/scripts/deploy/JsonConfig.s.sol b/scripts/deploy/JsonConfig.s.sol index 5922de0d..f4227b21 100644 --- a/scripts/deploy/JsonConfig.s.sol +++ b/scripts/deploy/JsonConfig.s.sol @@ -46,6 +46,7 @@ import { DEFAULT_FIRST_SEAL_RAGE_QUIT_SUPPORT, DEFAULT_SECOND_SEAL_RAGE_QUIT_SUPPORT, DEFAULT_MIN_ASSETS_LOCK_DURATION, + DEFAULT_MAX_ASSETS_LOCK_DURATION, DEFAULT_VETO_SIGNALLING_MIN_DURATION, DEFAULT_VETO_SIGNALLING_MAX_DURATION, DEFAULT_VETO_SIGNALLING_MIN_ACTIVE_DURATION, @@ -173,6 +174,9 @@ contract DGDeployJSONConfigProvider is Script { MIN_ASSETS_LOCK_DURATION: Durations.from( stdJson.readUintOr(jsonConfig, ".MIN_ASSETS_LOCK_DURATION", DEFAULT_MIN_ASSETS_LOCK_DURATION) ), + MAX_ASSETS_LOCK_DURATION: Durations.from( + stdJson.readUintOr(jsonConfig, ".MAX_ASSETS_LOCK_DURATION", DEFAULT_MAX_ASSETS_LOCK_DURATION) + ), VETO_SIGNALLING_MIN_DURATION: Durations.from( stdJson.readUintOr(jsonConfig, ".VETO_SIGNALLING_MIN_DURATION", DEFAULT_VETO_SIGNALLING_MIN_DURATION) ), diff --git a/test/mocks/EscrowMock.sol b/test/mocks/EscrowMock.sol index d87c5965..ed58f4a1 100644 --- a/test/mocks/EscrowMock.sol +++ b/test/mocks/EscrowMock.sol @@ -12,6 +12,7 @@ contract EscrowMock is IEscrow { event __RageQuitStarted(Duration rageQuitExtraTimelock, Duration rageQuitWithdrawalsTimelock); Duration public __minAssetsLockDuration; + Duration public __maxAssetsLockDuration; PercentD16 public __rageQuitSupport; bool public __isRageQuitFinalized; @@ -130,4 +131,12 @@ contract EscrowMock is IEscrow { function getMinAssetsLockDuration() external view returns (Duration minAssetsLockDuration) { return __minAssetsLockDuration; } + + function MAX_ASSETS_LOCK_DURATION() external view returns (Duration) { + return __maxAssetsLockDuration; + } + + function setMaxAssetsLockDuration(Duration newMaxAssetsLockDuration) external { + __maxAssetsLockDuration = newMaxAssetsLockDuration; + } } diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index add00fc9..3e8fe3ba 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -84,7 +84,8 @@ contract DualGovernanceUnitTests is UnitTest { minWithdrawalsBatchSize: 4, minTiebreakerActivationTimeout: Durations.from(30 days), maxTiebreakerActivationTimeout: Durations.from(180 days), - maxSealableWithdrawalBlockersCount: 128 + maxSealableWithdrawalBlockersCount: 128, + maxAssetsLockDuration: Durations.from(365 days) }); DualGovernance internal _dualGovernance = diff --git a/test/unit/Escrow.t.sol b/test/unit/Escrow.t.sol index 0f1a17ce..a697b1a8 100644 --- a/test/unit/Escrow.t.sol +++ b/test/unit/Escrow.t.sol @@ -44,8 +44,14 @@ contract EscrowUnitTests is UnitTest { _wstETH = IWstETH(address(new ERC20Mock())); _withdrawalQueue = new WithdrawalQueueMock(_stETH); _withdrawalQueue.setMaxStETHWithdrawalAmount(1_000 ether); - _masterCopy = - new Escrow(_stETH, _wstETH, WithdrawalQueueMock(_withdrawalQueue), IDualGovernance(_dualGovernance), 100); + _masterCopy = new Escrow( + _stETH, + _wstETH, + WithdrawalQueueMock(_withdrawalQueue), + IDualGovernance(_dualGovernance), + 100, + Durations.from(1000) + ); _escrow = Escrow(payable(Clones.clone(address(_masterCopy)))); vm.prank(address(_escrow)); diff --git a/test/unit/libraries/EscrowState.t.sol b/test/unit/libraries/EscrowState.t.sol index c7685468..88ff03f0 100644 --- a/test/unit/libraries/EscrowState.t.sol +++ b/test/unit/libraries/EscrowState.t.sol @@ -103,13 +103,17 @@ contract EscrowStateUnitTests is UnitTest { // setMinAssetsLockDuration() // --- - function test_setMinAssetsLockDuration_happyPath(Duration minAssetsLockDuration) external { + function testFuzz_setMinAssetsLockDuration_happyPath( + Duration minAssetsLockDuration, + Duration maxAssetsLockDuration + ) external { vm.assume(minAssetsLockDuration != Durations.ZERO); + vm.assume(minAssetsLockDuration <= maxAssetsLockDuration); vm.expectEmit(); emit EscrowState.MinAssetsLockDurationSet(minAssetsLockDuration); - EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxAssetsLockDuration); checkContext({ state: State.NotInitialized, @@ -120,13 +124,25 @@ contract EscrowStateUnitTests is UnitTest { }); } - function test_setMinAssetsLockDuration_RevertWhen_DurationNotChanged(Duration minAssetsLockDuration) external { + function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationNotChanged(Duration minAssetsLockDuration) external { _context.minAssetsLockDuration = minAssetsLockDuration; vm.expectRevert( abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, minAssetsLockDuration) ); - EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, Durations.from(type(uint16).max)); + } + + function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationGreaterThenMaxAssetsLockDuration( + Duration minAssetsLockDuration, + Duration maxAssetsLockDuration + ) external { + vm.assume(minAssetsLockDuration > maxAssetsLockDuration); + + vm.expectRevert( + abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, minAssetsLockDuration) + ); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxAssetsLockDuration); } // --- From b41866634ba195cd6db4cfb508f4554162a438cc Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Fri, 6 Dec 2024 12:20:52 +0300 Subject: [PATCH 2/5] DualGovernance dependencies split --- contracts/DualGovernance.sol | 37 +++++++++++++------- scripts/deploy/ContractsDeployment.sol | 10 +++--- test/unit/DualGovernance.t.sol | 47 +++++++++++++++++++------- 3 files changed, 65 insertions(+), 29 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index 19d4426d..4b78cf17 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -69,11 +69,13 @@ contract DualGovernance is IDualGovernance { /// @param maxSealableWithdrawalBlockersCount The upper bound for the number of sealable withdrawal blockers allowed to be /// registered in the Dual Governance. This parameter prevents filling the sealable withdrawal blockers /// with so many items that tiebreaker calls would revert due to out-of-gas errors. + /// @param maxAssetsLockDuration The maximum duration for which assets can be locked in the Rage Quit Escrow contract. struct SanityCheckParams { uint256 minWithdrawalsBatchSize; Duration minTiebreakerActivationTimeout; Duration maxTiebreakerActivationTimeout; uint256 maxSealableWithdrawalBlockersCount; + Duration maxAssetsLockDuration; } /// @notice The lower bound for the time the Dual Governance must spend in the "locked" state @@ -93,17 +95,21 @@ contract DualGovernance is IDualGovernance { // External Dependencies // --- - /// @notice The external dependencies of the Dual Governance system. + /// @notice Token addresses tha used in the Dual Governance as signalling tokens. /// @param stETH The address of the stETH token. /// @param wstETH The address of the wstETH token. /// @param withdrawalQueue The address of Lido's Withdrawal Queue and the unstETH token. - /// @param timelock The address of the Timelock contract. - /// @param resealManager The address of the Reseal Manager. - /// @param configProvider The address of the Dual Governance Config Provider. - struct ExternalDependencies { + struct SignallingTokens { IStETH stETH; IWstETH wstETH; IWithdrawalQueue withdrawalQueue; + } + + /// @notice Dependencies required by the Dual Governance contract. + /// @param timelock The address of the Timelock contract. + /// @param resealManager The address of the Reseal Manager. + /// @param configProvider The address of the Dual Governance Config Provider. + struct DualGovernanceComponents { ITimelock timelock; IResealManager resealManager; IDualGovernanceConfigProvider configProvider; @@ -140,12 +146,16 @@ contract DualGovernance is IDualGovernance { // Constructor // --- - constructor(ExternalDependencies memory dependencies, SanityCheckParams memory sanityCheckParams) { + constructor( + DualGovernanceComponents memory components, + SignallingTokens memory signallingTokens, + SanityCheckParams memory sanityCheckParams + ) { if (sanityCheckParams.minTiebreakerActivationTimeout > sanityCheckParams.maxTiebreakerActivationTimeout) { revert InvalidTiebreakerActivationTimeoutBounds(); } - TIMELOCK = dependencies.timelock; + TIMELOCK = components.timelock; MIN_TIEBREAKER_ACTIVATION_TIMEOUT = sanityCheckParams.minTiebreakerActivationTimeout; MAX_TIEBREAKER_ACTIVATION_TIMEOUT = sanityCheckParams.maxTiebreakerActivationTimeout; @@ -153,15 +163,16 @@ contract DualGovernance is IDualGovernance { ESCROW_MASTER_COPY = new Escrow({ dualGovernance: this, - stETH: dependencies.stETH, - wstETH: dependencies.wstETH, - withdrawalQueue: dependencies.withdrawalQueue, - minWithdrawalsBatchSize: sanityCheckParams.minWithdrawalsBatchSize + stETH: signallingTokens.stETH, + wstETH: signallingTokens.wstETH, + withdrawalQueue: signallingTokens.withdrawalQueue, + minWithdrawalsBatchSize: sanityCheckParams.minWithdrawalsBatchSize, + maxAssetsLockDuration: sanityCheckParams.maxAssetsLockDuration }); emit EscrowMasterCopyDeployed(ESCROW_MASTER_COPY); - _stateMachine.initialize(dependencies.configProvider, ESCROW_MASTER_COPY); - _resealer.setResealManager(address(dependencies.resealManager)); + _stateMachine.initialize(components.configProvider, ESCROW_MASTER_COPY); + _resealer.setResealManager(address(components.resealManager)); } // --- diff --git a/scripts/deploy/ContractsDeployment.sol b/scripts/deploy/ContractsDeployment.sol index f5a1b33d..af87be0e 100644 --- a/scripts/deploy/ContractsDeployment.sol +++ b/scripts/deploy/ContractsDeployment.sol @@ -216,14 +216,16 @@ library DGContractsDeployment { LidoContracts memory lidoAddresses ) internal returns (DualGovernance) { return new DualGovernance({ - dependencies: DualGovernance.ExternalDependencies({ - stETH: lidoAddresses.stETH, - wstETH: lidoAddresses.wstETH, - withdrawalQueue: lidoAddresses.withdrawalQueue, + components: DualGovernance.DualGovernanceComponents({ timelock: timelock, resealManager: resealManager, configProvider: configProvider }), + signallingTokens: DualGovernance.SignallingTokens({ + stETH: lidoAddresses.stETH, + wstETH: lidoAddresses.wstETH, + withdrawalQueue: lidoAddresses.withdrawalQueue + }), sanityCheckParams: DualGovernance.SanityCheckParams({ minWithdrawalsBatchSize: dgDeployConfig.MIN_WITHDRAWALS_BATCH_SIZE, minTiebreakerActivationTimeout: dgDeployConfig.MIN_TIEBREAKER_ACTIVATION_TIMEOUT, diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index 3e8fe3ba..e678a87a 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -71,10 +71,13 @@ contract DualGovernanceUnitTests is UnitTest { }) ); - DualGovernance.ExternalDependencies internal _externalDependencies = DualGovernance.ExternalDependencies({ + DualGovernance.SignallingTokens internal _signallingTokens = DualGovernance.SignallingTokens({ stETH: _STETH_MOCK, wstETH: _WSTETH_STUB, - withdrawalQueue: _WITHDRAWAL_QUEUE_MOCK, + withdrawalQueue: _WITHDRAWAL_QUEUE_MOCK + }); + + DualGovernance.DualGovernanceComponents internal _dgComponents = DualGovernance.DualGovernanceComponents({ timelock: _timelock, resealManager: _RESEAL_MANAGER_STUB, configProvider: _configProvider @@ -88,8 +91,11 @@ contract DualGovernanceUnitTests is UnitTest { maxAssetsLockDuration: Durations.from(365 days) }); - DualGovernance internal _dualGovernance = - new DualGovernance({dependencies: _externalDependencies, sanityCheckParams: _sanityCheckParams}); + DualGovernance internal _dualGovernance = new DualGovernance({ + components: _dgComponents, + signallingTokens: _signallingTokens, + sanityCheckParams: _sanityCheckParams + }); Escrow internal _escrow; @@ -122,14 +128,22 @@ contract DualGovernanceUnitTests is UnitTest { _sanityCheckParams.minTiebreakerActivationTimeout = Durations.from(999); _sanityCheckParams.maxTiebreakerActivationTimeout = Durations.from(1000); - new DualGovernance({dependencies: _externalDependencies, sanityCheckParams: _sanityCheckParams}); + new DualGovernance({ + components: _dgComponents, + signallingTokens: _signallingTokens, + sanityCheckParams: _sanityCheckParams + }); } function test_constructor_min_max_timeout_same() external { _sanityCheckParams.minTiebreakerActivationTimeout = Durations.from(1000); _sanityCheckParams.maxTiebreakerActivationTimeout = Durations.from(1000); - new DualGovernance({dependencies: _externalDependencies, sanityCheckParams: _sanityCheckParams}); + new DualGovernance({ + components: _dgComponents, + signallingTokens: _signallingTokens, + sanityCheckParams: _sanityCheckParams + }); } function test_constructor_RevertsOn_InvalidTiebreakerActivationTimeoutBounds() external { @@ -138,7 +152,11 @@ contract DualGovernanceUnitTests is UnitTest { vm.expectRevert(abi.encodeWithSelector(DualGovernance.InvalidTiebreakerActivationTimeoutBounds.selector)); - new DualGovernance({dependencies: _externalDependencies, sanityCheckParams: _sanityCheckParams}); + new DualGovernance({ + components: _dgComponents, + signallingTokens: _signallingTokens, + sanityCheckParams: _sanityCheckParams + }); } // --- @@ -160,21 +178,25 @@ contract DualGovernanceUnitTests is UnitTest { Duration minTiebreakerActivationTimeout = Durations.from(30 days); Duration maxTiebreakerActivationTimeout = Durations.from(180 days); uint256 maxSealableWithdrawalBlockersCount = 128; + Duration maxAssetsLockDuration = Durations.from(365 days); DualGovernance dualGovernanceLocal = new DualGovernance({ - dependencies: DualGovernance.ExternalDependencies({ - stETH: _STETH_MOCK, - wstETH: _WSTETH_STUB, - withdrawalQueue: _WITHDRAWAL_QUEUE_MOCK, + components: DualGovernance.DualGovernanceComponents({ timelock: _timelock, resealManager: _RESEAL_MANAGER_STUB, configProvider: _configProvider }), + signallingTokens: DualGovernance.SignallingTokens({ + stETH: _STETH_MOCK, + wstETH: _WSTETH_STUB, + withdrawalQueue: _WITHDRAWAL_QUEUE_MOCK + }), sanityCheckParams: DualGovernance.SanityCheckParams({ minWithdrawalsBatchSize: 4, minTiebreakerActivationTimeout: minTiebreakerActivationTimeout, maxTiebreakerActivationTimeout: maxTiebreakerActivationTimeout, - maxSealableWithdrawalBlockersCount: maxSealableWithdrawalBlockersCount + maxSealableWithdrawalBlockersCount: maxSealableWithdrawalBlockersCount, + maxAssetsLockDuration: maxAssetsLockDuration }) }); @@ -183,6 +205,7 @@ contract DualGovernanceUnitTests is UnitTest { assertEq(dualGovernanceLocal.MAX_TIEBREAKER_ACTIVATION_TIMEOUT(), maxTiebreakerActivationTimeout); assertEq(dualGovernanceLocal.MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT(), maxSealableWithdrawalBlockersCount); assertEq(address(dualGovernanceLocal.ESCROW_MASTER_COPY()), predictedEscrowCopyAddress); + assertEq(dualGovernanceLocal.ESCROW_MASTER_COPY().MAX_ASSETS_LOCK_DURATION(), maxAssetsLockDuration); } // --- From e77f36c21b62d86efbc9e60634aa97fdcd4f5f79 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Fri, 6 Dec 2024 14:34:39 +0300 Subject: [PATCH 3/5] Rename MAX_ASSETS_LOCK_DURATION to MAX_MIN_ASSETS_LOCK_DURATION for consistency --- contracts/DualGovernance.sol | 8 ++++---- contracts/Escrow.sol | 8 ++++---- contracts/interfaces/IEscrow.sol | 2 -- contracts/libraries/EscrowState.sol | 10 ++++++---- scripts/deploy/Config.sol | 4 ++-- scripts/deploy/ContractsDeployment.sol | 2 +- scripts/deploy/JsonConfig.s.sol | 6 +++--- test/mocks/EscrowMock.sol | 10 +++++----- test/unit/DualGovernance.t.sol | 10 ++++++---- test/unit/libraries/EscrowState.t.sol | 14 +++++++------- 10 files changed, 38 insertions(+), 36 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index 4b78cf17..276ceec5 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -69,13 +69,13 @@ contract DualGovernance is IDualGovernance { /// @param maxSealableWithdrawalBlockersCount The upper bound for the number of sealable withdrawal blockers allowed to be /// registered in the Dual Governance. This parameter prevents filling the sealable withdrawal blockers /// with so many items that tiebreaker calls would revert due to out-of-gas errors. - /// @param maxAssetsLockDuration The maximum duration for which assets can be locked in the Rage Quit Escrow contract. + /// @param maxMinAssetsLockDuration The upper bound for the minimum duration of assets lock in the Escrow. struct SanityCheckParams { uint256 minWithdrawalsBatchSize; Duration minTiebreakerActivationTimeout; Duration maxTiebreakerActivationTimeout; uint256 maxSealableWithdrawalBlockersCount; - Duration maxAssetsLockDuration; + Duration maxMinAssetsLockDuration; } /// @notice The lower bound for the time the Dual Governance must spend in the "locked" state @@ -95,7 +95,7 @@ contract DualGovernance is IDualGovernance { // External Dependencies // --- - /// @notice Token addresses tha used in the Dual Governance as signalling tokens. + /// @notice Token addresses that used in the Dual Governance as signalling tokens. /// @param stETH The address of the stETH token. /// @param wstETH The address of the wstETH token. /// @param withdrawalQueue The address of Lido's Withdrawal Queue and the unstETH token. @@ -167,7 +167,7 @@ contract DualGovernance is IDualGovernance { wstETH: signallingTokens.wstETH, withdrawalQueue: signallingTokens.withdrawalQueue, minWithdrawalsBatchSize: sanityCheckParams.minWithdrawalsBatchSize, - maxAssetsLockDuration: sanityCheckParams.maxAssetsLockDuration + maxMinAssetsLockDuration: sanityCheckParams.maxMinAssetsLockDuration }); emit EscrowMasterCopyDeployed(ESCROW_MASTER_COPY); diff --git a/contracts/Escrow.sol b/contracts/Escrow.sol index 1c5f9081..38208429 100644 --- a/contracts/Escrow.sol +++ b/contracts/Escrow.sol @@ -59,7 +59,7 @@ contract Escrow is IEscrow { uint256 public immutable MIN_WITHDRAWALS_BATCH_SIZE; /// @notice The maximum duration that can be set as the minimum assets lock duration. - Duration public immutable MAX_ASSETS_LOCK_DURATION; + Duration public immutable MAX_MIN_ASSETS_LOCK_DURATION; // --- // Dependencies Immutables @@ -108,7 +108,7 @@ contract Escrow is IEscrow { IWithdrawalQueue withdrawalQueue, IDualGovernance dualGovernance, uint256 minWithdrawalsBatchSize, - Duration maxAssetsLockDuration + Duration maxMinAssetsLockDuration ) { _SELF = address(this); DUAL_GOVERNANCE = dualGovernance; @@ -118,7 +118,7 @@ contract Escrow is IEscrow { WITHDRAWAL_QUEUE = withdrawalQueue; MIN_WITHDRAWALS_BATCH_SIZE = minWithdrawalsBatchSize; - MAX_ASSETS_LOCK_DURATION = maxAssetsLockDuration; + MAX_MIN_ASSETS_LOCK_DURATION = maxMinAssetsLockDuration; } /// @notice Initializes the proxy instance with the specified minimum assets lock duration. @@ -437,7 +437,7 @@ contract Escrow is IEscrow { /// @param newMinAssetsLockDuration The new minimum lock duration to be set. function setMinAssetsLockDuration(Duration newMinAssetsLockDuration) external { _checkCallerIsDualGovernance(); - _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration, MAX_ASSETS_LOCK_DURATION); + _escrowState.setMinAssetsLockDuration(newMinAssetsLockDuration, MAX_MIN_ASSETS_LOCK_DURATION); } // --- diff --git a/contracts/interfaces/IEscrow.sol b/contracts/interfaces/IEscrow.sol index a0e8973c..a51a5030 100644 --- a/contracts/interfaces/IEscrow.sol +++ b/contracts/interfaces/IEscrow.sol @@ -32,8 +32,6 @@ interface IEscrow { function initialize(Duration minAssetsLockDuration) external; - function MAX_ASSETS_LOCK_DURATION() external view returns (Duration); - function lockStETH(uint256 amount) external returns (uint256 lockedStETHShares); function unlockStETH() external returns (uint256 unlockedStETHShares); function lockWstETH(uint256 amount) external returns (uint256 lockedStETHShares); diff --git a/contracts/libraries/EscrowState.sol b/contracts/libraries/EscrowState.sol index dbd37c5b..844e6158 100644 --- a/contracts/libraries/EscrowState.sol +++ b/contracts/libraries/EscrowState.sol @@ -103,14 +103,16 @@ library EscrowState { /// @notice Sets the minimum assets lock duration. /// @param self The context of the Escrow State library. /// @param newMinAssetsLockDuration The new minimum assets lock duration. - /// @param maxAssetsLockDuration Sanity check for max assets lock duration. + /// @param maxMinAssetsLockDuration Sanity check for max assets lock duration. function setMinAssetsLockDuration( Context storage self, Duration newMinAssetsLockDuration, - Duration maxAssetsLockDuration + Duration maxMinAssetsLockDuration ) internal { - if (self.minAssetsLockDuration == newMinAssetsLockDuration || newMinAssetsLockDuration > maxAssetsLockDuration) - { + if ( + self.minAssetsLockDuration == newMinAssetsLockDuration + || newMinAssetsLockDuration > maxMinAssetsLockDuration + ) { revert InvalidMinAssetsLockDuration(newMinAssetsLockDuration); } _setMinAssetsLockDuration(self, newMinAssetsLockDuration); diff --git a/scripts/deploy/Config.sol b/scripts/deploy/Config.sol index bb438f90..a685ba14 100644 --- a/scripts/deploy/Config.sol +++ b/scripts/deploy/Config.sol @@ -31,7 +31,7 @@ uint256 constant DEFAULT_MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT = 255; uint256 constant DEFAULT_FIRST_SEAL_RAGE_QUIT_SUPPORT = 3_00; // 3% uint256 constant DEFAULT_SECOND_SEAL_RAGE_QUIT_SUPPORT = 15_00; // 15% uint256 constant DEFAULT_MIN_ASSETS_LOCK_DURATION = 5 hours; -uint256 constant DEFAULT_MAX_ASSETS_LOCK_DURATION = 365 days; +uint256 constant DEFAULT_MAX_MIN_ASSETS_LOCK_DURATION = 365 days; uint256 constant DEFAULT_VETO_SIGNALLING_MIN_DURATION = 3 days; uint256 constant DEFAULT_VETO_SIGNALLING_MAX_DURATION = 30 days; uint256 constant DEFAULT_VETO_SIGNALLING_MIN_ACTIVE_DURATION = 5 hours; @@ -81,7 +81,7 @@ struct DeployConfig { PercentD16 FIRST_SEAL_RAGE_QUIT_SUPPORT; PercentD16 SECOND_SEAL_RAGE_QUIT_SUPPORT; Duration MIN_ASSETS_LOCK_DURATION; - Duration MAX_ASSETS_LOCK_DURATION; + Duration MAX_MIN_ASSETS_LOCK_DURATION; Duration VETO_SIGNALLING_MIN_DURATION; Duration VETO_SIGNALLING_MAX_DURATION; Duration VETO_SIGNALLING_MIN_ACTIVE_DURATION; diff --git a/scripts/deploy/ContractsDeployment.sol b/scripts/deploy/ContractsDeployment.sol index af87be0e..0ac0a25d 100644 --- a/scripts/deploy/ContractsDeployment.sol +++ b/scripts/deploy/ContractsDeployment.sol @@ -231,7 +231,7 @@ library DGContractsDeployment { minTiebreakerActivationTimeout: dgDeployConfig.MIN_TIEBREAKER_ACTIVATION_TIMEOUT, maxTiebreakerActivationTimeout: dgDeployConfig.MAX_TIEBREAKER_ACTIVATION_TIMEOUT, maxSealableWithdrawalBlockersCount: dgDeployConfig.MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT, - maxAssetsLockDuration: dgDeployConfig.MAX_ASSETS_LOCK_DURATION + maxMinAssetsLockDuration: dgDeployConfig.MAX_MIN_ASSETS_LOCK_DURATION }) }); } diff --git a/scripts/deploy/JsonConfig.s.sol b/scripts/deploy/JsonConfig.s.sol index f4227b21..6ff4e783 100644 --- a/scripts/deploy/JsonConfig.s.sol +++ b/scripts/deploy/JsonConfig.s.sol @@ -46,7 +46,7 @@ import { DEFAULT_FIRST_SEAL_RAGE_QUIT_SUPPORT, DEFAULT_SECOND_SEAL_RAGE_QUIT_SUPPORT, DEFAULT_MIN_ASSETS_LOCK_DURATION, - DEFAULT_MAX_ASSETS_LOCK_DURATION, + DEFAULT_MAX_MIN_ASSETS_LOCK_DURATION, DEFAULT_VETO_SIGNALLING_MIN_DURATION, DEFAULT_VETO_SIGNALLING_MAX_DURATION, DEFAULT_VETO_SIGNALLING_MIN_ACTIVE_DURATION, @@ -174,8 +174,8 @@ contract DGDeployJSONConfigProvider is Script { MIN_ASSETS_LOCK_DURATION: Durations.from( stdJson.readUintOr(jsonConfig, ".MIN_ASSETS_LOCK_DURATION", DEFAULT_MIN_ASSETS_LOCK_DURATION) ), - MAX_ASSETS_LOCK_DURATION: Durations.from( - stdJson.readUintOr(jsonConfig, ".MAX_ASSETS_LOCK_DURATION", DEFAULT_MAX_ASSETS_LOCK_DURATION) + MAX_MIN_ASSETS_LOCK_DURATION: Durations.from( + stdJson.readUintOr(jsonConfig, ".MAX_MIN_ASSETS_LOCK_DURATION", DEFAULT_MAX_MIN_ASSETS_LOCK_DURATION) ), VETO_SIGNALLING_MIN_DURATION: Durations.from( stdJson.readUintOr(jsonConfig, ".VETO_SIGNALLING_MIN_DURATION", DEFAULT_VETO_SIGNALLING_MIN_DURATION) diff --git a/test/mocks/EscrowMock.sol b/test/mocks/EscrowMock.sol index ed58f4a1..07dd6180 100644 --- a/test/mocks/EscrowMock.sol +++ b/test/mocks/EscrowMock.sol @@ -12,7 +12,7 @@ contract EscrowMock is IEscrow { event __RageQuitStarted(Duration rageQuitExtraTimelock, Duration rageQuitWithdrawalsTimelock); Duration public __minAssetsLockDuration; - Duration public __maxAssetsLockDuration; + Duration public __maxMinAssetsLockDuration; PercentD16 public __rageQuitSupport; bool public __isRageQuitFinalized; @@ -132,11 +132,11 @@ contract EscrowMock is IEscrow { return __minAssetsLockDuration; } - function MAX_ASSETS_LOCK_DURATION() external view returns (Duration) { - return __maxAssetsLockDuration; + function MAX_MIN_ASSETS_LOCK_DURATION() external view returns (Duration) { + return __maxMinAssetsLockDuration; } - function setMaxAssetsLockDuration(Duration newMaxAssetsLockDuration) external { - __maxAssetsLockDuration = newMaxAssetsLockDuration; + function setmaxMinAssetsLockDuration(Duration newmaxMinAssetsLockDuration) external { + __maxMinAssetsLockDuration = newmaxMinAssetsLockDuration; } } diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index e678a87a..bfc37f4f 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -88,7 +88,7 @@ contract DualGovernanceUnitTests is UnitTest { minTiebreakerActivationTimeout: Durations.from(30 days), maxTiebreakerActivationTimeout: Durations.from(180 days), maxSealableWithdrawalBlockersCount: 128, - maxAssetsLockDuration: Durations.from(365 days) + maxMinAssetsLockDuration: Durations.from(365 days) }); DualGovernance internal _dualGovernance = new DualGovernance({ @@ -178,7 +178,7 @@ contract DualGovernanceUnitTests is UnitTest { Duration minTiebreakerActivationTimeout = Durations.from(30 days); Duration maxTiebreakerActivationTimeout = Durations.from(180 days); uint256 maxSealableWithdrawalBlockersCount = 128; - Duration maxAssetsLockDuration = Durations.from(365 days); + Duration maxMinAssetsLockDuration = Durations.from(365 days); DualGovernance dualGovernanceLocal = new DualGovernance({ components: DualGovernance.DualGovernanceComponents({ @@ -196,7 +196,7 @@ contract DualGovernanceUnitTests is UnitTest { minTiebreakerActivationTimeout: minTiebreakerActivationTimeout, maxTiebreakerActivationTimeout: maxTiebreakerActivationTimeout, maxSealableWithdrawalBlockersCount: maxSealableWithdrawalBlockersCount, - maxAssetsLockDuration: maxAssetsLockDuration + maxMinAssetsLockDuration: maxMinAssetsLockDuration }) }); @@ -205,7 +205,9 @@ contract DualGovernanceUnitTests is UnitTest { assertEq(dualGovernanceLocal.MAX_TIEBREAKER_ACTIVATION_TIMEOUT(), maxTiebreakerActivationTimeout); assertEq(dualGovernanceLocal.MAX_SEALABLE_WITHDRAWAL_BLOCKERS_COUNT(), maxSealableWithdrawalBlockersCount); assertEq(address(dualGovernanceLocal.ESCROW_MASTER_COPY()), predictedEscrowCopyAddress); - assertEq(dualGovernanceLocal.ESCROW_MASTER_COPY().MAX_ASSETS_LOCK_DURATION(), maxAssetsLockDuration); + + address payable escrowMasterCopyAddress = payable(address(dualGovernanceLocal.ESCROW_MASTER_COPY())); + assertEq(Escrow(escrowMasterCopyAddress).MAX_MIN_ASSETS_LOCK_DURATION(), maxMinAssetsLockDuration); } // --- diff --git a/test/unit/libraries/EscrowState.t.sol b/test/unit/libraries/EscrowState.t.sol index 88ff03f0..9beb5920 100644 --- a/test/unit/libraries/EscrowState.t.sol +++ b/test/unit/libraries/EscrowState.t.sol @@ -105,15 +105,15 @@ contract EscrowStateUnitTests is UnitTest { function testFuzz_setMinAssetsLockDuration_happyPath( Duration minAssetsLockDuration, - Duration maxAssetsLockDuration + Duration maxMinAssetsLockDuration ) external { vm.assume(minAssetsLockDuration != Durations.ZERO); - vm.assume(minAssetsLockDuration <= maxAssetsLockDuration); + vm.assume(minAssetsLockDuration <= maxMinAssetsLockDuration); vm.expectEmit(); emit EscrowState.MinAssetsLockDurationSet(minAssetsLockDuration); - EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxAssetsLockDuration); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxMinAssetsLockDuration); checkContext({ state: State.NotInitialized, @@ -133,16 +133,16 @@ contract EscrowStateUnitTests is UnitTest { EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, Durations.from(type(uint16).max)); } - function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationGreaterThenMaxAssetsLockDuration( + function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationGreaterThenmaxMinAssetsLockDuration( Duration minAssetsLockDuration, - Duration maxAssetsLockDuration + Duration maxMinAssetsLockDuration ) external { - vm.assume(minAssetsLockDuration > maxAssetsLockDuration); + vm.assume(minAssetsLockDuration > maxMinAssetsLockDuration); vm.expectRevert( abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, minAssetsLockDuration) ); - EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxAssetsLockDuration); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, maxMinAssetsLockDuration); } // --- From a8f3edecc90ec0c59a2378ad72edd79a6bdd8f0c Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Fri, 6 Dec 2024 19:44:06 +0300 Subject: [PATCH 4/5] using IResealManager --- contracts/DualGovernance.sol | 4 ++-- contracts/interfaces/IDualGovernance.sol | 2 +- contracts/libraries/Resealer.sol | 10 +++++----- test/unit/DualGovernance.t.sol | 5 +++-- test/unit/libraries/Resealer.t.sol | 12 ++++++------ 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index f01eeeff..b8c4ed31 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -169,7 +169,7 @@ contract DualGovernance is IDualGovernance { emit EscrowMasterCopyDeployed(escrowMasterCopy); _stateMachine.initialize(components.configProvider, escrowMasterCopy); - _resealer.setResealManager(address(components.resealManager)); + _resealer.setResealManager(components.resealManager); } // --- @@ -532,7 +532,7 @@ contract DualGovernance is IDualGovernance { /// @notice Sets the address of the Reseal Manager. /// @param newResealManager The address of the new Reseal Manager. - function setResealManager(address newResealManager) external { + function setResealManager(IResealManager newResealManager) external { _checkCallerIsAdminExecutor(); _resealer.setResealManager(newResealManager); } diff --git a/contracts/interfaces/IDualGovernance.sol b/contracts/interfaces/IDualGovernance.sol index 721b2eba..336b1d82 100644 --- a/contracts/interfaces/IDualGovernance.sol +++ b/contracts/interfaces/IDualGovernance.sol @@ -47,7 +47,7 @@ interface IDualGovernance is IGovernance, ITiebreaker { function resealSealable(address sealable) external; function setResealCommittee(address newResealCommittee) external; - function setResealManager(address newResealManager) external; + function setResealManager(IResealManager newResealManager) external; function getResealManager() external view returns (IResealManager); function getResealCommittee() external view returns (address); } diff --git a/contracts/libraries/Resealer.sol b/contracts/libraries/Resealer.sol index 176a6fb1..4b176512 100644 --- a/contracts/libraries/Resealer.sol +++ b/contracts/libraries/Resealer.sol @@ -9,7 +9,7 @@ library Resealer { // --- // Errors // --- - error InvalidResealManager(address resealManager); + error InvalidResealManager(IResealManager resealManager); error InvalidResealCommittee(address resealCommittee); error CallerIsNotResealCommittee(address caller); @@ -17,7 +17,7 @@ library Resealer { // Events // --- event ResealCommitteeSet(address resealCommittee); - event ResealManagerSet(address resealManager); + event ResealManagerSet(IResealManager resealManager); // --- // Data Types @@ -35,11 +35,11 @@ library Resealer { /// @dev Sets a new Reseal Manager contract address. /// @param self The context struct containing the current state. /// @param newResealManager The address of the new Reseal Manager. - function setResealManager(Context storage self, address newResealManager) internal { - if (newResealManager == address(self.resealManager) || newResealManager == address(0)) { + function setResealManager(Context storage self, IResealManager newResealManager) internal { + if (newResealManager == self.resealManager || address(newResealManager) == address(0)) { revert InvalidResealManager(newResealManager); } - self.resealManager = IResealManager(newResealManager); + self.resealManager = newResealManager; emit ResealManagerSet(newResealManager); } diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index 561bcfbb..3b9c1088 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -173,7 +173,7 @@ contract DualGovernanceUnitTests is UnitTest { vm.expectEmit(); emit DualGovernance.EscrowMasterCopyDeployed(IEscrowBase(predictedEscrowCopyAddress)); vm.expectEmit(); - emit Resealer.ResealManagerSet(address(_RESEAL_MANAGER_STUB)); + emit Resealer.ResealManagerSet(_RESEAL_MANAGER_STUB); Duration minTiebreakerActivationTimeout = Durations.from(30 days); Duration maxTiebreakerActivationTimeout = Durations.from(180 days); @@ -2371,10 +2371,11 @@ contract DualGovernanceUnitTests is UnitTest { function test_setResealManger_RevertOn_CallerIsNotAdminExecutor(address stranger) external { vm.assume(stranger != address(_executor)); + address newResealManager = makeAddr("NEW_RESEAL_MANAGER"); vm.prank(stranger); vm.expectRevert(abi.encodeWithSelector(DualGovernance.CallerIsNotAdminExecutor.selector, stranger)); - _dualGovernance.setResealManager(address(0x123)); + _dualGovernance.setResealManager(IResealManager(newResealManager)); } // --- diff --git a/test/unit/libraries/Resealer.t.sol b/test/unit/libraries/Resealer.t.sol index dddd84da..f06c9903 100644 --- a/test/unit/libraries/Resealer.t.sol +++ b/test/unit/libraries/Resealer.t.sol @@ -18,22 +18,22 @@ contract ResealerTest is UnitTest { ctx.resealCommittee = resealCommittee; } - function test_setResealManager_HappyPath(address newResealManager) external { - vm.assume(newResealManager != address(ctx.resealManager) && newResealManager != address(0)); + function testFuzz_setResealManager_HappyPath(IResealManager newResealManager) external { + vm.assume(newResealManager != ctx.resealManager && address(newResealManager) != address(0)); vm.expectEmit(); emit Resealer.ResealManagerSet(newResealManager); this.external__setResealManager(newResealManager); - assertEq(address(ctx.resealManager), newResealManager); + assertEq(address(ctx.resealManager), address(newResealManager)); } function test_setResealManager_RevertOn_InvalidResealManager() external { vm.expectRevert(abi.encodeWithSelector(Resealer.InvalidResealManager.selector, address(ctx.resealManager))); - this.external__setResealManager(address(ctx.resealManager)); + this.external__setResealManager(ctx.resealManager); vm.expectRevert(abi.encodeWithSelector(Resealer.InvalidResealManager.selector, address(0))); - this.external__setResealManager(address(0)); + this.external__setResealManager(IResealManager(address(0))); } function testFuzz_setResealCommittee_HappyPath(address newResealCommittee) external { @@ -72,7 +72,7 @@ contract ResealerTest is UnitTest { ctx.setResealCommittee(newResealCommittee); } - function external__setResealManager(address newResealManager) external { + function external__setResealManager(IResealManager newResealManager) external { ctx.setResealManager(newResealManager); } } From 602235ff3374cb4521248de7abcfa39b4395dcf3 Mon Sep 17 00:00:00 2001 From: Aleksandr Tarelkin Date: Fri, 6 Dec 2024 19:59:20 +0300 Subject: [PATCH 5/5] detailed error --- contracts/DualGovernance.sol | 8 ++++++-- test/unit/DualGovernance.t.sol | 8 +++++++- test/unit/Escrow.t.sol | 6 ++++-- test/unit/libraries/EscrowState.t.sol | 4 ++-- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index b8c4ed31..e8c6e0b7 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -44,7 +44,9 @@ contract DualGovernance is IDualGovernance { error ProposalSubmissionBlocked(); error ProposalSchedulingBlocked(uint256 proposalId); error ResealIsNotAllowedInNormalState(); - error InvalidTiebreakerActivationTimeoutBounds(); + error InvalidTiebreakerActivationTimeoutBounds( + Duration minTiebreakerActivationTimeout, Duration maxTiebreakerActivationTimeout + ); // --- // Events @@ -148,7 +150,9 @@ contract DualGovernance is IDualGovernance { SanityCheckParams memory sanityCheckParams ) { if (sanityCheckParams.minTiebreakerActivationTimeout > sanityCheckParams.maxTiebreakerActivationTimeout) { - revert InvalidTiebreakerActivationTimeoutBounds(); + revert InvalidTiebreakerActivationTimeoutBounds( + sanityCheckParams.minTiebreakerActivationTimeout, sanityCheckParams.maxTiebreakerActivationTimeout + ); } TIMELOCK = components.timelock; diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index 3b9c1088..20263fdc 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -150,7 +150,13 @@ contract DualGovernanceUnitTests is UnitTest { _sanityCheckParams.minTiebreakerActivationTimeout = Durations.from(1000); _sanityCheckParams.maxTiebreakerActivationTimeout = Durations.from(999); - vm.expectRevert(abi.encodeWithSelector(DualGovernance.InvalidTiebreakerActivationTimeoutBounds.selector)); + vm.expectRevert( + abi.encodeWithSelector( + DualGovernance.InvalidTiebreakerActivationTimeoutBounds.selector, + _sanityCheckParams.minTiebreakerActivationTimeout, + _sanityCheckParams.maxTiebreakerActivationTimeout + ) + ); new DualGovernance({ components: _dgComponents, diff --git a/test/unit/Escrow.t.sol b/test/unit/Escrow.t.sol index 32eee4ff..05f38efa 100644 --- a/test/unit/Escrow.t.sol +++ b/test/unit/Escrow.t.sol @@ -94,7 +94,8 @@ contract EscrowUnitTests is UnitTest { address wsteth, address withdrawalQueue, address dualGovernance, - uint256 size + uint256 size, + Duration maxMinAssetsLockDuration ) external { Escrow instance = new Escrow( IStETH(steth), @@ -102,7 +103,7 @@ contract EscrowUnitTests is UnitTest { IWithdrawalQueue(withdrawalQueue), IDualGovernance(dualGovernance), size, - _maxMinAssetsLockDuration + maxMinAssetsLockDuration ); assertEq(address(instance.ST_ETH()), address(steth)); @@ -110,6 +111,7 @@ contract EscrowUnitTests is UnitTest { assertEq(address(instance.WITHDRAWAL_QUEUE()), address(withdrawalQueue)); assertEq(address(instance.DUAL_GOVERNANCE()), address(dualGovernance)); assertEq(instance.MIN_WITHDRAWALS_BATCH_SIZE(), size); + assertEq(instance.MAX_MIN_ASSETS_LOCK_DURATION(), maxMinAssetsLockDuration); } // --- diff --git a/test/unit/libraries/EscrowState.t.sol b/test/unit/libraries/EscrowState.t.sol index 9beb5920..b4040116 100644 --- a/test/unit/libraries/EscrowState.t.sol +++ b/test/unit/libraries/EscrowState.t.sol @@ -130,10 +130,10 @@ contract EscrowStateUnitTests is UnitTest { vm.expectRevert( abi.encodeWithSelector(EscrowState.InvalidMinAssetsLockDuration.selector, minAssetsLockDuration) ); - EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, Durations.from(type(uint16).max)); + EscrowState.setMinAssetsLockDuration(_context, minAssetsLockDuration, Durations.from(MAX_DURATION_VALUE)); } - function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationGreaterThenmaxMinAssetsLockDuration( + function testFuzz_setMinAssetsLockDuration_RevertWhen_DurationGreaterThenMaxMinAssetsLockDuration( Duration minAssetsLockDuration, Duration maxMinAssetsLockDuration ) external {