From 82e34fd6a40f209a7a2d32a950fb778be2276221 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Thu, 15 Jun 2023 21:29:41 -0300 Subject: [PATCH 01/19] first proposal --- contracts/governance/TimelockController.sol | 46 ++++++++----------- .../extensions/GovernorTimelockControl.sol | 10 +++- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index a25cd7b4a77..a543874ca83 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -169,28 +169,6 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { return getTimestamp(id) > 0; } - /** - * @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; - } - - /** - * @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; - } - - /** - * @dev Returns whether an operation is done or not. - */ - function isOperationDone(bytes32 id) public view virtual returns (bool) { - return getTimestamp(id) == _DONE_TIMESTAMP; - } - /** * @dev Returns the timestamp at which an operation becomes ready (0 for * unset operations, 1 for done operations). @@ -199,6 +177,22 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { return _timestamps[id]; } + /** + * @dev Returns operation state. + */ + function getOperationState(bytes32 id) public view virtual returns (OperationState) { + uint timestamp = getTimestamp(id); + if (timestamp == 0) { + return OperationState.Unset; + } else if (getTimestamp(id) > _DONE_TIMESTAMP && timestamp > block.timestamp) { + return OperationState.Pending; + } else if (timestamp > _DONE_TIMESTAMP) { + return OperationState.Ready; + } else { + return OperationState.Done; + } + } + /** * @dev Returns the minimum delay for an operation to become valid. * @@ -314,7 +308,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * - the caller must have the 'canceller' role. */ function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { - if (!isOperationPending(id)) { + if (getOperationState(id) != OperationState.Pending) { revert TimelockUnexpectedOperationState(id, OperationState.Pending); } delete _timestamps[id]; @@ -397,10 +391,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Checks before execution of an operation's calls. */ function _beforeCall(bytes32 id, bytes32 predecessor) private view { - if (!isOperationReady(id)) { + if (getOperationState(id) != OperationState.Ready) { revert TimelockUnexpectedOperationState(id, OperationState.Ready); } - if (predecessor != bytes32(0) && !isOperationDone(predecessor)) { + if (predecessor != bytes32(0) && getOperationState(predecessor) != OperationState.Done) { revert TimelockUnexecutedPredecessor(predecessor); } } @@ -409,7 +403,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Checks after execution of an operation's calls. */ function _afterCall(bytes32 id) private { - if (!isOperationReady(id)) { + if (getOperationState(id) != OperationState.Ready) { revert TimelockUnexpectedOperationState(id, OperationState.Ready); } _timestamps[id] = _DONE_TIMESTAMP; diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 888406ba7e7..f65b6fb7e0d 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -60,9 +60,15 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 queueid = _timelockIds[proposalId]; if (queueid == bytes32(0)) { return currentState; - } else if (_timelock.isOperationDone(queueid)) { + } + + TimelockController.OperationState _operationState = _timelock.getOperationState(queueid); + if (_operationState == TimelockController.OperationState.Done) { return ProposalState.Executed; - } else if (_timelock.isOperationPending(queueid)) { + } else if ( + _operationState == TimelockController.OperationState.Pending || + _operationState == TimelockController.OperationState.Ready + ) { return ProposalState.Queued; } else { return ProposalState.Canceled; From 536628e1da48060031669e16195d1f14a5d3c277 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Thu, 15 Jun 2023 21:34:08 -0300 Subject: [PATCH 02/19] Add changeset --- .changeset/loud-shrimps-play.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/loud-shrimps-play.md diff --git a/.changeset/loud-shrimps-play.md b/.changeset/loud-shrimps-play.md new file mode 100644 index 00000000000..96e34c657ed --- /dev/null +++ b/.changeset/loud-shrimps-play.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +Add state getter in TimelockController using OperationState enum From b2b6844f997bc5602320eee05a1d2cfc1c382d25 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Fri, 16 Jun 2023 07:37:40 -0300 Subject: [PATCH 03/19] Improove getOperationState decision tree --- contracts/governance/TimelockController.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index a543874ca83..68298c97f57 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -181,15 +181,15 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Returns operation state. */ function getOperationState(bytes32 id) public view virtual returns (OperationState) { - uint timestamp = getTimestamp(id); + uint256 timestamp = getTimestamp(id); if (timestamp == 0) { return OperationState.Unset; - } else if (getTimestamp(id) > _DONE_TIMESTAMP && timestamp > block.timestamp) { + } else if (timestamp == _DONE_TIMESTAMP) { + return OperationState.Done; + } else if (timestamp > block.timestamp) { return OperationState.Pending; - } else if (timestamp > _DONE_TIMESTAMP) { - return OperationState.Ready; } else { - return OperationState.Done; + return OperationState.Ready; } } From 832c2d58b7850fd02000a1d2474cf63206eaf84c Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Wed, 28 Jun 2023 20:02:05 -0300 Subject: [PATCH 04/19] Reinstate functions --- contracts/governance/README.adoc | 8 ++++++- contracts/governance/TimelockController.sol | 24 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 00edfe23d1b..37d6ff508fc 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -70,14 +70,20 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorTimelockCompound}} +{{IGovernorTimelock}} + {{GovernorSettings}} {{GovernorPreventLateQuorum}} {{GovernorCompatibilityBravo}} +{{IGovernorCompatibilityBravo}} + == Utils +{{IVotes}} + {{Votes}} == Timelock @@ -92,7 +98,7 @@ 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. +** *Pending:* An operation that has been scheduled but not executed or canceled. ** *Ready:* An operation that has been scheduled, after the timer expires. ** *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. diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 68298c97f57..10f6239fc77 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -166,7 +166,29 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * includes both Pending, Ready and Done operations. */ function isOperation(bytes32 id) public view virtual returns (bool) { - return getTimestamp(id) > 0; + 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) { + OperationState state = getOperationState(id); + return state == OperationState.Pending || 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) { + return getOperationState(id) == OperationState.Ready; + } + + /** + * @dev Returns whether an operation is done or not. + */ + function isOperationDone(bytes32 id) public view virtual returns (bool) { + return getOperationState(id) == OperationState.Done; } /** From e0bbaada41609aee5f6f930ea0b8c8ca7e58c471 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Thu, 29 Jun 2023 15:01:05 -0300 Subject: [PATCH 05/19] Change Pending to Blocked --- contracts/governance/README.adoc | 3 ++- contracts/governance/TimelockController.sol | 27 +++++++++++++-------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 37d6ff508fc..43f109e363a 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -98,8 +98,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 but not executed or canceled. +** *Blocked:* 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 blocked 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 10f6239fc77..bca9ba176e5 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -34,7 +34,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { enum OperationState { Unset, - Pending, + Blocked, Ready, Done } @@ -170,11 +170,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { } /** - * @dev Returns whether an operation is pending or not. Note that a "pending" operation may also be "ready". + * @dev Returns whether an operation is waitinf for expiration. Note that a "blocked" operation is also "pending". */ - function isOperationPending(bytes32 id) public view virtual returns (bool) { - OperationState state = getOperationState(id); - return state == OperationState.Pending || state == OperationState.Ready; + function isOperationBlocked(bytes32 id) public view virtual returns (bool) { + return getOperationState(id) == OperationState.Blocked; } /** @@ -184,6 +183,14 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { return getOperationState(id) == OperationState.Ready; } + /** + * @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) { + OperationState state = getOperationState(id); + return state == OperationState.Blocked || state == OperationState.Ready; + } + /** * @dev Returns whether an operation is done or not. */ @@ -209,7 +216,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { } else if (timestamp == _DONE_TIMESTAMP) { return OperationState.Done; } else if (timestamp > block.timestamp) { - return OperationState.Pending; + return OperationState.Blocked; } else { return OperationState.Ready; } @@ -330,8 +337,8 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * - the caller must have the 'canceller' role. */ function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { - if (getOperationState(id) != OperationState.Pending) { - revert TimelockUnexpectedOperationState(id, OperationState.Pending); + if (!isOperationPending(id)) { + revert TimelockUnexpectedOperationState(id, OperationState.Blocked); } delete _timestamps[id]; @@ -413,10 +420,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Checks before execution of an operation's calls. */ function _beforeCall(bytes32 id, bytes32 predecessor) private view { - if (getOperationState(id) != OperationState.Ready) { + if (!isOperationReady(id)) { revert TimelockUnexpectedOperationState(id, OperationState.Ready); } - if (predecessor != bytes32(0) && getOperationState(predecessor) != OperationState.Done) { + if (predecessor != bytes32(0) && !isOperationDone(predecessor)) { revert TimelockUnexecutedPredecessor(predecessor); } } From 8e671f997993fbc2d3631cdede174b0407e19686 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 1 Jul 2023 14:22:47 -0300 Subject: [PATCH 06/19] Change TimelockController error from state to bitmap --- contracts/governance/TimelockController.sol | 25 ++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index bca9ba176e5..8f4cf0af196 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -52,7 +52,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev The current state of an operation is not as required. */ - error TimelockUnexpectedOperationState(bytes32 operationId, OperationState expected); + error TimelockUnexpectedOperationState(bytes32 operationId, bytes32 expected); /** * @dev The predecessor to an operation not yet done. @@ -320,7 +320,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) { @@ -338,7 +338,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { if (!isOperationPending(id)) { - revert TimelockUnexpectedOperationState(id, OperationState.Blocked); + revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Blocked) | _encodeStateBitmap(OperationState.Ready)); } delete _timestamps[id]; @@ -421,7 +421,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); @@ -433,7 +433,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function _afterCall(bytes32 id) private { if (getOperationState(id) != OperationState.Ready) { - revert TimelockUnexpectedOperationState(id, OperationState.Ready); + revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready)); } _timestamps[id] = _DONE_TIMESTAMP; } @@ -455,4 +455,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 `ProposalState` enum. For example: + * + * 0x000...1000 + * ^^^^^^----- ... + * ^---- Done + * ^--- Ready + * ^-- Blocked + * ^- Unset + */ + function _encodeStateBitmap(OperationState proposalState) internal pure returns (bytes32) { + return bytes32(1 << uint8(proposalState)); + } } From de0fb956083c9300f5f34e34945c9061209e8022 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 1 Jul 2023 14:44:10 -0300 Subject: [PATCH 07/19] Fix tests --- test/governance/TimelockController.test.js | 23 ++++++++++--------- .../GovernorTimelockControl.test.js | 5 ++-- test/helpers/enums.js | 2 +- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index d8fcdce6ca7..fcc4c36a39f 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.Blocked, OperationState.Ready])], ); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 3265dfa6898..54dd597a723 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -19,6 +19,7 @@ const TOKENS = [ ]; contract('GovernorTimelockControl', function (accounts) { + const [owner, voter1, voter2, voter3, voter4, other] = accounts; const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; @@ -159,7 +160,7 @@ contract('GovernorTimelockControl', function (accounts) { await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [ this.proposal.timelockid, - Enums.OperationState.Ready, + proposalStatesToBitMap(Enums.OperationState.Ready), ]); }); @@ -174,7 +175,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..06872a8aa93 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', 'Blocked', 'Ready', 'Done'), }; From ef63f3750e4ee38d26ad21288d39fa570b197fa9 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Sat, 1 Jul 2023 15:22:32 -0300 Subject: [PATCH 08/19] Fix linter --- contracts/governance/TimelockController.sol | 7 +++++-- test/governance/extensions/GovernorTimelockControl.test.js | 1 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 8f4cf0af196..427e85e94b2 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -338,7 +338,10 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function cancel(bytes32 id) public virtual onlyRole(CANCELLER_ROLE) { if (!isOperationPending(id)) { - revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Blocked) | _encodeStateBitmap(OperationState.Ready)); + revert TimelockUnexpectedOperationState( + id, + _encodeStateBitmap(OperationState.Blocked) | _encodeStateBitmap(OperationState.Ready) + ); } delete _timestamps[id]; @@ -455,7 +458,7 @@ 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 `ProposalState` enum. For example: diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 54dd597a723..5954d4117f7 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -19,7 +19,6 @@ const TOKENS = [ ]; contract('GovernorTimelockControl', function (accounts) { - const [owner, voter1, voter2, voter3, voter4, other] = accounts; const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; From 9e8c8ca7914b6ed3372fa0f3a5c5107b7b01d203 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Jul 2023 11:57:48 -0300 Subject: [PATCH 09/19] Update loud-shrimps-play.md --- .changeset/loud-shrimps-play.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/loud-shrimps-play.md b/.changeset/loud-shrimps-play.md index 96e34c657ed..3de2da080dc 100644 --- a/.changeset/loud-shrimps-play.md +++ b/.changeset/loud-shrimps-play.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -Add state getter in TimelockController using OperationState enum +`TimelockController`: Add a state getter that returns an `OperationState` enum. From cf4a2dff8c673346a553b0e43443448d05d89642 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Jul 2023 11:58:56 -0300 Subject: [PATCH 10/19] rename error param --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 09e756af831..7a07770a208 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -53,7 +53,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev The current state of an operation is not as required. */ - error TimelockUnexpectedOperationState(bytes32 operationId, bytes32 expected); + error TimelockUnexpectedOperationState(bytes32 operationId, bytes32 expectedStates); /** * @dev The predecessor to an operation not yet done. From ab10a19a9a80cb148526d308b386a93fdf1a221e Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Jul 2023 12:02:31 -0300 Subject: [PATCH 11/19] remove isOperationBlocked --- contracts/governance/TimelockController.sol | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 7a07770a208..f7c89d4181c 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -171,10 +171,11 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { } /** - * @dev Returns whether an operation is waitinf for expiration. Note that a "blocked" operation is also "pending". + * @dev Returns whether an operation is pending or not. Note that a "pending" operation may also be "ready". */ - function isOperationBlocked(bytes32 id) public view virtual returns (bool) { - return getOperationState(id) == OperationState.Blocked; + function isOperationPending(bytes32 id) public view virtual returns (bool) { + OperationState state = getOperationState(id); + return state == OperationState.Blocked || state == OperationState.Ready; } /** @@ -184,14 +185,6 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { return getOperationState(id) == OperationState.Ready; } - /** - * @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) { - OperationState state = getOperationState(id); - return state == OperationState.Blocked || state == OperationState.Ready; - } - /** * @dev Returns whether an operation is done or not. */ From 643b5495e58f59029fa759c7823f1cd9b3b34fb3 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Jul 2023 12:04:53 -0300 Subject: [PATCH 12/19] remove virtual --- contracts/governance/TimelockController.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index f7c89d4181c..25c378b3149 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -166,14 +166,14 @@ 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) { + 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) { + function isOperationPending(bytes32 id) public view returns (bool) { OperationState state = getOperationState(id); return state == OperationState.Blocked || state == OperationState.Ready; } @@ -181,14 +181,14 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @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) { + 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) { + function isOperationDone(bytes32 id) public view returns (bool) { return getOperationState(id) == OperationState.Done; } From 86f1724383f3add79b2b0b6eeb2cbb266f1b4718 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Jul 2023 12:05:05 -0300 Subject: [PATCH 13/19] revert afterCall change --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 25c378b3149..9c2cf0ab45d 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -429,7 +429,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * @dev Checks after execution of an operation's calls. */ function _afterCall(bytes32 id) private { - if (getOperationState(id) != OperationState.Ready) { + if (!isOperationReady(id)) { revert TimelockUnexpectedOperationState(id, _encodeStateBitmap(OperationState.Ready)); } _timestamps[id] = _DONE_TIMESTAMP; From f9498a9b1a8cfb4b7315225ab55c733e6dc6dd40 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Jul 2023 12:10:55 -0300 Subject: [PATCH 14/19] rename Blocked to Waiting --- contracts/governance/TimelockController.sol | 10 +++++----- test/governance/TimelockController.test.js | 2 +- test/helpers/enums.js | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 9c2cf0ab45d..950babf25ac 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -35,7 +35,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { enum OperationState { Unset, - Blocked, + Waiting, Ready, Done } @@ -175,7 +175,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { */ function isOperationPending(bytes32 id) public view returns (bool) { OperationState state = getOperationState(id); - return state == OperationState.Blocked || state == OperationState.Ready; + return state == OperationState.Waiting || state == OperationState.Ready; } /** @@ -210,7 +210,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { } else if (timestamp == _DONE_TIMESTAMP) { return OperationState.Done; } else if (timestamp > block.timestamp) { - return OperationState.Blocked; + return OperationState.Waiting; } else { return OperationState.Ready; } @@ -334,7 +334,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { if (!isOperationPending(id)) { revert TimelockUnexpectedOperationState( id, - _encodeStateBitmap(OperationState.Blocked) | _encodeStateBitmap(OperationState.Ready) + _encodeStateBitmap(OperationState.Waiting) | _encodeStateBitmap(OperationState.Ready) ); } delete _timestamps[id]; @@ -461,7 +461,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * ^^^^^^----- ... * ^---- Done * ^--- Ready - * ^-- Blocked + * ^-- Waiting * ^- Unset */ function _encodeStateBitmap(OperationState proposalState) internal pure returns (bytes32) { diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index fcc4c36a39f..ce051e7870a 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -882,7 +882,7 @@ contract('TimelockController', function (accounts) { await expectRevertCustomError( this.mock.cancel(constants.ZERO_BYTES32, { from: canceller }), 'TimelockUnexpectedOperationState', - [constants.ZERO_BYTES32, proposalStatesToBitMap([OperationState.Blocked, OperationState.Ready])], + [constants.ZERO_BYTES32, proposalStatesToBitMap([OperationState.Waiting, OperationState.Ready])], ); }); diff --git a/test/helpers/enums.js b/test/helpers/enums.js index 06872a8aa93..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', 'Blocked', 'Ready', 'Done'), + OperationState: Enum('Unset', 'Waiting', 'Ready', 'Done'), }; From 220cfc30e78e5ab37e8db6f2176dce8bb5ed7a65 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Tue, 4 Jul 2023 12:32:53 -0300 Subject: [PATCH 15/19] Change enum in documentation --- contracts/governance/README.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 43f109e363a..936bc8dc84f 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -98,9 +98,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. -** *Blocked:* 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 blocked or ready. +** *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*: From bd74728f5630baf36622c4782d08f8c8b31d1f69 Mon Sep 17 00:00:00 2001 From: RenanSouza2 Date: Tue, 4 Jul 2023 13:35:33 -0300 Subject: [PATCH 16/19] Remove interfaces from docs --- contracts/governance/README.adoc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 936bc8dc84f..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 @@ -70,20 +68,14 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorTimelockCompound}} -{{IGovernorTimelock}} - {{GovernorSettings}} {{GovernorPreventLateQuorum}} {{GovernorCompatibilityBravo}} -{{IGovernorCompatibilityBravo}} - == Utils -{{IVotes}} - {{Votes}} == Timelock From a6083847a6c33833e8e554be4539f6fb68b9fcb1 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Jul 2023 14:11:41 -0300 Subject: [PATCH 17/19] Update contracts/governance/TimelockController.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/governance/TimelockController.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 9a6656d0be5..ffe054b8f27 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -465,7 +465,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { * ^-- Waiting * ^- Unset */ - function _encodeStateBitmap(OperationState proposalState) internal pure returns (bytes32) { - return bytes32(1 << uint8(proposalState)); + function _encodeStateBitmap(OperationState operationState) internal pure returns (bytes32) { + return bytes32(1 << uint8(operationState)); } } From bd2f0faaadb4a142ab7d22943ecb472911be120f Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Jul 2023 14:11:50 -0300 Subject: [PATCH 18/19] Update contracts/governance/TimelockController.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/governance/TimelockController.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index ffe054b8f27..c2c44f6014c 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -456,7 +456,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder { /** * @dev Encodes a `OperationState` into a `bytes32` representation where each bit enabled corresponds to - * the underlying position in the `ProposalState` enum. For example: + * the underlying position in the `OperationState` enum. For example: * * 0x000...1000 * ^^^^^^----- ... From 8d43f44772c1b74a106b6725f6db7b082354e8c8 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 4 Jul 2023 14:12:04 -0300 Subject: [PATCH 19/19] Update contracts/governance/TimelockController.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/governance/TimelockController.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index c2c44f6014c..ff8d4559583 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -52,6 +52,10 @@ 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, bytes32 expectedStates);