From 3021d2715ac7d75508892c3abde11132ce63770a Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 21 Aug 2024 08:55:37 -0400 Subject: [PATCH 1/5] admin is already default admin role by default --- src/Executor.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index 440e4a1..39af72e 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -47,9 +47,6 @@ contract Executor is IExecutor, AccessControl { _updateDelay(delay_); _updateGracePeriod(gracePeriod_); - _setRoleAdmin(SUBMISSION_ROLE, DEFAULT_ADMIN_ROLE); - _setRoleAdmin(GUARDIAN_ROLE, DEFAULT_ADMIN_ROLE); - _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); _grantRole(DEFAULT_ADMIN_ROLE, address(this)); // Necessary for self-referential calls to change configuration } From b0586cf1453de97a91eaa797e72940ba4d51df3a Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 21 Aug 2024 08:58:22 -0400 Subject: [PATCH 2/5] merge dupe grace period check --- src/Executor.sol | 6 +----- src/interfaces/IExecutor.sol | 1 - test/Executor.t.sol | 2 +- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index 39af72e..d6c0687 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -40,10 +40,6 @@ contract Executor is IExecutor, AccessControl { uint256 delay_, uint256 gracePeriod_ ) { - if ( - gracePeriod_ < MINIMUM_GRACE_PERIOD - ) revert InvalidInitParams(); - _updateDelay(delay_); _updateGracePeriod(gracePeriod_); @@ -148,7 +144,6 @@ contract Executor is IExecutor, AccessControl { function updateGracePeriod(uint256 newGracePeriod) external override onlyRole(DEFAULT_ADMIN_ROLE) { - if (newGracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); _updateGracePeriod(newGracePeriod); } @@ -229,6 +224,7 @@ contract Executor is IExecutor, AccessControl { } function _updateGracePeriod(uint256 newGracePeriod) internal { + if (newGracePeriod < MINIMUM_GRACE_PERIOD) revert GracePeriodTooShort(); emit GracePeriodUpdate(gracePeriod, newGracePeriod); gracePeriod = newGracePeriod; } diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index 8daec0b..68e4b2b 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -14,7 +14,6 @@ interface IExecutor is IAccessControl { /*** Errors ***/ /******************************************************************************************************************/ - error InvalidInitParams(); error GracePeriodTooShort(); error OnlyQueuedActions(); error TimelockNotFinished(); diff --git a/test/Executor.t.sol b/test/Executor.t.sol index c7643f2..7d06cb9 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -137,7 +137,7 @@ contract ExecutorTestBase is Test { contract ExecutorConstructorTests is ExecutorTestBase { function test_constructor_invalidInitParams_boundary() public { - vm.expectRevert(abi.encodeWithSignature("InvalidInitParams()")); + vm.expectRevert(abi.encodeWithSignature("GracePeriodTooShort()")); executor = new Executor({ delay_: DELAY, gracePeriod_: 10 minutes - 1 From 1940e6c5bd5a08d48fd8f72333198c5475b926f7 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 21 Aug 2024 09:42:42 -0400 Subject: [PATCH 3/5] spacing --- src/Executor.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Executor.sol b/src/Executor.sol index d6c0687..2d9a56b 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -183,7 +183,7 @@ contract Executor is IExecutor, AccessControl { function getCurrentState(uint256 actionsSetId) public view override returns (ActionsSetState) { if (actionsSetCount <= actionsSetId) revert InvalidActionsSetId(); - ActionsSet storage actionsSet =_actionsSets[actionsSetId]; + ActionsSet storage actionsSet = _actionsSets[actionsSetId]; if (actionsSet.canceled) return ActionsSetState.Canceled; else if (actionsSet.executed) return ActionsSetState.Executed; From 2ca228fca9f18811c1e19d5c3b52cab87e76e274 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 21 Aug 2024 09:58:38 -0400 Subject: [PATCH 4/5] remove unused param --- src/Executor.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index 2d9a56b..30fa10c 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -113,7 +113,6 @@ contract Executor is IExecutor, AccessControl { actionsSet.values[i], actionsSet.signatures[i], actionsSet.calldatas[i], - actionsSet.executionTime, actionsSet.withDelegatecalls[i] ); } @@ -200,7 +199,6 @@ contract Executor is IExecutor, AccessControl { uint256 value, string memory signature, bytes memory data, - uint256 executionTime, bool withDelegatecall ) internal returns (bytes memory) { if (address(this).balance < value) revert InsufficientBalance(); From 94a84a5976aa226efc04c833407dd9fa18da1f60 Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 21 Aug 2024 10:02:25 -0400 Subject: [PATCH 5/5] better wording on comment --- src/interfaces/IExecutor.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index 68e4b2b..5041cea 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -160,7 +160,7 @@ interface IExecutor is IAccessControl { * @dev If a signature is empty, calldata is used for the execution, calldata is appended to signature otherwise. * @param targets Array of targets to be called by the actions set. * @param values Array of values to pass in each call by the actions set. - * @param signatures Array of function signatures to encode in each call by the actions (can be empty). + * @param signatures Array of function signatures to encode in each call by the actions which can be empty strings. * @param calldatas Array of calldata to pass in each call by the actions set. * @param withDelegatecalls Array of whether to delegatecall for each call of the actions set. **/