Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add state getter in TimelockController using OperationState enum #4358

Merged
merged 24 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
82e34fd
first proposal
RenanSouza2 Jun 16, 2023
536628e
Add changeset
RenanSouza2 Jun 16, 2023
a2a6a0c
Merge branch 'master' into timelockControler-getter
RenanSouza2 Jun 16, 2023
b2b6844
Improove getOperationState decision tree
RenanSouza2 Jun 16, 2023
813360d
Merge branch 'master' into timelockControler-getter
RenanSouza2 Jun 17, 2023
c3f0ac1
Merge branch 'master' into timelockControler-getter
RenanSouza2 Jun 28, 2023
832c2d5
Reinstate functions
RenanSouza2 Jun 28, 2023
e0bbaad
Change Pending to Blocked
RenanSouza2 Jun 29, 2023
8e671f9
Change TimelockController error from state to bitmap
RenanSouza2 Jul 1, 2023
de0fb95
Fix tests
RenanSouza2 Jul 1, 2023
ef63f37
Fix linter
RenanSouza2 Jul 1, 2023
7587ab4
Merge branch 'master' into timelockControler-getter
RenanSouza2 Jul 1, 2023
9e8c8ca
Update loud-shrimps-play.md
frangio Jul 4, 2023
cf4a2df
rename error param
frangio Jul 4, 2023
ab10a19
remove isOperationBlocked
frangio Jul 4, 2023
643b549
remove virtual
frangio Jul 4, 2023
86f1724
revert afterCall change
frangio Jul 4, 2023
f9498a9
rename Blocked to Waiting
frangio Jul 4, 2023
84099df
Merge branch 'master' into timelockControler-getter
frangio Jul 4, 2023
220cfc3
Change enum in documentation
RenanSouza2 Jul 4, 2023
bd74728
Remove interfaces from docs
RenanSouza2 Jul 4, 2023
a608384
Update contracts/governance/TimelockController.sol
frangio Jul 4, 2023
bd2f0fa
Update contracts/governance/TimelockController.sol
frangio Jul 4, 2023
8d43f44
Update contracts/governance/TimelockController.sol
frangio Jul 4, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/loud-shrimps-play.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`TimelockController`: Add a state getter that returns an `OperationState` enum.
5 changes: 2 additions & 3 deletions contracts/governance/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ NOTE: Functions of the `Governor` contract do not include access control. If you

=== Core

{{IGovernor}}

{{Governor}}

=== Modules
Expand Down Expand Up @@ -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*:
Expand Down
68 changes: 53 additions & 15 deletions contracts/governance/TimelockController.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {

enum OperationState {
Unset,
Pending,
Waiting,
Ready,
Done
}
Expand All @@ -52,8 +52,12 @@ contract TimelockController is AccessControl, ERC721Holder, ERC1155Holder {

/**
* @dev The current state of an operation is not as required.
frangio marked this conversation as resolved.
Show resolved Hide resolved
* 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.
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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.
*
Expand Down Expand Up @@ -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) {
Expand All @@ -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];

Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand All @@ -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));
}
}
23 changes: 12 additions & 11 deletions test/governance/TimelockController.test.js
Original file line number Diff line number Diff line change
@@ -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');

Expand Down Expand Up @@ -195,7 +196,7 @@ contract('TimelockController', function (accounts) {
{ from: proposer },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Unset],
[this.operation.id, proposalStatesToBitMap(OperationState.Unset)],
);
});

Expand Down Expand Up @@ -267,7 +268,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand Down Expand Up @@ -295,7 +296,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand All @@ -313,7 +314,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand Down Expand Up @@ -408,7 +409,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[reentrantOperation.id, OperationState.Ready],
[reentrantOperation.id, proposalStatesToBitMap(OperationState.Ready)],
);

// Disable reentrancy
Expand Down Expand Up @@ -505,7 +506,7 @@ contract('TimelockController', function (accounts) {
{ from: proposer },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Unset],
[this.operation.id, proposalStatesToBitMap(OperationState.Unset)],
);
});

Expand Down Expand Up @@ -596,7 +597,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand Down Expand Up @@ -624,7 +625,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand All @@ -642,7 +643,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[this.operation.id, OperationState.Ready],
[this.operation.id, proposalStatesToBitMap(OperationState.Ready)],
);
});

Expand Down Expand Up @@ -784,7 +785,7 @@ contract('TimelockController', function (accounts) {
{ from: executor },
),
'TimelockUnexpectedOperationState',
[reentrantBatchOperation.id, OperationState.Ready],
[reentrantBatchOperation.id, proposalStatesToBitMap(OperationState.Ready)],
);

// Disable reentrancy
Expand Down Expand Up @@ -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])],
);
});

Expand Down
4 changes: 2 additions & 2 deletions test/governance/extensions/GovernorTimelockControl.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ contract('GovernorTimelockControl', function (accounts) {

await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [
this.proposal.timelockid,
Enums.OperationState.Ready,
proposalStatesToBitMap(Enums.OperationState.Ready),
]);
});

Expand All @@ -174,7 +174,7 @@ contract('GovernorTimelockControl', function (accounts) {

await expectRevertCustomError(this.helper.execute(), 'TimelockUnexpectedOperationState', [
this.proposal.timelockid,
Enums.OperationState.Ready,
proposalStatesToBitMap(Enums.OperationState.Ready),
]);
});

Expand Down
2 changes: 1 addition & 1 deletion test/helpers/enums.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
};