From 5ed105fd33b5524cb6e493e9fdc56985c4a2391a Mon Sep 17 00:00:00 2001 From: Sam MacPherson Date: Wed, 31 Jul 2024 21:13:54 +0900 Subject: [PATCH] remove the useless isActionQueued logic (#23) --- src/Executor.sol | 46 ------------------------------------ src/interfaces/IExecutor.sol | 9 ------- test/Executor.t.sol | 46 ------------------------------------ 3 files changed, 101 deletions(-) diff --git a/src/Executor.sol b/src/Executor.sol index e0524e1..440e4a1 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -31,8 +31,6 @@ contract Executor is IExecutor, AccessControl { uint256 public override delay; // Time between queuing and execution uint256 public override gracePeriod; // Time after delay during which an actions set can be executed - mapping(bytes32 => bool) public override isActionQueued; - /** * @dev Constructor * @param delay_ The delay before which an actions set can be executed. @@ -83,22 +81,6 @@ contract Executor is IExecutor, AccessControl { unchecked { ++actionsSetCount; } - for (uint256 i = 0; i < targetsLength; ++i) { - bytes32 actionHash = keccak256( - abi.encode( - targets[i], - values[i], - signatures[i], - calldatas[i], - executionTime, - withDelegatecalls[i] - ) - ); - if (isActionQueued[actionHash]) revert DuplicateAction(); - - isActionQueued[actionHash] = true; - } - ActionsSet storage actionsSet = _actionsSets[actionsSetId]; actionsSet.targets = targets; @@ -153,18 +135,6 @@ contract Executor is IExecutor, AccessControl { ActionsSet storage actionsSet = _actionsSets[actionsSetId]; actionsSet.canceled = true; - uint256 targetsLength = actionsSet.targets.length; - for (uint256 i = 0; i < targetsLength; ++i) { - _removeActionFromQueue( - actionsSet.targets[i], - actionsSet.values[i], - actionsSet.signatures[i], - actionsSet.calldatas[i], - actionsSet.executionTime, - actionsSet.withDelegatecalls[i] - ); - } - emit ActionsSetCanceled(actionsSetId); } @@ -243,8 +213,6 @@ contract Executor is IExecutor, AccessControl { ) internal returns (bytes memory) { if (address(this).balance < value) revert InsufficientBalance(); - _removeActionFromQueue(target, value, signature, data, executionTime, withDelegatecall); - bytes memory callData = bytes(signature).length == 0 ? data : abi.encodePacked(bytes4(keccak256(bytes(signature))), data); @@ -254,20 +222,6 @@ contract Executor is IExecutor, AccessControl { return target.functionCallWithValue(callData, value); } - function _removeActionFromQueue( - address target, - uint256 value, - string memory signature, - bytes memory data, - uint256 executionTime, - bool withDelegatecall - ) internal { - bytes32 actionHash = keccak256( - abi.encode(target, value, signature, data, executionTime, withDelegatecall) - ); - isActionQueued[actionHash] = false; - } - /******************************************************************************************************************/ /*** Internal admin helper functions ***/ /******************************************************************************************************************/ diff --git a/src/interfaces/IExecutor.sol b/src/interfaces/IExecutor.sol index 1869cf2..8daec0b 100644 --- a/src/interfaces/IExecutor.sol +++ b/src/interfaces/IExecutor.sol @@ -21,7 +21,6 @@ interface IExecutor is IAccessControl { error InvalidActionsSetId(); error EmptyTargets(); error InconsistentParamsLength(); - error DuplicateAction(); error InsufficientBalance(); /******************************************************************************************************************/ @@ -153,14 +152,6 @@ interface IExecutor is IAccessControl { **/ function gracePeriod() external view returns (uint256); - /** - * @notice Returns whether an actions set (by actionHash) is queued. - * @dev actionHash = keccak256(abi.encode(target, value, signature, data, executionTime, withDelegatecall)). - * @param actionHash hash of the action to be checked. - * @return True if the underlying action of actionHash is queued, false otherwise. - **/ - function isActionQueued(bytes32 actionHash) external view returns (bool); - /******************************************************************************************************************/ /*** ActionSet functions ***/ /******************************************************************************************************************/ diff --git a/test/Executor.t.sol b/test/Executor.t.sol index ec138bc..c7643f2 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -120,21 +120,6 @@ contract ExecutorTestBase is Test { _queueAction(action); } - function _encodeHash(Action memory action, uint256 index, uint256 executionTime) internal pure returns (bytes32) { - return keccak256(abi.encode( - action.targets[index], - action.values[index], - action.signatures[index], - action.calldatas[index], - executionTime, - action.withDelegatecalls[index] - )); - } - - function _encodeHash(Action memory action, uint256 executionTime) internal pure returns (bytes32) { - return _encodeHash(action, 0, executionTime); - } - function _assertActionSet(uint256 id, bool executed, bool canceled, uint256 executionTime, Action memory action) internal view { IExecutor.ActionsSet memory actionsSet = executor.getActionsSetById(id); assertEq(actionsSet.targets, action.targets); @@ -218,22 +203,10 @@ contract ExecutorQueueTests is ExecutorTestBase { executor.queue(new address[](1), new uint256[](1), new string[](1), new bytes[](1), new bool[](2)); } - function test_queue_duplicateAction() public { - Action memory action = _getDefaultAction(); - _queueAction(action); - - vm.expectRevert(abi.encodeWithSignature("DuplicateAction()")); - _queueAction(action); - } - function test_queue() public { Action memory action = _getDefaultAction(); - bytes32 actionHash1 = _encodeHash(action, block.timestamp + DELAY); - bytes32 actionHash2 = _encodeHash(action, block.timestamp + DELAY + 1); assertEq(executor.actionsSetCount(), 0); - assertEq(executor.isActionQueued(actionHash1), false); - assertEq(executor.isActionQueued(actionHash2), false); vm.expectEmit(address(executor)); emit ActionsSetQueued( @@ -248,8 +221,6 @@ contract ExecutorQueueTests is ExecutorTestBase { _queueAction(action); assertEq(executor.actionsSetCount(), 1); - assertEq(executor.isActionQueued(actionHash1), true); - assertEq(executor.isActionQueued(actionHash2), false); _assertActionSet({ id: 0, executed: false, @@ -273,8 +244,6 @@ contract ExecutorQueueTests is ExecutorTestBase { _queueAction(action); assertEq(executor.actionsSetCount(), 2); - assertEq(executor.isActionQueued(actionHash1), true); - assertEq(executor.isActionQueued(actionHash2), true); _assertActionSet({ id: 1, executed: false, @@ -427,11 +396,9 @@ contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_delegateCall() public { Action memory action = _getDefaultAction(); - bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); _queueAction(action); skip(DELAY); - assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); @@ -443,7 +410,6 @@ contract ExecutorExecuteTests is ExecutorTestBase { emit ActionsSetExecuted(0, address(this), returnedData); executor.execute(0); - assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } @@ -451,11 +417,9 @@ contract ExecutorExecuteTests is ExecutorTestBase { function test_execute_call() public { Action memory action = _getDefaultAction(); action.withDelegatecalls[0] = false; - bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); _queueAction(action); skip(DELAY); - assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); @@ -467,7 +431,6 @@ contract ExecutorExecuteTests is ExecutorTestBase { emit ActionsSetExecuted(0, address(this), returnedData); executor.execute(0); - assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } @@ -476,11 +439,9 @@ contract ExecutorExecuteTests is ExecutorTestBase { Action memory action = _getDefaultAction(); action.signatures[0] = ""; action.calldatas[0] = abi.encodeWithSignature("execute()"); - bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); _queueAction(action); skip(DELAY); - assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); @@ -492,7 +453,6 @@ contract ExecutorExecuteTests is ExecutorTestBase { emit ActionsSetExecuted(0, address(this), returnedData); executor.execute(0); - assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } @@ -502,11 +462,9 @@ contract ExecutorExecuteTests is ExecutorTestBase { action.signatures[0] = ""; action.calldatas[0] = abi.encodeWithSignature("execute()"); action.withDelegatecalls[0] = false; - bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); _queueAction(action); skip(DELAY); - assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).executed, false); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); @@ -518,7 +476,6 @@ contract ExecutorExecuteTests is ExecutorTestBase { emit ActionsSetExecuted(0, address(this), returnedData); executor.execute(0); - assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).executed, true); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Executed)); } @@ -586,10 +543,8 @@ contract ExecutorCancelTests is ExecutorTestBase { function test_cancel() public { Action memory action = _getDefaultAction(); - bytes32 actionHash = _encodeHash(action, block.timestamp + DELAY); _queueAction(action); - assertEq(executor.isActionQueued(actionHash), true); assertEq(executor.getActionsSetById(0).canceled, false); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Queued)); @@ -598,7 +553,6 @@ contract ExecutorCancelTests is ExecutorTestBase { vm.prank(guardian); executor.cancel(0); - assertEq(executor.isActionQueued(actionHash), false); assertEq(executor.getActionsSetById(0).canceled, true); assertEq(uint8(executor.getCurrentState(0)), uint8(IExecutor.ActionsSetState.Canceled)); }