Skip to content

Commit

Permalink
remove the useless isActionQueued logic (#23)
Browse files Browse the repository at this point in the history
  • Loading branch information
@hexonaut
hexonaut authored Jul 31, 2024
1 parent b6ce510 commit 5ed105f
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 101 deletions.
46 changes: 0 additions & 46 deletions src/Executor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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 ***/
/******************************************************************************************************************/
Expand Down
9 changes: 0 additions & 9 deletions src/interfaces/IExecutor.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ interface IExecutor is IAccessControl {
error InvalidActionsSetId();
error EmptyTargets();
error InconsistentParamsLength();
error DuplicateAction();
error InsufficientBalance();

/******************************************************************************************************************/
Expand Down Expand Up @@ -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 ***/
/******************************************************************************************************************/
Expand Down
46 changes: 0 additions & 46 deletions test/Executor.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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));

Expand All @@ -443,19 +410,16 @@ 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));
}

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));

Expand All @@ -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));
}
Expand All @@ -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));

Expand All @@ -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));
}
Expand All @@ -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));

Expand All @@ -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));
}
Expand Down Expand Up @@ -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));

Expand All @@ -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));
}
Expand Down

0 comments on commit 5ed105f

Please sign in to comment.