diff --git a/.changeset/loud-shrimps-play.md b/.changeset/loud-shrimps-play.md new file mode 100644 index 00000000000..3de2da080dc --- /dev/null +++ b/.changeset/loud-shrimps-play.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`TimelockController`: Add a state getter that returns an `OperationState` enum. diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 00edfe23d1b..29c3887c789 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -52,8 +52,6 @@ NOTE: Functions of the `Governor` contract do not include access control. If you === Core -{{IGovernor}} - {{Governor}} === Modules @@ -92,8 +90,9 @@ In a governance system, the {TimelockController} contract is in charge of introd * *Operation:* A transaction (or a set of transactions) that is the subject of the timelock. It has to be scheduled by a proposer and executed by an executor. The timelock enforces a minimum delay between the proposition and the execution (see xref:access-control.adoc#operation_lifecycle[operation lifecycle]). If the operation contains multiple transactions (batch mode), they are executed atomically. Operations are identified by the hash of their content. * *Operation status:* ** *Unset:* An operation that is not part of the timelock mechanism. -** *Pending:* An operation that has been scheduled, before the timer expires. +** *Waiting:* An operation that has been scheduled, before the timer expires. ** *Ready:* An operation that has been scheduled, after the timer expires. +** *Pending:* An operation that is either waiting or ready. ** *Done:* An operation that has been executed. * *Predecessor*: An (optional) dependency between operations. An operation can depend on another operation (its predecessor), forcing the execution order of these two operations. * *Role*: diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 56a25fe3025..ff8d4559583 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -35,7 +35,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { enum OperationState { Unset, - Pending, + Waiting, Ready, Done } @@ -52,8 +52,12 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev The current state of an operation is not as required. + * The `expectedStates` is a bitmap with the bits enabled for each OperationState enum position + * counting from right to left. + * + * See {_encodeStateBitmap}. */ - error TimelockUnexpectedOperationState(bytes32 operationId, OperationState expected); + error TimelockUnexpectedOperationState(bytes32 operationId, bytes32 expectedStates); /** * @dev The predecessor to an operation not yet done. @@ -166,30 +170,30 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Returns whether an id correspond to a registered operation. This * includes both Pending, Ready and Done operations. */ - function isOperation(bytes32 id) public view virtual returns (bool) { - return getTimestamp(id) > 0; + function isOperation(bytes32 id) public view returns (bool) { + return getOperationState(id) != OperationState.Unset; } /** * @dev Returns whether an operation is pending or not. Note that a "pending" operation may also be "ready". */ - function isOperationPending(bytes32 id) public view virtual returns (bool) { - return getTimestamp(id) > _DONE_TIMESTAMP; + function isOperationPending(bytes32 id) public view returns (bool) { + OperationState state = getOperationState(id); + return state == OperationState.Waiting || state == OperationState.Ready; } /** * @dev Returns whether an operation is ready for execution. Note that a "ready" operation is also "pending". */ - function isOperationReady(bytes32 id) public view virtual returns (bool) { - uint256 timestamp = getTimestamp(id); - return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp; + function isOperationReady(bytes32 id) public view returns (bool) { + return getOperationState(id) == OperationState.Ready; } /** * @dev Returns whether an operation is done or not. */ - function isOperationDone(bytes32 id) public view virtual returns (bool) { - return getTimestamp(id) == _DONE_TIMESTAMP; + function isOperationDone(bytes32 id) public view returns (bool) { + return getOperationState(id) == OperationState.Done; } /** @@ -200,6 +204,22 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { return _timestamps[id]; } + /** + * @dev Returns operation state. + */ + function getOperationState(bytes32 id) public view virtual returns (OperationState) { + uint256 timestamp = getTimestamp(id); + if (timestamp == 0) { + return OperationState.Unset; + } else if (timestamp == _DONE_TIMESTAMP) { + return OperationState.Done; + } else if (timestamp > block.timestamp) { + return OperationState.Waiting; + } else { + return OperationState.Ready; + } + } + /** * @dev Returns the minimum delay for an operation to become valid. * @@ -298,7 +318,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function _schedule(bytes32 id, uint256 delay) private { if (isOperation(id)) { - revert TimelockUnexpectedOperationState(id, OperationState.Unset); + revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Unset)); } uint256 minDelay = getMinDelay(); if (delay < minDelay) { @@ -316,7 +336,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { if (!isOperationPending(id)) { - revert TimelockUnexpectedOperationState(id, OperationState.Pending); + revert TimelockUnexpectedOperationState( + id, + _encodeStateBitmap(OperationState.Waiting) | _encodeStateBitmap(OperationState.Ready) + ); } delete _timestamps[id]; @@ -399,7 +422,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function _beforeCall(bytes32 id, bytes32 predecessor) private view { if (!isOperationReady(id)) { - revert TimelockUnexpectedOperationState(id, OperationState.Ready); + revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready)); } if (predecessor != bytes32(0) && !isOperationDone(predecessor)) { revert TimelockUnexecutedPredecessor(predecessor); @@ -411,7 +434,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function _afterCall(bytes32 id) private { if (!isOperationReady(id)) { - revert TimelockUnexpectedOperationState(id, OperationState.Ready); + revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready)); } _timestamps[id] = _DONE_TIMESTAMP; } @@ -434,4 +457,19 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { emit MinDelayChange(_minDelay, newDelay); _minDelay = newDelay; } + + /** + * @dev Encodes a `OperationState` into a `bytes32` representation where each bit enabled corresponds to + * the underlying position in the `OperationState` enum. For example: + * + * 0x000...1000 + * ^^^^^^----- ... + * ^---- Done + * ^--- Ready + * ^-- Waiting + * ^- Unset + */ + function _encodeStateBitmap(OperationState operationState) internal pure returns (bytes32) { + return bytes32(1 << uint8(operationState)); + } } diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index d8fcdce6ca7..ce051e7870a 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -1,5 +1,6 @@ const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); const { ZERO_ADDRESS, ZERO_BYTES32 } = constants; +const { proposalStatesToBitMap } = require('../helpers/governance'); const { expect } = require('chai'); @@ -195,7 +196,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Unset], + [this.operation.id, proposalStatesToBitMap(OperationState.Unset)], ); }); @@ -267,7 +268,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -295,7 +296,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -313,7 +314,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -408,7 +409,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [reentrantOperation.id, OperationState.Ready], + [reentrantOperation.id, proposalStatesToBitMap(OperationState.Ready)], ); // Disable reentrancy @@ -505,7 +506,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Unset], + [this.operation.id, proposalStatesToBitMap(OperationState.Unset)], ); }); @@ -596,7 +597,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -624,7 +625,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -642,7 +643,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [this.operation.id, OperationState.Ready], + [this.operation.id, proposalStatesToBitMap(OperationState.Ready)], ); }); @@ -784,7 +785,7 @@ contract('TimelockController', function (accounts) { { from: executor }, ), 'TimelockUnexpectedOperationState', - [reentrantBatchOperation.id, OperationState.Ready], + [reentrantBatchOperation.id, proposalStatesToBitMap(OperationState.Ready)], ); // Disable reentrancy @@ -881,7 +882,7 @@ contract('TimelockController', function (accounts) { await expectRevertCustomError( this.mock.cancel(constants.ZERO_BYTES32, { from: canceller }), 'TimelockUnexpectedOperationState', - [constants.ZERO_BYTES32, OperationState.Pending], + [constants.ZERO_BYTES32, proposalStatesToBitMap([OperationState.Waiting, OperationState.Ready])], ); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 3265dfa6898..5954d4117f7 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -159,7 +159,7 @@ contract('GovernorTimelockControl', function (accounts) { await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [ this.proposal.timelockid, - Enums.OperationState.Ready, + proposalStatesToBitMap(Enums.OperationState.Ready), ]); }); @@ -174,7 +174,7 @@ contract('GovernorTimelockControl', function (accounts) { await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [ this.proposal.timelockid, - Enums.OperationState.Ready, + proposalStatesToBitMap(Enums.OperationState.Ready), ]); }); diff --git a/test/helpers/enums.js b/test/helpers/enums.js index d4a4fdbd0b7..75746e08744 100644 --- a/test/helpers/enums.js +++ b/test/helpers/enums.js @@ -7,5 +7,5 @@ module.exports = { ProposalState: Enum('Pending', 'Active', 'Canceled', 'Defeated', 'Succeeded', 'Queued', 'Expired', 'Executed'), VoteType: Enum('Against', 'For', 'Abstain'), Rounding: Enum('Down', 'Up', 'Zero'), - OperationState: Enum('Unset', 'Pending', 'Ready', 'Done'), + OperationState: Enum('Unset', 'Waiting', 'Ready', 'Done'), };