diff --git a/.changeset/brave-lobsters-punch.md b/.changeset/brave-lobsters-punch.md new file mode 100644 index 00000000000..60f04e4301b --- /dev/null +++ b/.changeset/brave-lobsters-punch.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Governor`: Refactored internals to implement common queuing logic in the core module of the Governor. Added `queue` and `_queueOperations` functions that act at different levels. Modules that implement queuing via timelocks are expected to override `_queueOperations` to implement the timelock-specific logic. Added `_executeOperations` as the equivalent for execution. diff --git a/.changeset/wild-beds-visit.md b/.changeset/wild-beds-visit.md new file mode 100644 index 00000000000..e97dee28413 --- /dev/null +++ b/.changeset/wild-beds-visit.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`GovernorStorage`: Added a new governor extension that stores the proposal details in storage, with an interface that operates on `proposalId`, as well as proposal enumerability. This replaces the old `GovernorCompatibilityBravo` module. diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 9e76bfea938..b9874c820f3 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -34,7 +34,6 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 "ExtendedBallot(uint256 proposalId,uint8 support,address voter,uint256 nonce,string reason,bytes params)" ); - // solhint-disable var-name-mixedcase struct ProposalCore { address proposer; uint48 voteStart; @@ -42,12 +41,21 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 bool executed; bool canceled; } - // solhint-enable var-name-mixedcase + + struct ProposalExtra { + uint48 eta; + } + + // Each object in this should fit into a single slot so it can be cached efficiently + struct ProposalFull { + ProposalCore core; + ProposalExtra extra; + } bytes32 private constant _ALL_PROPOSAL_STATES_BITMAP = bytes32((2 ** (uint8(type(ProposalState).max) + 1)) - 1); string private _name; - mapping(uint256 => ProposalCore) private _proposals; + mapping(uint256 => ProposalFull) private _proposals; // This queue keeps track of the governor operating on itself. Calls to functions protected by the // {onlyGovernance} modifier needs to be whitelisted in this queue. Whitelisting is set in {_beforeExecute}, @@ -90,27 +98,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { - bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector; - - bytes4 governorParamsId = this.castVoteWithReasonAndParams.selector ^ - this.castVoteWithReasonAndParamsBySig.selector ^ - this.getVotesWithParams.selector; - - // The original interface id in v4.3. - bytes4 governor43Id = type(IGovernor).interfaceId ^ - type(IERC6372).interfaceId ^ - governorCancelId ^ - governorParamsId; - - // An updated interface id in v4.6, with params added. - bytes4 governor46Id = type(IGovernor).interfaceId ^ type(IERC6372).interfaceId ^ governorCancelId; - - // For the updated interface id in v4.9, we use governorCancelId directly. - return - interfaceId == governor43Id || - interfaceId == governor46Id || - interfaceId == governorCancelId || + interfaceId == type(IGovernor).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); } @@ -157,13 +146,11 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 function state(uint256 proposalId) public view virtual override returns (ProposalState) { // ProposalCore is just one slot. We can load it from storage to memory with a single sload and use memory // object as a cache. This avoid duplicating expensive sloads. - ProposalCore memory proposal = _proposals[proposalId]; + ProposalCore memory core = _proposals[proposalId].core; - if (proposal.executed) { + if (core.executed) { return ProposalState.Executed; - } - - if (proposal.canceled) { + } else if (core.canceled) { return ProposalState.Canceled; } @@ -183,19 +170,19 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 if (deadline >= currentTimepoint) { return ProposalState.Active; - } - - if (_quorumReached(proposalId) && _voteSucceeded(proposalId)) { + } else if (!_quorumReached(proposalId) || !_voteSucceeded(proposalId)) { + return ProposalState.Defeated; + } else if (proposalEta(proposalId) == 0) { return ProposalState.Succeeded; } else { - return ProposalState.Defeated; + return ProposalState.Queued; } } /** - * @dev Part of the Governor Bravo's interface: _"The number of votes required in order for a voter to become a proposer"_. + * @dev See {IGovernor-proposalThreshold}. */ - function proposalThreshold() public view virtual returns (uint256) { + function proposalThreshold() public view virtual override returns (uint256) { return 0; } @@ -203,21 +190,28 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-proposalSnapshot}. */ function proposalSnapshot(uint256 proposalId) public view virtual override returns (uint256) { - return _proposals[proposalId].voteStart; + return _proposals[proposalId].core.voteStart; } /** * @dev See {IGovernor-proposalDeadline}. */ function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) { - return _proposals[proposalId].voteStart + _proposals[proposalId].voteDuration; + return _proposals[proposalId].core.voteStart + _proposals[proposalId].core.voteDuration; } /** - * @dev Returns the account that created a given proposal. + * @dev See {IGovernor-proposalProposer}. */ function proposalProposer(uint256 proposalId) public view virtual override returns (address) { - return _proposals[proposalId].proposer; + return _proposals[proposalId].core.proposer; + } + + /** + * @dev See {IGovernor-proposalEta}. + */ + function proposalEta(uint256 proposalId) public view virtual override returns (uint256) { + return _proposals[proposalId].extra.eta; } /** @@ -284,32 +278,47 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 string memory description ) public virtual override returns (uint256) { address proposer = _msgSender(); - require(_isValidDescriptionForProposer(proposer, description), "Governor: proposer restricted"); - uint256 currentTimepoint = clock(); + // check description restriction + if (!_isValidDescriptionForProposer(proposer, description)) { + revert GovernorRestrictedProposer(proposer); + } - // Avoid stack too deep - { - uint256 proposerVotes = getVotes(proposer, currentTimepoint - 1); - uint256 votesThreshold = proposalThreshold(); - if (proposerVotes < votesThreshold) { - revert GovernorInsufficientProposerVotes(proposer, proposerVotes, votesThreshold); - } + // check proposal threshold + uint256 proposerVotes = getVotes(proposer, clock() - 1); + uint256 votesThreshold = proposalThreshold(); + if (proposerVotes < votesThreshold) { + revert GovernorInsufficientProposerVotes(proposer, proposerVotes, votesThreshold); } + return _propose(targets, values, calldatas, description, proposer); + } + + /** + * @dev Internal propose mechanism. Can be overridden to add more logic on proposal creation. + * + * Emits a {IGovernor-ProposalCreated} event. + */ + function _propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description, + address proposer + ) internal virtual returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, keccak256(bytes(description))); if (targets.length != values.length || targets.length != calldatas.length || targets.length == 0) { revert GovernorInvalidProposalLength(targets.length, calldatas.length, values.length); } - if (_proposals[proposalId].voteStart != 0) { + if (_proposals[proposalId].core.voteStart != 0) { revert GovernorUnexpectedProposalState(proposalId, state(proposalId), bytes32(0)); } - uint256 snapshot = currentTimepoint + votingDelay(); + uint256 snapshot = clock() + votingDelay(); uint256 duration = votingPeriod(); - _proposals[proposalId] = ProposalCore({ + _proposals[proposalId].core = ProposalCore({ proposer: proposer, voteStart: SafeCast.toUint48(snapshot), voteDuration: SafeCast.toUint32(duration), @@ -333,59 +342,101 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } /** - * @dev See {IGovernor-execute}. + * @dev See {IGovernor-queue}. */ - function execute( + function queue( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public payable virtual override returns (uint256) { + ) public virtual override returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - ProposalState currentState = state(proposalId); - if (currentState != ProposalState.Succeeded && currentState != ProposalState.Queued) { - revert GovernorUnexpectedProposalState( - proposalId, - currentState, - _encodeStateBitmap(ProposalState.Succeeded) | _encodeStateBitmap(ProposalState.Queued) - ); - } - _proposals[proposalId].executed = true; + _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Succeeded)); - emit ProposalExecuted(proposalId); + uint48 eta = _queueOperations(proposalId, targets, values, calldatas, descriptionHash); - _beforeExecute(proposalId, targets, values, calldatas, descriptionHash); - _execute(proposalId, targets, values, calldatas, descriptionHash); - _afterExecute(proposalId, targets, values, calldatas, descriptionHash); + if (eta != 0) { + _proposals[proposalId].extra.eta = eta; + emit ProposalQueued(proposalId, eta); + } else { + revert GovernorQueueNotImplemented(); + } return proposalId; } /** - * @dev See {IGovernor-cancel}. + * @dev Internal queuing mechanism. Can be overridden (without a super call) to modify the way queuing is + * performed (for example adding a vault/timelock). + * + * This is empty by default, and must be overridden to implement queuing. + * + * This function returns a timestamp that describes the expected eta for execution. If the returned value is 0 + * (which is the default value), the core will consider queueing did not succeed, and the public {queue} function + * will revert. + * + * NOTE: Calling this function directly will NOT check the current state of the proposal, or emit the + * `ProposalQueued` event. Queuing a proposal should be done using {queue} or {_queue}. + */ + function _queueOperations( + uint256 /*proposalId*/, + address[] memory /*targets*/, + uint256[] memory /*values*/, + bytes[] memory /*calldatas*/, + bytes32 /*descriptionHash*/ + ) internal virtual returns (uint48) { + return 0; + } + + /** + * @dev See {IGovernor-execute}. */ - function cancel( + function execute( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public virtual override returns (uint256) { + ) public payable virtual override returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - ProposalState currentState = state(proposalId); - if (currentState != ProposalState.Pending) { - revert GovernorUnexpectedProposalState(proposalId, currentState, _encodeStateBitmap(ProposalState.Pending)); + + _validateStateBitmap( + proposalId, + _encodeStateBitmap(ProposalState.Succeeded) | _encodeStateBitmap(ProposalState.Queued) + ); + + // mark as executed before calls to avoid reentrancy + _proposals[proposalId].core.executed = true; + + // before execute: register governance call in queue. + if (_executor() != address(this)) { + for (uint256 i = 0; i < targets.length; ++i) { + if (targets[i] == address(this)) { + _governanceCall.pushBack(keccak256(calldatas[i])); + } + } } - if (_msgSender() != proposalProposer(proposalId)) { - revert GovernorOnlyProposer(_msgSender()); + + _executeOperations(proposalId, targets, values, calldatas, descriptionHash); + + // after execute: cleanup governance call queue. + if (_executor() != address(this) && !_governanceCall.empty()) { + _governanceCall.clear(); } - return _cancel(targets, values, calldatas, descriptionHash); + + emit ProposalExecuted(proposalId); + + return proposalId; } /** - * @dev Internal execution mechanism. Can be overridden to implement different execution mechanism + * @dev Internal execution mechanism. Can be overridden (without a super call) to modify the way execution is + * performed (for example adding a vault/timelock). + * + * NOTE: Calling this function directly will NOT check the current state of the proposal, set the executed flag to + * true or emit the `ProposalExecuted` event. Executing a proposal should be done using {execute} or {_execute}. */ - function _execute( + function _executeOperations( uint256 /* proposalId */, address[] memory targets, uint256[] memory values, @@ -399,44 +450,31 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } /** - * @dev Hook before execution is triggered. + * @dev See {IGovernor-cancel}. */ - function _beforeExecute( - uint256 /* proposalId */, + function cancel( address[] memory targets, - uint256[] memory /* values */, + uint256[] memory values, bytes[] memory calldatas, - bytes32 /*descriptionHash*/ - ) internal virtual { - if (_executor() != address(this)) { - for (uint256 i = 0; i < targets.length; ++i) { - if (targets[i] == address(this)) { - _governanceCall.pushBack(keccak256(calldatas[i])); - } - } - } - } + bytes32 descriptionHash + ) public virtual override returns (uint256) { + // The proposalId will be recomputed in the `_cancel` call further down. However we need the value before we + // do the internal call, because we need to check the proposal state BEFORE the internal `_cancel` call + // changes it. The `hashProposal` duplication has a cost that is limited, and that we accept. + uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - /** - * @dev Hook after execution is triggered. - */ - function _afterExecute( - uint256 /* proposalId */, - address[] memory /* targets */, - uint256[] memory /* values */, - bytes[] memory /* calldatas */, - bytes32 /*descriptionHash*/ - ) internal virtual { - if (_executor() != address(this)) { - if (!_governanceCall.empty()) { - _governanceCall.clear(); - } + // public cancel restrictions (on top of existing _cancel restrictions). + _validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Pending)); + if (_msgSender() != proposalProposer(proposalId)) { + revert GovernorOnlyProposer(_msgSender()); } + + return _cancel(targets, values, calldatas, descriptionHash); } /** - * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as - * canceled to allow distinguishing it from executed proposals. + * @dev Internal cancel mechanism with minimal restrictions. A proposal can be cancelled in any state other than + * Canceled, Expired, or Executed. Once cancelled a proposal can't be re-submitted. * * Emits a {IGovernor-ProposalCanceled} event. */ @@ -448,20 +486,15 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 ) internal virtual returns (uint256) { uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - ProposalState currentState = state(proposalId); - - bytes32 forbiddenStates = _encodeStateBitmap(ProposalState.Canceled) | - _encodeStateBitmap(ProposalState.Expired) | - _encodeStateBitmap(ProposalState.Executed); - if (forbiddenStates & _encodeStateBitmap(currentState) != 0) { - revert GovernorUnexpectedProposalState( - proposalId, - currentState, - _ALL_PROPOSAL_STATES_BITMAP ^ forbiddenStates - ); - } - _proposals[proposalId].canceled = true; + _validateStateBitmap( + proposalId, + _ALL_PROPOSAL_STATES_BITMAP ^ + _encodeStateBitmap(ProposalState.Canceled) ^ + _encodeStateBitmap(ProposalState.Expired) ^ + _encodeStateBitmap(ProposalState.Executed) + ); + _proposals[proposalId].core.canceled = true; emit ProposalCanceled(proposalId); return proposalId; @@ -695,6 +728,20 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 return bytes32(1 << uint8(proposalState)); } + /** + * @dev Check that the current state of a proposal matches the requirements described by the `allowedStates` bitmap. + * This bitmap should be built using `_encodeStateBitmap`. + * + * If requirements are not met, reverts with a {GovernorUnexpectedProposalState} error. + */ + function _validateStateBitmap(uint256 proposalId, bytes32 allowedStates) private view returns (ProposalState) { + ProposalState currentState = state(proposalId); + if (_encodeStateBitmap(currentState) & allowedStates == bytes32(0)) { + revert GovernorUnexpectedProposalState(proposalId, currentState, allowedStates); + } + return currentState; + } + /* * @dev Check if the proposer is authorized to submit a proposal with the given description. * @@ -779,4 +826,30 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } } } + + /** + * @inheritdoc IERC6372 + */ + function clock() public view virtual returns (uint48); + + /** + * @inheritdoc IERC6372 + */ + // solhint-disable-next-line func-name-mixedcase + function CLOCK_MODE() public view virtual returns (string memory); + + /** + * @inheritdoc IGovernor + */ + function votingDelay() public view virtual returns (uint256); + + /** + * @inheritdoc IGovernor + */ + function votingPeriod() public view virtual returns (uint256); + + /** + * @inheritdoc IGovernor + */ + function quorum(uint256 timepoint) public view virtual returns (uint256); } diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index c4a28fd1947..3b31bc502f8 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -9,7 +9,7 @@ import {IERC6372} from "../interfaces/IERC6372.sol"; /** * @dev Interface of the {Governor} core. */ -abstract contract IGovernor is IERC165, IERC6372 { +interface IGovernor is IERC165, IERC6372 { enum ProposalState { Pending, Active, @@ -69,15 +69,35 @@ abstract contract IGovernor is IERC165, IERC6372 { error GovernorInvalidVotingPeriod(uint256 votingPeriod); /** - * @dev The `proposer` does not have the required votes to operate on a proposal. + * @dev The `proposer` does not have the required votes to create a proposal. */ error GovernorInsufficientProposerVotes(address proposer, uint256 votes, uint256 threshold); + /** + * @dev The `proposer` is not allowed to create a proposal. + */ + error GovernorRestrictedProposer(address proposer); + /** * @dev The vote type used is not valid for the corresponding counting module. */ error GovernorInvalidVoteType(); + /** + * @dev Queue operation is not implemented for this governor. Execute should be called directly. + */ + error GovernorQueueNotImplemented(); + + /** + * @dev The proposal hasn't been queued yet. + */ + error GovernorNotQueuedProposal(uint256 proposalId); + + /** + * @dev The proposal has already been queued. + */ + error GovernorAlreadyQueuedProposal(uint256 proposalId); + /** * @dev The provided signature is not valid for the expected `voter`. * If the `voter` is a contract, the signature is not valid using {IERC1271-isValidSignature}. @@ -100,15 +120,20 @@ abstract contract IGovernor is IERC165, IERC6372 { ); /** - * @dev Emitted when a proposal is canceled. + * @dev Emitted when a proposal is queued. */ - event ProposalCanceled(uint256 proposalId); + event ProposalQueued(uint256 proposalId, uint256 eta); /** * @dev Emitted when a proposal is executed. */ event ProposalExecuted(uint256 proposalId); + /** + * @dev Emitted when a proposal is canceled. + */ + event ProposalCanceled(uint256 proposalId); + /** * @dev Emitted when a vote is cast without params. * @@ -135,26 +160,13 @@ abstract contract IGovernor is IERC165, IERC6372 { * @notice module:core * @dev Name of the governor instance (used in building the ERC712 domain separator). */ - function name() public view virtual returns (string memory); + function name() external view returns (string memory); /** * @notice module:core * @dev Version of the governor instance (used in building the ERC712 domain separator). Default: "1" */ - function version() public view virtual returns (string memory); - - /** - * @notice module:core - * @dev See {IERC6372} - */ - function clock() public view virtual returns (uint48); - - /** - * @notice module:core - * @dev See EIP-6372. - */ - // solhint-disable-next-line func-name-mixedcase - function CLOCK_MODE() public view virtual returns (string memory); + function version() external view returns (string memory); /** * @notice module:voting @@ -179,7 +191,7 @@ abstract contract IGovernor is IERC165, IERC6372 { * JavaScript class. */ // solhint-disable-next-line func-name-mixedcase - function COUNTING_MODE() public view virtual returns (string memory); + function COUNTING_MODE() external view returns (string memory); /** * @notice module:core @@ -190,13 +202,19 @@ abstract contract IGovernor is IERC165, IERC6372 { uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public pure virtual returns (uint256); + ) external pure returns (uint256); /** * @notice module:core * @dev Current state of a proposal, following Compound's convention */ - function state(uint256 proposalId) public view virtual returns (ProposalState); + function state(uint256 proposalId) external view returns (ProposalState); + + /** + * @notice module:core + * @dev The number of votes required in order for a voter to become a proposer. + */ + function proposalThreshold() external view returns (uint256); /** * @notice module:core @@ -204,20 +222,28 @@ abstract contract IGovernor is IERC165, IERC6372 { * snapshot is performed at the end of this block. Hence, voting for this proposal starts at the beginning of the * following block. */ - function proposalSnapshot(uint256 proposalId) public view virtual returns (uint256); + function proposalSnapshot(uint256 proposalId) external view returns (uint256); /** * @notice module:core * @dev Timepoint at which votes close. If using block number, votes close at the end of this block, so it is * possible to cast a vote during this block. */ - function proposalDeadline(uint256 proposalId) public view virtual returns (uint256); + function proposalDeadline(uint256 proposalId) external view returns (uint256); /** * @notice module:core * @dev The account that created a proposal. */ - function proposalProposer(uint256 proposalId) public view virtual returns (address); + function proposalProposer(uint256 proposalId) external view returns (address); + + /** + * @notice module:core + * @dev The time when a queued proposal becomes executable ("ETA"). Unlike {proposalSnapshot} and + * {proposalDeadline}, this doesn't use the governor clock, and instead relies on the executor's clock which may be + * different. In most cases this will be a timestamp. + */ + function proposalEta(uint256 proposalId) external view returns (uint256); /** * @notice module:user-config @@ -230,7 +256,7 @@ abstract contract IGovernor is IERC165, IERC6372 { * NOTE: While this interface returns a uint256, timepoints are stored as uint48 following the ERC-6372 clock type. * Consequently this value must fit in a uint48 (when added to the current clock). See {IERC6372-clock}. */ - function votingDelay() public view virtual returns (uint256); + function votingDelay() external view returns (uint256); /** * @notice module:user-config @@ -244,7 +270,7 @@ abstract contract IGovernor is IERC165, IERC6372 { * proposals that have already been submitted. The type used to save it is a uint32. Consequently, while this * interface returns a uint256, the value it returns should fit in a uint32. */ - function votingPeriod() public view virtual returns (uint256); + function votingPeriod() external view returns (uint256); /** * @notice module:user-config @@ -253,7 +279,7 @@ abstract contract IGovernor is IERC165, IERC6372 { * NOTE: The `timepoint` parameter corresponds to the snapshot used for counting vote. This allows to scale the * quorum depending on values such as the totalSupply of a token at this timepoint (see {ERC20Votes}). */ - function quorum(uint256 timepoint) public view virtual returns (uint256); + function quorum(uint256 timepoint) external view returns (uint256); /** * @notice module:reputation @@ -262,7 +288,7 @@ abstract contract IGovernor is IERC165, IERC6372 { * Note: this can be implemented in a number of ways, for example by reading the delegated balance from one (or * multiple), {ERC20Votes} tokens. */ - function getVotes(address account, uint256 timepoint) public view virtual returns (uint256); + function getVotes(address account, uint256 timepoint) external view returns (uint256); /** * @notice module:reputation @@ -272,13 +298,13 @@ abstract contract IGovernor is IERC165, IERC6372 { address account, uint256 timepoint, bytes memory params - ) public view virtual returns (uint256); + ) external view returns (uint256); /** * @notice module:voting * @dev Returns whether `account` has cast a vote on `proposalId`. */ - function hasVoted(uint256 proposalId, address account) public view virtual returns (bool); + function hasVoted(uint256 proposalId, address account) external view returns (bool); /** * @dev Create a new proposal. Vote start after a delay specified by {IGovernor-votingDelay} and lasts for a @@ -291,22 +317,37 @@ abstract contract IGovernor is IERC165, IERC6372 { uint256[] memory values, bytes[] memory calldatas, string memory description - ) public virtual returns (uint256 proposalId); + ) external returns (uint256 proposalId); + + /** + * @dev Queue a proposal. Some governors require this step to be performed before execution can happen. If queuing + * is not necessary, this function may revert. + * Queuing a proposal requires the quorum to be reached, the vote to be successful, and the deadline to be reached. + * + * Emits a {ProposalQueued} event. + */ + function queue( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) external returns (uint256 proposalId); /** * @dev Execute a successful proposal. This requires the quorum to be reached, the vote to be successful, and the - * deadline to be reached. + * deadline to be reached. Depending on the governor it might also be required that the proposal was queued and + * that some delay passed. * * Emits a {ProposalExecuted} event. * - * Note: some module can modify the requirements for execution, for example by adding an additional timelock. + * NOTE: Some modules can modify the requirements for execution, for example by adding an additional timelock. */ function execute( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public payable virtual returns (uint256 proposalId); + ) external payable returns (uint256 proposalId); /** * @dev Cancel a proposal. A proposal is cancellable by the proposer, but only while it is Pending state, i.e. @@ -319,14 +360,14 @@ abstract contract IGovernor is IERC165, IERC6372 { uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public virtual returns (uint256 proposalId); + ) external returns (uint256 proposalId); /** * @dev Cast a vote * * Emits a {VoteCast} event. */ - function castVote(uint256 proposalId, uint8 support) public virtual returns (uint256 balance); + function castVote(uint256 proposalId, uint8 support) external returns (uint256 balance); /** * @dev Cast a vote with a reason @@ -337,7 +378,7 @@ abstract contract IGovernor is IERC165, IERC6372 { uint256 proposalId, uint8 support, string calldata reason - ) public virtual returns (uint256 balance); + ) external returns (uint256 balance); /** * @dev Cast a vote with a reason and additional encoded parameters @@ -349,7 +390,7 @@ abstract contract IGovernor is IERC165, IERC6372 { uint8 support, string calldata reason, bytes memory params - ) public virtual returns (uint256 balance); + ) external returns (uint256 balance); /** * @dev Cast a vote using the voter's signature, including ERC-1271 signature support. @@ -361,7 +402,7 @@ abstract contract IGovernor is IERC165, IERC6372 { uint8 support, address voter, bytes memory signature - ) public virtual returns (uint256 balance); + ) external returns (uint256 balance); /** * @dev Cast a vote with a reason and additional encoded parameters using the voter's signature, @@ -376,5 +417,5 @@ abstract contract IGovernor is IERC165, IERC6372 { string calldata reason, bytes memory params, bytes memory signature - ) public virtual returns (uint256 balance); + ) external returns (uint256 balance); } diff --git a/contracts/governance/README.adoc b/contracts/governance/README.adoc index 35f324b7ede..5b38c4d53f8 100644 --- a/contracts/governance/README.adoc +++ b/contracts/governance/README.adoc @@ -36,7 +36,7 @@ Timelock extensions add a delay for governance decisions to be executed. The wor Other extensions can customize the behavior or interface in multiple ways. -* {GovernorCompatibilityBravo}: Extends the interface to be fully `GovernorBravo`-compatible. Note that events are compatible regardless of whether this extension is included or not. +* {GovernorStorage}: Stores the proposal details onchain and provides enumerability of the proposals. This can be useful for some L2 chains where storage is cheap compared to calldata. * {GovernorSettings}: Manages some of the settings (voting delay, voting period duration, and proposal threshold) in a way that can be updated through a governance proposal, without requiring an upgrade. @@ -74,7 +74,7 @@ NOTE: Functions of the `Governor` contract do not include access control. If you {{GovernorPreventLateQuorum}} -{{GovernorCompatibilityBravo}} +{{GovernorStorage}} == Utils diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol deleted file mode 100644 index 27c34de297f..00000000000 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ /dev/null @@ -1,333 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (governance/compatibility/GovernorCompatibilityBravo.sol) - -pragma solidity ^0.8.20; - -import {SafeCast} from "../../utils/math/SafeCast.sol"; -import {IGovernorTimelock} from "../extensions/IGovernorTimelock.sol"; -import {IGovernor, Governor} from "../Governor.sol"; -import {IGovernorCompatibilityBravo} from "./IGovernorCompatibilityBravo.sol"; - -/** - * @dev Compatibility layer that implements GovernorBravo compatibility on top of {Governor}. - * - * This compatibility layer includes a voting system and requires a {IGovernorTimelock} compatible module to be added - * through inheritance. It does not include token bindings, nor does it include any variable upgrade patterns. - * - * NOTE: When using this module, you may need to enable the Solidity optimizer to avoid hitting the contract size limit. - */ -abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorCompatibilityBravo, Governor { - enum VoteType { - Against, - For, - Abstain - } - - struct ProposalDetails { - address[] targets; - uint256[] values; - string[] signatures; - bytes[] calldatas; - uint256 forVotes; - uint256 againstVotes; - uint256 abstainVotes; - mapping(address => Receipt) receipts; - bytes32 descriptionHash; - } - - mapping(uint256 => ProposalDetails) private _proposalDetails; - - // solhint-disable-next-line func-name-mixedcase - function COUNTING_MODE() public pure virtual override returns (string memory) { - return "support=bravo&quorum=bravo"; - } - - // ============================================== Proposal lifecycle ============================================== - /** - * @dev See {IGovernor-propose}. - */ - function propose( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - string memory description - ) public virtual override(IGovernor, Governor) returns (uint256) { - // Stores the proposal details (if not already present) and executes the propose logic from the core. - _storeProposal(targets, values, new string[](calldatas.length), calldatas, description); - return super.propose(targets, values, calldatas, description); - } - - /** - * @dev See {IGovernorCompatibilityBravo-propose}. - */ - function propose( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - string memory description - ) public virtual override returns (uint256) { - if (signatures.length != calldatas.length) { - revert GovernorInvalidSignaturesLength(signatures.length, calldatas.length); - } - // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done - // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we - // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code - // is added there is also executed when calling this alternative interface. - _storeProposal(targets, values, signatures, calldatas, description); - return propose(targets, values, _encodeCalldata(signatures, calldatas), description); - } - - /** - * @dev See {IGovernorCompatibilityBravo-queue}. - */ - function queue(uint256 proposalId) public virtual override { - ( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) = _getProposalParameters(proposalId); - - queue(targets, values, calldatas, descriptionHash); - } - - /** - * @dev See {IGovernorCompatibilityBravo-execute}. - */ - function execute(uint256 proposalId) public payable virtual override { - ( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) = _getProposalParameters(proposalId); - - execute(targets, values, calldatas, descriptionHash); - } - - /** - * @dev Cancel a proposal with GovernorBravo logic. - */ - function cancel(uint256 proposalId) public virtual override { - ( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) = _getProposalParameters(proposalId); - - cancel(targets, values, calldatas, descriptionHash); - } - - /** - * @dev Cancel a proposal with GovernorBravo logic. At any moment a proposal can be cancelled, either by the - * proposer, or by third parties if the proposer's voting power has dropped below the proposal threshold. - */ - function cancel( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) public virtual override(IGovernor, Governor) returns (uint256) { - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - address proposer = proposalProposer(proposalId); - - uint256 proposerVotes = getVotes(proposer, clock() - 1); - uint256 votesThreshold = proposalThreshold(); - if (_msgSender() != proposer && proposerVotes >= votesThreshold) { - revert GovernorInsufficientProposerVotes(proposer, proposerVotes, votesThreshold); - } - - return _cancel(targets, values, calldatas, descriptionHash); - } - - /** - * @dev Encodes calldatas with optional function signature. - */ - function _encodeCalldata( - string[] memory signatures, - bytes[] memory calldatas - ) private pure returns (bytes[] memory) { - bytes[] memory fullcalldatas = new bytes[](calldatas.length); - for (uint256 i = 0; i < fullcalldatas.length; ++i) { - fullcalldatas[i] = bytes(signatures[i]).length == 0 - ? calldatas[i] - : bytes.concat(abi.encodeWithSignature(signatures[i]), calldatas[i]); - } - - return fullcalldatas; - } - - /** - * @dev Retrieve proposal parameters by id, with fully encoded calldatas. - */ - function _getProposalParameters( - uint256 proposalId - ) - private - view - returns (address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash) - { - ProposalDetails storage details = _proposalDetails[proposalId]; - return ( - details.targets, - details.values, - _encodeCalldata(details.signatures, details.calldatas), - details.descriptionHash - ); - } - - /** - * @dev Store proposal metadata (if not already present) for later lookup. - */ - function _storeProposal( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - string memory description - ) private { - bytes32 descriptionHash = keccak256(bytes(description)); - uint256 proposalId = hashProposal(targets, values, _encodeCalldata(signatures, calldatas), descriptionHash); - - ProposalDetails storage details = _proposalDetails[proposalId]; - if (details.descriptionHash == bytes32(0)) { - details.targets = targets; - details.values = values; - details.signatures = signatures; - details.calldatas = calldatas; - details.descriptionHash = descriptionHash; - } - } - - // ==================================================== Views ===================================================== - /** - * @dev See {IGovernorCompatibilityBravo-proposals}. - */ - function proposals( - uint256 proposalId - ) - public - view - virtual - override - returns ( - uint256 id, - address proposer, - uint256 eta, - uint256 startBlock, - uint256 endBlock, - uint256 forVotes, - uint256 againstVotes, - uint256 abstainVotes, - bool canceled, - bool executed - ) - { - id = proposalId; - proposer = proposalProposer(proposalId); - eta = proposalEta(proposalId); - startBlock = proposalSnapshot(proposalId); - endBlock = proposalDeadline(proposalId); - - ProposalDetails storage details = _proposalDetails[proposalId]; - forVotes = details.forVotes; - againstVotes = details.againstVotes; - abstainVotes = details.abstainVotes; - - ProposalState currentState = state(proposalId); - canceled = currentState == ProposalState.Canceled; - executed = currentState == ProposalState.Executed; - } - - /** - * @dev See {IGovernorCompatibilityBravo-getActions}. - */ - function getActions( - uint256 proposalId - ) - public - view - virtual - override - returns ( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas - ) - { - ProposalDetails storage details = _proposalDetails[proposalId]; - return (details.targets, details.values, details.signatures, details.calldatas); - } - - /** - * @dev See {IGovernorCompatibilityBravo-getReceipt}. - */ - function getReceipt(uint256 proposalId, address voter) public view virtual override returns (Receipt memory) { - return _proposalDetails[proposalId].receipts[voter]; - } - - /** - * @dev See {IGovernorCompatibilityBravo-quorumVotes}. - */ - function quorumVotes() public view virtual override returns (uint256) { - return quorum(clock() - 1); - } - - // ==================================================== Voting ==================================================== - /** - * @dev See {IGovernor-hasVoted}. - */ - function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { - return _proposalDetails[proposalId].receipts[account].hasVoted; - } - - /** - * @dev See {Governor-_quorumReached}. In this module, only forVotes count toward the quorum. - */ - function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) { - ProposalDetails storage details = _proposalDetails[proposalId]; - return quorum(proposalSnapshot(proposalId)) <= details.forVotes; - } - - /** - * @dev See {Governor-_voteSucceeded}. In this module, the forVotes must be strictly over the againstVotes. - */ - function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) { - ProposalDetails storage details = _proposalDetails[proposalId]; - return details.forVotes > details.againstVotes; - } - - /** - * @dev See {Governor-_countVote}. In this module, the support follows Governor Bravo. - */ - function _countVote( - uint256 proposalId, - address account, - uint8 support, - uint256 weight, - bytes memory // params - ) internal virtual override { - ProposalDetails storage details = _proposalDetails[proposalId]; - Receipt storage receipt = details.receipts[account]; - - if (receipt.hasVoted) { - revert GovernorAlreadyCastVote(account); - } - receipt.hasVoted = true; - receipt.support = support; - receipt.votes = SafeCast.toUint96(weight); - - if (support == uint8(VoteType.Against)) { - details.againstVotes += weight; - } else if (support == uint8(VoteType.For)) { - details.forVotes += weight; - } else if (support == uint8(VoteType.Abstain)) { - details.abstainVotes += weight; - } else { - revert GovernorInvalidVoteType(); - } - } -} diff --git a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol deleted file mode 100644 index 26fdb31eaa7..00000000000 --- a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol +++ /dev/null @@ -1,121 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (governance/compatibility/IGovernorCompatibilityBravo.sol) - -pragma solidity ^0.8.20; - -import {IGovernor} from "../IGovernor.sol"; - -/** - * @dev Interface extension that adds missing functions to the {Governor} core to provide `GovernorBravo` compatibility. - */ -abstract contract IGovernorCompatibilityBravo is IGovernor { - /** - * @dev Mismatch between the parameters length for a proposal call. - */ - error GovernorInvalidSignaturesLength(uint256 signatures, uint256 calldatas); - - /** - * @dev Proposal structure from Compound Governor Bravo. Not actually used by the compatibility layer, as - * {{proposal}} returns a very different structure. - */ - struct Proposal { - uint256 id; - address proposer; - uint256 eta; - address[] targets; - uint256[] values; - string[] signatures; - bytes[] calldatas; - uint256 startBlock; - uint256 endBlock; - uint256 forVotes; - uint256 againstVotes; - uint256 abstainVotes; - bool canceled; - bool executed; - mapping(address => Receipt) receipts; - } - - /** - * @dev Receipt structure from Compound Governor Bravo - */ - struct Receipt { - bool hasVoted; - uint8 support; - uint96 votes; - } - - /** - * @dev Part of the Governor Bravo's interface. - */ - function quorumVotes() public view virtual returns (uint256); - - /** - * @dev Part of the Governor Bravo's interface: _"The official record of all proposals ever proposed"_. - */ - function proposals( - uint256 - ) - public - view - virtual - returns ( - uint256 id, - address proposer, - uint256 eta, - uint256 startBlock, - uint256 endBlock, - uint256 forVotes, - uint256 againstVotes, - uint256 abstainVotes, - bool canceled, - bool executed - ); - - /** - * @dev Part of the Governor Bravo's interface: _"Function used to propose a new proposal"_. - */ - function propose( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas, - string memory description - ) public virtual returns (uint256); - - /** - * @dev Part of the Governor Bravo's interface: _"Queues a proposal of state succeeded"_. - */ - function queue(uint256 proposalId) public virtual; - - /** - * @dev Part of the Governor Bravo's interface: _"Executes a queued proposal if eta has passed"_. - */ - function execute(uint256 proposalId) public payable virtual; - - /** - * @dev Cancels a proposal only if the sender is the proposer or the proposer delegates' voting power dropped below the proposal threshold. - */ - function cancel(uint256 proposalId) public virtual; - - /** - * @dev Part of the Governor Bravo's interface: _"Gets actions of a proposal"_. - */ - function getActions( - uint256 proposalId - ) - public - view - virtual - returns ( - address[] memory targets, - uint256[] memory values, - string[] memory signatures, - bytes[] memory calldatas - ); - - /** - * @dev Part of the Governor Bravo's interface: _"Gets the receipt for a voter on a given proposal"_. - */ - function getReceipt(uint256 proposalId, address voter) public view virtual returns (Receipt memory); -} diff --git a/contracts/governance/extensions/GovernorStorage.sol b/contracts/governance/extensions/GovernorStorage.sol new file mode 100644 index 00000000000..6443b789541 --- /dev/null +++ b/contracts/governance/extensions/GovernorStorage.sol @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.19; + +import {Governor} from "../Governor.sol"; + +/** + * @dev Extension of {Governor} that implements storage of proposal details. This modules also provides primitives for + * the enumerability of proposals. + * + * Use cases for this module include: + * - UIs that explore the proposal state without relying on event indexing. + * - Using only the proposalId as an argument in the {Governor-queue} and {Governor-execute} functions for L2 chains where storage is cheap compared to calldata. + */ +abstract contract GovernorStorage is Governor { + struct ProposalDetails { + address[] targets; + uint256[] values; + bytes[] calldatas; + bytes32 descriptionHash; + } + + uint256[] private _proposalIds; + mapping(uint256 => ProposalDetails) private _proposalDetails; + + /** + * @dev Hook into the proposing mechanism + */ + function _propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description, + address proposer + ) internal virtual override returns (uint256) { + uint256 proposalId = super._propose(targets, values, calldatas, description, proposer); + + // store + _proposalIds.push(proposalId); + _proposalDetails[proposalId] = ProposalDetails({ + targets: targets, + values: values, + calldatas: calldatas, + descriptionHash: keccak256(bytes(description)) + }); + + return proposalId; + } + + /** + * @dev Version of {IGovernorTimelock-queue} with only `proposalId` as an argument. + */ + function queue(uint256 proposalId) public virtual { + // here, using storage is more efficient than memory + ProposalDetails storage details = _proposalDetails[proposalId]; + queue(details.targets, details.values, details.calldatas, details.descriptionHash); + } + + /** + * @dev Version of {IGovernor-execute} with only `proposalId` as an argument. + */ + function execute(uint256 proposalId) public payable virtual { + // here, using storage is more efficient than memory + ProposalDetails storage details = _proposalDetails[proposalId]; + execute(details.targets, details.values, details.calldatas, details.descriptionHash); + } + + /** + * @dev ProposalId version of {IGovernor-cancel}. + */ + function cancel(uint256 proposalId) public virtual { + // here, using storage is more efficient than memory + ProposalDetails storage details = _proposalDetails[proposalId]; + cancel(details.targets, details.values, details.calldatas, details.descriptionHash); + } + + /** + * @dev Returns the number of stored proposals. + */ + function proposalCount() public view virtual returns (uint256) { + return _proposalIds.length; + } + + /** + * @dev Returns the details of a proposalId. Reverts if `proposalId` is not a known proposal. + */ + function proposalDetails( + uint256 proposalId + ) public view virtual returns (address[] memory, uint256[] memory, bytes[] memory, bytes32) { + // here, using memory is more efficient than storage + ProposalDetails memory details = _proposalDetails[proposalId]; + if (details.descriptionHash == 0) { + revert GovernorNonexistentProposal(proposalId); + } + return (details.targets, details.values, details.calldatas, details.descriptionHash); + } + + /** + * @dev Returns the details (including the proposalId) of a proposal given its sequential index. + */ + function proposalDetailsAt( + uint256 index + ) public view virtual returns (uint256, address[] memory, uint256[] memory, bytes[] memory, bytes32) { + uint256 proposalId = _proposalIds[index]; + ( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) = proposalDetails(proposalId); + return (proposalId, targets, values, calldatas, descriptionHash); + } +} diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 30181b6d284..fe554dad5d2 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -3,11 +3,11 @@ pragma solidity ^0.8.20; -import {IGovernorTimelock} from "./IGovernorTimelock.sol"; import {IGovernor, Governor} from "../Governor.sol"; import {ICompoundTimelock} from "../../vendor/compound/ICompoundTimelock.sol"; import {IERC165} from "../../interfaces/IERC165.sol"; import {Address} from "../../utils/Address.sol"; +import {SafeCast} from "../../utils/math/SafeCast.sol"; /** * @dev Extension of {Governor} that binds the execution process to a Compound Timelock. This adds a delay, enforced by @@ -19,11 +19,9 @@ import {Address} from "../../utils/Address.sol"; * the assets and permissions must be attached to the {TimelockController}. Any asset sent to the {Governor} will be * inaccessible. */ -abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { +abstract contract GovernorTimelockCompound is Governor { ICompoundTimelock private _timelock; - mapping(uint256 => uint256) private _proposalTimelocks; - /** * @dev Emitted when the timelock controller used for proposal execution is modified. */ @@ -37,68 +35,36 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { } /** - * @dev See {IERC165-supportsInterface}. - */ - function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, Governor) returns (bool) { - return interfaceId == type(IGovernorTimelock).interfaceId || super.supportsInterface(interfaceId); - } - - /** - * @dev Overridden version of the {Governor-state} function with added support for the `Queued` and `Expired` state. + * @dev Overridden version of the {Governor-state} function with added support for the `Expired` state. */ - function state(uint256 proposalId) public view virtual override(IGovernor, Governor) returns (ProposalState) { + function state(uint256 proposalId) public view virtual override returns (ProposalState) { ProposalState currentState = super.state(proposalId); - if (currentState != ProposalState.Succeeded) { - return currentState; - } - - uint256 eta = proposalEta(proposalId); - if (eta == 0) { - return currentState; - } else if (block.timestamp >= eta + _timelock.GRACE_PERIOD()) { - return ProposalState.Expired; - } else { - return ProposalState.Queued; - } + return + (currentState == ProposalState.Queued && + block.timestamp >= proposalEta(proposalId) + _timelock.GRACE_PERIOD()) + ? ProposalState.Expired + : currentState; } /** * @dev Public accessor to check the address of the timelock */ - function timelock() public view virtual override returns (address) { + function timelock() public view virtual returns (address) { return address(_timelock); } - /** - * @dev Public accessor to check the eta of a queued proposal - */ - function proposalEta(uint256 proposalId) public view virtual override returns (uint256) { - return _proposalTimelocks[proposalId]; - } - /** * @dev Function to queue a proposal to the timelock. */ - function queue( + function _queueOperations( + uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, - bytes32 descriptionHash - ) public virtual override returns (uint256) { - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - - ProposalState currentState = state(proposalId); - if (currentState != ProposalState.Succeeded) { - revert GovernorUnexpectedProposalState( - proposalId, - currentState, - _encodeStateBitmap(ProposalState.Succeeded) - ); - } - - uint256 eta = block.timestamp + _timelock.delay(); - _proposalTimelocks[proposalId] = eta; + bytes32 /*descriptionHash*/ + ) internal virtual override returns (uint48) { + uint48 eta = SafeCast.toUint48(block.timestamp + _timelock.delay()); for (uint256 i = 0; i < targets.length; ++i) { if (_timelock.queuedTransactions(keccak256(abi.encode(targets[i], values[i], "", calldatas[i], eta)))) { @@ -107,15 +73,14 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { _timelock.queueTransaction(targets[i], values[i], "", calldatas[i], eta); } - emit ProposalQueued(proposalId, eta); - - return proposalId; + return eta; } /** - * @dev Overridden execute function that run the already queued proposal through the timelock. + * @dev Overridden version of the {Governor-_executeOperations} function that run the already queued proposal through + * the timelock. */ - function _execute( + function _executeOperations( uint256 proposalId, address[] memory targets, uint256[] memory values, @@ -146,8 +111,6 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { uint256 eta = proposalEta(proposalId); if (eta > 0) { - // update state first - delete _proposalTimelocks[proposalId]; // do external call later for (uint256 i = 0; i < targets.length; ++i) { _timelock.cancelTransaction(targets[i], values[i], "", calldatas[i], eta); diff --git a/contracts/governance/extensions/GovernorTimelockControl.sol b/contracts/governance/extensions/GovernorTimelockControl.sol index 615b9330a93..c468f328b31 100644 --- a/contracts/governance/extensions/GovernorTimelockControl.sol +++ b/contracts/governance/extensions/GovernorTimelockControl.sol @@ -3,10 +3,10 @@ pragma solidity ^0.8.20; -import {IGovernorTimelock} from "./IGovernorTimelock.sol"; import {IGovernor, Governor} from "../Governor.sol"; import {TimelockController} from "../TimelockController.sol"; import {IERC165} from "../../interfaces/IERC165.sol"; +import {SafeCast} from "../../utils/math/SafeCast.sol"; /** * @dev Extension of {Governor} that binds the execution process to an instance of {TimelockController}. This adds a @@ -22,7 +22,7 @@ import {IERC165} from "../../interfaces/IERC165.sol"; * available to them through the timelock, and 2) approved governance proposals can be blocked by them, effectively * executing a Denial of Service attack. This risk will be mitigated in a future release. */ -abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { +abstract contract GovernorTimelockControl is Governor { TimelockController private _timelock; mapping(uint256 => bytes32) private _timelockIds; @@ -39,27 +39,17 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { } /** - * @dev See {IERC165-supportsInterface}. + * @dev Overridden version of the {Governor-state} function that considers the status reported by the timelock. */ - function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, Governor) returns (bool) { - return interfaceId == type(IGovernorTimelock).interfaceId || super.supportsInterface(interfaceId); - } - - /** - * @dev Overridden version of the {Governor-state} function with added support for the `Queued` state. - */ - function state(uint256 proposalId) public view virtual override(IGovernor, Governor) returns (ProposalState) { + function state(uint256 proposalId) public view virtual override returns (ProposalState) { ProposalState currentState = super.state(proposalId); - if (currentState != ProposalState.Succeeded) { + if (currentState != ProposalState.Queued) { return currentState; } - // core tracks execution, so we just have to check if successful proposal have been queued. bytes32 queueid = _timelockIds[proposalId]; - if (queueid == bytes32(0)) { - return currentState; - } else if (_timelock.isOperationPending(queueid)) { + if (_timelock.isOperationPending(queueid)) { return ProposalState.Queued; } else if (_timelock.isOperationDone(queueid)) { // This can happen if the proposal is executed directly on the timelock. @@ -73,52 +63,34 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { /** * @dev Public accessor to check the address of the timelock */ - function timelock() public view virtual override returns (address) { + function timelock() public view virtual returns (address) { return address(_timelock); } - /** - * @dev Public accessor to check the eta of a queued proposal - */ - function proposalEta(uint256 proposalId) public view virtual override returns (uint256) { - uint256 eta = _timelock.getTimestamp(_timelockIds[proposalId]); - return eta == 1 ? 0 : eta; // _DONE_TIMESTAMP (1) should be replaced with a 0 value - } - /** * @dev Function to queue a proposal to the timelock. */ - function queue( + function _queueOperations( + uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public virtual override returns (uint256) { - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); - - ProposalState currentState = state(proposalId); - if (currentState != ProposalState.Succeeded) { - revert GovernorUnexpectedProposalState( - proposalId, - currentState, - _encodeStateBitmap(ProposalState.Succeeded) - ); - } - + ) internal virtual override returns (uint48) { uint256 delay = _timelock.getMinDelay(); + bytes32 salt = _timelockSalt(descriptionHash); _timelockIds[proposalId] = _timelock.hashOperationBatch(targets, values, calldatas, 0, salt); _timelock.scheduleBatch(targets, values, calldatas, 0, salt, delay); - emit ProposalQueued(proposalId, block.timestamp + delay); - - return proposalId; + return SafeCast.toUint48(block.timestamp + delay); } /** - * @dev Overridden execute function that run the already queued proposal through the timelock. + * @dev Overridden version of the {Governor-_executeOperations} function that runs the already queued proposal through + * the timelock. */ - function _execute( + function _executeOperations( uint256 proposalId, address[] memory targets, uint256[] memory values, @@ -145,8 +117,8 @@ abstract contract GovernorTimelockControl is IGovernorTimelock, Governor { bytes32 descriptionHash ) internal virtual override returns (uint256) { uint256 proposalId = super._cancel(targets, values, calldatas, descriptionHash); - bytes32 timelockId = _timelockIds[proposalId]; + bytes32 timelockId = _timelockIds[proposalId]; if (timelockId != 0) { // cancel _timelock.cancel(timelockId); diff --git a/contracts/governance/extensions/IGovernorTimelock.sol b/contracts/governance/extensions/IGovernorTimelock.sol deleted file mode 100644 index 28a9f933010..00000000000 --- a/contracts/governance/extensions/IGovernorTimelock.sol +++ /dev/null @@ -1,34 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.1 (governance/extensions/IGovernorTimelock.sol) - -pragma solidity ^0.8.20; - -import {IGovernor} from "../IGovernor.sol"; - -/** - * @dev Extension of the {IGovernor} for timelock supporting modules. - */ -abstract contract IGovernorTimelock is IGovernor { - /** - * @dev The proposal hasn't been queued yet. - */ - error GovernorNotQueuedProposal(uint256 proposalId); - - /** - * @dev The proposal has already been queued. - */ - error GovernorAlreadyQueuedProposal(uint256 proposalId); - - event ProposalQueued(uint256 proposalId, uint256 eta); - - function timelock() public view virtual returns (address); - - function proposalEta(uint256 proposalId) public view virtual returns (uint256); - - function queue( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) public virtual returns (uint256 proposalId); -} diff --git a/contracts/mocks/docs/governance/MyGovernor.sol b/contracts/mocks/docs/governance/MyGovernor.sol index 2ac6d2cefab..7cd5f3c2c74 100644 --- a/contracts/mocks/docs/governance/MyGovernor.sol +++ b/contracts/mocks/docs/governance/MyGovernor.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; import {IGovernor, Governor} from "../../../governance/Governor.sol"; -import {GovernorCompatibilityBravo} from "../../../governance/compatibility/GovernorCompatibilityBravo.sol"; +import {GovernorCountingSimple} from "../../../governance/extensions/GovernorCountingSimple.sol"; import {GovernorVotes} from "../../../governance/extensions/GovernorVotes.sol"; import {GovernorVotesQuorumFraction} from "../../../governance/extensions/GovernorVotesQuorumFraction.sol"; import {GovernorTimelockControl} from "../../../governance/extensions/GovernorTimelockControl.sol"; @@ -12,7 +12,7 @@ import {IERC165} from "../../../interfaces/IERC165.sol"; contract MyGovernor is Governor, - GovernorCompatibilityBravo, + GovernorCountingSimple, GovernorVotes, GovernorVotesQuorumFraction, GovernorTimelockControl @@ -36,38 +36,28 @@ contract MyGovernor is // The functions below are overrides required by Solidity. - function state( - uint256 proposalId - ) public view override(Governor, IGovernor, GovernorTimelockControl) returns (ProposalState) { + function state(uint256 proposalId) public view override(Governor, GovernorTimelockControl) returns (ProposalState) { return super.state(proposalId); } - function propose( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - string memory description - ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) { - return super.propose(targets, values, calldatas, description); - } - - function cancel( + function _queueOperations( + uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) { - return super.cancel(targets, values, calldatas, descriptionHash); + ) internal override(Governor, GovernorTimelockControl) returns (uint48) { + return super._queueOperations(proposalId, targets, values, calldatas, descriptionHash); } - function _execute( + function _executeOperations( uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal override(Governor, GovernorTimelockControl) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); + super._executeOperations(proposalId, targets, values, calldatas, descriptionHash); } function _cancel( @@ -82,10 +72,4 @@ contract MyGovernor is function _executor() internal view override(Governor, GovernorTimelockControl) returns (address) { return super._executor(); } - - function supportsInterface( - bytes4 interfaceId - ) public view override(Governor, IERC165, GovernorTimelockControl) returns (bool) { - return super.supportsInterface(interfaceId); - } } diff --git a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol deleted file mode 100644 index 041c85f280b..00000000000 --- a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol +++ /dev/null @@ -1,102 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {IGovernor, Governor} from "../../governance/Governor.sol"; -import {GovernorCompatibilityBravo} from "../../governance/compatibility/GovernorCompatibilityBravo.sol"; -import {IGovernorTimelock, GovernorTimelockCompound} from "../../governance/extensions/GovernorTimelockCompound.sol"; -import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; -import {GovernorVotes} from "../../governance/extensions/GovernorVotes.sol"; -import {IERC165} from "../../interfaces/IERC165.sol"; - -abstract contract GovernorCompatibilityBravoMock is - GovernorCompatibilityBravo, - GovernorSettings, - GovernorTimelockCompound, - GovernorVotes -{ - function quorum(uint256) public pure override returns (uint256) { - return 0; - } - - function supportsInterface( - bytes4 interfaceId - ) public view override(IERC165, Governor, GovernorTimelockCompound) returns (bool) { - return super.supportsInterface(interfaceId); - } - - function state( - uint256 proposalId - ) public view override(IGovernor, Governor, GovernorTimelockCompound) returns (ProposalState) { - return super.state(proposalId); - } - - function proposalEta( - uint256 proposalId - ) public view override(IGovernorTimelock, GovernorTimelockCompound) returns (uint256) { - return super.proposalEta(proposalId); - } - - function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { - return super.proposalThreshold(); - } - - function propose( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - string memory description - ) public override(IGovernor, Governor, GovernorCompatibilityBravo) returns (uint256) { - return super.propose(targets, values, calldatas, description); - } - - function queue( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 salt - ) public override(IGovernorTimelock, GovernorTimelockCompound) returns (uint256) { - return super.queue(targets, values, calldatas, salt); - } - - function execute( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 salt - ) public payable override(IGovernor, Governor) returns (uint256) { - return super.execute(targets, values, calldatas, salt); - } - - function cancel( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) { - return super.cancel(targets, values, calldatas, descriptionHash); - } - - function _execute( - uint256 proposalId, - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) internal override(Governor, GovernorTimelockCompound) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); - } - - function _cancel( - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 salt - ) internal override(Governor, GovernorTimelockCompound) returns (uint256 proposalId) { - return super._cancel(targets, values, calldatas, salt); - } - - function _executor() internal view override(Governor, GovernorTimelockCompound) returns (address) { - return super._executor(); - } -} diff --git a/contracts/mocks/governance/GovernorStorageMock.sol b/contracts/mocks/governance/GovernorStorageMock.sol new file mode 100644 index 00000000000..3d08a003170 --- /dev/null +++ b/contracts/mocks/governance/GovernorStorageMock.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.19; + +import {IGovernor, Governor} from "../../governance/Governor.sol"; +import {GovernorTimelockControl} from "../../governance/extensions/GovernorTimelockControl.sol"; +import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; +import {GovernorCountingSimple} from "../../governance/extensions/GovernorCountingSimple.sol"; +import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; +import {GovernorStorage} from "../../governance/extensions/GovernorStorage.sol"; + +abstract contract GovernorStorageMock is + GovernorSettings, + GovernorTimelockControl, + GovernorVotesQuorumFraction, + GovernorCountingSimple, + GovernorStorage +{ + function quorum(uint256 blockNumber) public view override(Governor, GovernorVotesQuorumFraction) returns (uint256) { + return super.quorum(blockNumber); + } + + function state(uint256 proposalId) public view override(Governor, GovernorTimelockControl) returns (ProposalState) { + return super.state(proposalId); + } + + function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function _propose( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + string memory description, + address proposer + ) internal virtual override(Governor, GovernorStorage) returns (uint256) { + return super._propose(targets, values, calldatas, description, proposer); + } + + function _queueOperations( + uint256 proposalId, + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockControl) returns (uint48) { + return super._queueOperations(proposalId, targets, values, calldatas, descriptionHash); + } + + function _executeOperations( + uint256 proposalId, + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockControl) { + super._executeOperations(proposalId, targets, values, calldatas, descriptionHash); + } + + function _cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockControl) returns (uint256) { + return super._cancel(targets, values, calldatas, descriptionHash); + } + + function _executor() internal view override(Governor, GovernorTimelockControl) returns (address) { + return super._executor(); + } +} diff --git a/contracts/mocks/governance/GovernorTimelockCompoundMock.sol b/contracts/mocks/governance/GovernorTimelockCompoundMock.sol index d17d4428a65..124b0346fbf 100644 --- a/contracts/mocks/governance/GovernorTimelockCompoundMock.sol +++ b/contracts/mocks/governance/GovernorTimelockCompoundMock.sol @@ -14,15 +14,7 @@ abstract contract GovernorTimelockCompoundMock is GovernorVotesQuorumFraction, GovernorCountingSimple { - function supportsInterface( - bytes4 interfaceId - ) public view override(Governor, GovernorTimelockCompound) returns (bool) { - return super.supportsInterface(interfaceId); - } - - function quorum( - uint256 blockNumber - ) public view override(IGovernor, GovernorVotesQuorumFraction) returns (uint256) { + function quorum(uint256 blockNumber) public view override(Governor, GovernorVotesQuorumFraction) returns (uint256) { return super.quorum(blockNumber); } @@ -36,23 +28,33 @@ abstract contract GovernorTimelockCompoundMock is return super.proposalThreshold(); } - function _execute( + function _queueOperations( + uint256 proposalId, + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockCompound) returns (uint48) { + return super._queueOperations(proposalId, targets, values, calldatas, descriptionHash); + } + + function _executeOperations( uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal override(Governor, GovernorTimelockCompound) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); + super._executeOperations(proposalId, targets, values, calldatas, descriptionHash); } function _cancel( address[] memory targets, uint256[] memory values, bytes[] memory calldatas, - bytes32 salt - ) internal override(Governor, GovernorTimelockCompound) returns (uint256 proposalId) { - return super._cancel(targets, values, calldatas, salt); + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockCompound) returns (uint256) { + return super._cancel(targets, values, calldatas, descriptionHash); } function _executor() internal view override(Governor, GovernorTimelockCompound) returns (address) { diff --git a/contracts/mocks/governance/GovernorTimelockControlMock.sol b/contracts/mocks/governance/GovernorTimelockControlMock.sol index 7182a38be10..125aa37df11 100644 --- a/contracts/mocks/governance/GovernorTimelockControlMock.sol +++ b/contracts/mocks/governance/GovernorTimelockControlMock.sol @@ -14,15 +14,7 @@ abstract contract GovernorTimelockControlMock is GovernorVotesQuorumFraction, GovernorCountingSimple { - function supportsInterface( - bytes4 interfaceId - ) public view override(Governor, GovernorTimelockControl) returns (bool) { - return super.supportsInterface(interfaceId); - } - - function quorum( - uint256 blockNumber - ) public view override(IGovernor, GovernorVotesQuorumFraction) returns (uint256) { + function quorum(uint256 blockNumber) public view override(Governor, GovernorVotesQuorumFraction) returns (uint256) { return super.quorum(blockNumber); } @@ -34,14 +26,24 @@ abstract contract GovernorTimelockControlMock is return super.proposalThreshold(); } - function _execute( + function _queueOperations( + uint256 proposalId, + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 descriptionHash + ) internal override(Governor, GovernorTimelockControl) returns (uint48) { + return super._queueOperations(proposalId, targets, values, calldatas, descriptionHash); + } + + function _executeOperations( uint256 proposalId, address[] memory targets, uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash ) internal override(Governor, GovernorTimelockControl) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); + super._executeOperations(proposalId, targets, values, calldatas, descriptionHash); } function _cancel( @@ -49,7 +51,7 @@ abstract contract GovernorTimelockControlMock is uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) internal override(Governor, GovernorTimelockControl) returns (uint256 proposalId) { + ) internal override(Governor, GovernorTimelockControl) returns (uint256) { return super._cancel(targets, values, calldatas, descriptionHash); } diff --git a/docs/modules/ROOT/pages/governance.adoc b/docs/modules/ROOT/pages/governance.adoc index ce5a9c34085..b8db6423005 100644 --- a/docs/modules/ROOT/pages/governance.adoc +++ b/docs/modules/ROOT/pages/governance.adoc @@ -18,11 +18,11 @@ OpenZeppelin’s Governor system was designed with a concern for compatibility w The ERC20 extension to keep track of votes and vote delegation is one such case. The shorter one is the more generic version because it can support token supplies greater than 2^96, while the “Comp” variant is limited in that regard, but exactly fits the interface of the COMP token that is used by GovernorAlpha and Bravo. Both contract variants share the same events, so they are fully compatible when looking at events only. -=== Governor & GovernorCompatibilityBravo +=== Governor & GovernorStorage -An OpenZeppelin Governor contract is by default not interface-compatible with Compound's GovernorAlpha or Bravo. Even though events are fully compatible, proposal lifecycle functions (creation, execution, etc.) have different signatures that are meant to optimize storage use. Other functions from GovernorAlpha are Bravo are likewise not available. It’s possible to opt in to a higher level of compatibility by inheriting from the GovernorCompatibilityBravo module, which covers the proposal lifecycle functions such as `propose` and `execute`. +An OpenZeppelin Governor contract is not interface-compatible with Compound's GovernorAlpha or Bravo. Even though events are fully compatible, proposal lifecycle functions (creation, execution, etc.) have different signatures that are meant to optimize storage use. Other functions from GovernorAlpha are Bravo are likewise not available. It’s possible to opt in some Bravo-like behavior by inheriting from the GovernorStorage module. This module provides proposal enumerability and alternate versions of the `queue`, `execute` and `cancel` function that only take the proposal id. This module reduces the calldata needed by some operations in exchange for an increased the storage footprint. This might be a good trade-off for some L2 chains. It also provides primitives for indexer-free frontends. -Note that even with the use of this module, there will still be differences in the way that `proposalId`s are calculated. Governor uses the hash of the proposal parameters with the purpose of keeping its data off-chain by event indexing, while the original Bravo implementation uses sequential `proposalId`s. Due to this and other differences, several of the functions from GovernorBravo are not included in the compatibility module. +Note that even with the use of this module, one important difference with Compound's GovernorBravo is the way that `proposalId`s are calculated. Governor uses the hash of the proposal parameters with the purpose of keeping its data off-chain by event indexing, while the original Bravo implementation uses sequential `proposalId`s. === GovernorTimelockControl & GovernorTimelockCompound @@ -201,13 +201,14 @@ The Governor will automatically detect the clock mode used by the token and adap pragma solidity ^0.8.20; import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; -import {GovernorCompatibilityBravo} from "@openzeppelin/contracts/governance/compatibility/GovernorCompatibilityBravo.sol"; +import {GovernorCountingSimple} from "@openzeppelin/contracts/governance/compatibility/GovernorCountingSimple.sol"; import {GovernorVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; import {GovernorVotesQuorumFraction} from "@openzeppelin/contracts/governance/extensions/GovernorVotesQuorumFraction.sol"; import {GovernorTimelockControl} from "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol"; +import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol"; import {IVotes} from "@openzeppelin/contracts/governance/utils/IVotes.sol"; -contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, GovernorVotesQuorumFraction, GovernorTimelockControl { +contract MyGovernor is Governor, GovernorCountingSimple, GovernorVotes, GovernorVotesQuorumFraction, GovernorTimelockControl { constructor(IVotes _token, TimelockController _timelock) Governor("MyGovernor") GovernorVotes(_token) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 4e47ab9f210..9dfa53e7bbd 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -79,7 +79,7 @@ contract('Governor', function (accounts) { ); }); - shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor', 'GovernorWithParams', 'GovernorCancel']); + shouldSupportInterfaces(['ERC165', 'ERC1155Receiver', 'Governor']); shouldBehaveLikeEIP6372(mode); it('deployment check', async function () { @@ -305,6 +305,17 @@ contract('Governor', function (accounts) { ZERO_BYTES32, ]); }); + + it('if proposer has below threshold votes', async function () { + const votes = web3.utils.toWei('10'); + const threshold = web3.utils.toWei('1000'); + await this.mock.$_setProposalThreshold(threshold); + await expectRevertCustomError(this.helper.propose({ from: voter1 }), 'GovernorInsufficientProposerVotes', [ + voter1, + votes, + threshold, + ]); + }); }); describe('on vote', function () { @@ -427,6 +438,16 @@ contract('Governor', function (accounts) { }); }); + describe('on queue', function () { + it('always', async function () { + await this.helper.propose({ from: proposer }); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await expectRevertCustomError(this.helper.queue(), 'GovernorQueueNotImplemented', []); + }); + }); + describe('on execute', function () { it('if proposal does not exist', async function () { await expectRevertCustomError(this.helper.execute(), 'GovernorNonexistentProposal', [this.proposal.id]); @@ -826,7 +847,9 @@ contract('Governor', function (accounts) { }); it('someone else cannot propose', async function () { - await expectRevert(this.helper.propose({ from: voter1 }), 'Governor: proposer restricted'); + await expectRevertCustomError(this.helper.propose({ from: voter1 }), 'GovernorRestrictedProposer', [ + voter1, + ]); }); }); }); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js deleted file mode 100644 index 26255342f6c..00000000000 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ /dev/null @@ -1,297 +0,0 @@ -const { expectEvent } = require('@openzeppelin/test-helpers'); -const { expect } = require('chai'); -const { computeCreateAddress } = require('../../helpers/create'); -const Enums = require('../../helpers/enums'); -const { GovernorHelper } = require('../../helpers/governance'); -const { clockFromReceipt } = require('../../helpers/time'); -const { expectRevertCustomError } = require('../../helpers/customError'); - -const Timelock = artifacts.require('CompTimelock'); -const Governor = artifacts.require('$GovernorCompatibilityBravoMock'); -const CallReceiver = artifacts.require('CallReceiverMock'); - -const { shouldBehaveLikeEIP6372 } = require('../utils/EIP6372.behavior'); - -const TOKENS = [ - { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, - { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, -]; - -contract('GovernorCompatibilityBravo', function (accounts) { - const [owner, proposer, voter1, voter2, voter3, voter4, other] = accounts; - - const name = 'OZ-Governor'; - const version = '1'; - const tokenName = 'MockToken'; - const tokenSymbol = 'MTKN'; - const tokenSupply = web3.utils.toWei('100'); - const votingDelay = web3.utils.toBN(4); - const votingPeriod = web3.utils.toBN(16); - const proposalThreshold = web3.utils.toWei('10'); - const value = web3.utils.toWei('1'); - - const votes = { - [owner]: tokenSupply, - [proposer]: proposalThreshold, - [voter1]: web3.utils.toWei('10'), - [voter2]: web3.utils.toWei('7'), - [voter3]: web3.utils.toWei('5'), - [voter4]: web3.utils.toWei('2'), - [other]: 0, - }; - - for (const { mode, Token } of TOKENS) { - describe(`using ${Token._json.contractName}`, function () { - beforeEach(async function () { - const [deployer] = await web3.eth.getAccounts(); - - this.token = await Token.new(tokenName, tokenSymbol, tokenName, version); - - // Need to predict governance address to set it as timelock admin with a delayed transfer - const nonce = await web3.eth.getTransactionCount(deployer); - const predictGovernor = computeCreateAddress(deployer, nonce + 1); - - this.timelock = await Timelock.new(predictGovernor, 2 * 86400); - this.mock = await Governor.new( - name, - votingDelay, - votingPeriod, - proposalThreshold, - this.timelock.address, - this.token.address, - ); - this.receiver = await CallReceiver.new(); - - this.helper = new GovernorHelper(this.mock, mode); - - await web3.eth.sendTransaction({ from: owner, to: this.timelock.address, value }); - - await this.token.$_mint(owner, tokenSupply); - await this.helper.delegate({ token: this.token, to: proposer, value: votes[proposer] }, { from: owner }); - await this.helper.delegate({ token: this.token, to: voter1, value: votes[voter1] }, { from: owner }); - await this.helper.delegate({ token: this.token, to: voter2, value: votes[voter2] }, { from: owner }); - await this.helper.delegate({ token: this.token, to: voter3, value: votes[voter3] }, { from: owner }); - await this.helper.delegate({ token: this.token, to: voter4, value: votes[voter4] }, { from: owner }); - - // default proposal - this.proposal = this.helper.setProposal( - [ - { - target: this.receiver.address, - value, - signature: 'mockFunction()', - }, - ], - '', - ); - }); - - shouldBehaveLikeEIP6372(mode); - - it('deployment check', async function () { - expect(await this.mock.name()).to.be.equal(name); - expect(await this.mock.token()).to.be.equal(this.token.address); - expect(await this.mock.votingDelay()).to.be.bignumber.equal(votingDelay); - expect(await this.mock.votingPeriod()).to.be.bignumber.equal(votingPeriod); - expect(await this.mock.quorum(0)).to.be.bignumber.equal('0'); - expect(await this.mock.quorumVotes()).to.be.bignumber.equal('0'); - expect(await this.mock.COUNTING_MODE()).to.be.equal('support=bravo&quorum=bravo'); - }); - - it('nominal workflow', async function () { - // Before - expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false); - expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(false); - expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(false); - expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal('0'); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(value); - expect(await web3.eth.getBalance(this.receiver.address)).to.be.bignumber.equal('0'); - - // Run proposal - const txPropose = await this.helper.propose({ from: proposer }); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For, reason: 'This is nice' }, { from: voter1 }); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter2 }); - await this.helper.vote({ support: Enums.VoteType.Against }, { from: voter3 }); - await this.helper.vote({ support: Enums.VoteType.Abstain }, { from: voter4 }); - await this.helper.waitForDeadline(); - await this.helper.queue(); - await this.helper.waitForEta(); - const txExecute = await this.helper.execute(); - - // After - expect(await this.mock.hasVoted(this.proposal.id, owner)).to.be.equal(false); - expect(await this.mock.hasVoted(this.proposal.id, voter1)).to.be.equal(true); - expect(await this.mock.hasVoted(this.proposal.id, voter2)).to.be.equal(true); - expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal('0'); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal('0'); - expect(await web3.eth.getBalance(this.receiver.address)).to.be.bignumber.equal(value); - - const proposal = await this.mock.proposals(this.proposal.id); - expect(proposal.id).to.be.bignumber.equal(this.proposal.id); - expect(proposal.proposer).to.be.equal(proposer); - expect(proposal.eta).to.be.bignumber.equal(await this.mock.proposalEta(this.proposal.id)); - expect(proposal.startBlock).to.be.bignumber.equal(await this.mock.proposalSnapshot(this.proposal.id)); - expect(proposal.endBlock).to.be.bignumber.equal(await this.mock.proposalDeadline(this.proposal.id)); - expect(proposal.canceled).to.be.equal(false); - expect(proposal.executed).to.be.equal(true); - - const action = await this.mock.getActions(this.proposal.id); - expect(action.targets).to.be.deep.equal(this.proposal.targets); - // expect(action.values).to.be.deep.equal(this.proposal.values); - expect(action.signatures).to.be.deep.equal(this.proposal.signatures); - expect(action.calldatas).to.be.deep.equal(this.proposal.data); - - const voteReceipt1 = await this.mock.getReceipt(this.proposal.id, voter1); - expect(voteReceipt1.hasVoted).to.be.equal(true); - expect(voteReceipt1.support).to.be.bignumber.equal(Enums.VoteType.For); - expect(voteReceipt1.votes).to.be.bignumber.equal(web3.utils.toWei('10')); - - const voteReceipt2 = await this.mock.getReceipt(this.proposal.id, voter2); - expect(voteReceipt2.hasVoted).to.be.equal(true); - expect(voteReceipt2.support).to.be.bignumber.equal(Enums.VoteType.For); - expect(voteReceipt2.votes).to.be.bignumber.equal(web3.utils.toWei('7')); - - const voteReceipt3 = await this.mock.getReceipt(this.proposal.id, voter3); - expect(voteReceipt3.hasVoted).to.be.equal(true); - expect(voteReceipt3.support).to.be.bignumber.equal(Enums.VoteType.Against); - expect(voteReceipt3.votes).to.be.bignumber.equal(web3.utils.toWei('5')); - - const voteReceipt4 = await this.mock.getReceipt(this.proposal.id, voter4); - expect(voteReceipt4.hasVoted).to.be.equal(true); - expect(voteReceipt4.support).to.be.bignumber.equal(Enums.VoteType.Abstain); - expect(voteReceipt4.votes).to.be.bignumber.equal(web3.utils.toWei('2')); - - expectEvent(txPropose, 'ProposalCreated', { - proposalId: this.proposal.id, - proposer, - targets: this.proposal.targets, - // values: this.proposal.values, - signatures: this.proposal.signatures.map(() => ''), // this event doesn't contain the proposal detail - calldatas: this.proposal.fulldata, - voteStart: web3.utils.toBN(await clockFromReceipt[mode](txPropose.receipt)).add(votingDelay), - voteEnd: web3.utils - .toBN(await clockFromReceipt[mode](txPropose.receipt)) - .add(votingDelay) - .add(votingPeriod), - description: this.proposal.description, - }); - expectEvent(txExecute, 'ProposalExecuted', { proposalId: this.proposal.id }); - await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalled'); - }); - - it('double voting is forbidden', async function () { - await this.helper.propose({ from: proposer }); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await expectRevertCustomError( - this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }), - 'GovernorAlreadyCastVote', - [voter1], - ); - }); - - it('with function selector and arguments', async function () { - const target = this.receiver.address; - this.helper.setProposal( - [ - { target, data: this.receiver.contract.methods.mockFunction().encodeABI() }, - { target, data: this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI() }, - { target, signature: 'mockFunctionNonPayable()' }, - { - target, - signature: 'mockFunctionWithArgs(uint256,uint256)', - data: web3.eth.abi.encodeParameters(['uint256', 'uint256'], [18, 43]), - }, - ], - '', - ); - - await this.helper.propose({ from: proposer }); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For, reason: 'This is nice' }, { from: voter1 }); - await this.helper.waitForDeadline(); - await this.helper.queue(); - await this.helper.waitForEta(); - const txExecute = await this.helper.execute(); - - await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalled'); - await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalled'); - await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalledWithArgs', { - a: '17', - b: '42', - }); - await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalledWithArgs', { - a: '18', - b: '43', - }); - }); - - it('with inconsistent array size for selector and arguments', async function () { - const target = this.receiver.address; - const signatures = ['mockFunction()']; // One signature - const data = ['0x', this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI()]; // Two data entries - this.helper.setProposal( - { - targets: [target, target], - values: [0, 0], - signatures, - data, - }, - '', - ); - - await expectRevertCustomError(this.helper.propose({ from: proposer }), 'GovernorInvalidSignaturesLength', [ - signatures.length, - data.length, - ]); - }); - - describe('should revert', function () { - describe('on propose', function () { - it('if proposal does not meet proposalThreshold', async function () { - await expectRevertCustomError(this.helper.propose({ from: other }), 'GovernorInsufficientProposerVotes', [ - other, - votes[other], - proposalThreshold, - ]); - }); - }); - - describe('on vote', function () { - it('if vote type is invalid', async function () { - await this.helper.propose({ from: proposer }); - await this.helper.waitForSnapshot(); - await expectRevertCustomError( - this.helper.vote({ support: 5 }, { from: voter1 }), - 'GovernorInvalidVoteType', - [], - ); - }); - }); - }); - - describe('cancel', function () { - it('proposer can cancel', async function () { - await this.helper.propose({ from: proposer }); - await this.helper.cancel('external', { from: proposer }); - }); - - it('anyone can cancel if proposer drop below threshold', async function () { - await this.helper.propose({ from: proposer }); - await this.token.transfer(voter1, web3.utils.toWei('1'), { from: proposer }); - await this.helper.cancel('external'); - }); - - it('cannot cancel is proposer is still above threshold', async function () { - await this.helper.propose({ from: proposer }); - await expectRevertCustomError(this.helper.cancel('external'), 'GovernorInsufficientProposerVotes', [ - proposer, - votes[proposer], - proposalThreshold, - ]); - }); - }); - }); - } -}); diff --git a/test/governance/extensions/GovernorStorage.test.js b/test/governance/extensions/GovernorStorage.test.js new file mode 100644 index 00000000000..99a97886c37 --- /dev/null +++ b/test/governance/extensions/GovernorStorage.test.js @@ -0,0 +1,150 @@ +const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const { expectRevertCustomError } = require('../../helpers/customError'); +const Enums = require('../../helpers/enums'); +const { GovernorHelper, timelockSalt } = require('../../helpers/governance'); + +const Timelock = artifacts.require('TimelockController'); +const Governor = artifacts.require('$GovernorStorageMock'); +const CallReceiver = artifacts.require('CallReceiverMock'); + +const TOKENS = [ + { Token: artifacts.require('$ERC20Votes'), mode: 'blocknumber' }, + { Token: artifacts.require('$ERC20VotesTimestampMock'), mode: 'timestamp' }, +]; + +contract('GovernorStorage', function (accounts) { + const [owner, voter1, voter2, voter3, voter4] = accounts; + + const DEFAULT_ADMIN_ROLE = '0x0000000000000000000000000000000000000000000000000000000000000000'; + const PROPOSER_ROLE = web3.utils.soliditySha3('PROPOSER_ROLE'); + const EXECUTOR_ROLE = web3.utils.soliditySha3('EXECUTOR_ROLE'); + const CANCELLER_ROLE = web3.utils.soliditySha3('CANCELLER_ROLE'); + + const name = 'OZ-Governor'; + const version = '1'; + const tokenName = 'MockToken'; + const tokenSymbol = 'MTKN'; + const tokenSupply = web3.utils.toWei('100'); + const votingDelay = web3.utils.toBN(4); + const votingPeriod = web3.utils.toBN(16); + const value = web3.utils.toWei('1'); + + for (const { mode, Token } of TOKENS) { + describe(`using ${Token._json.contractName}`, function () { + beforeEach(async function () { + const [deployer] = await web3.eth.getAccounts(); + + this.token = await Token.new(tokenName, tokenSymbol, tokenName, version); + this.timelock = await Timelock.new(3600, [], [], deployer); + this.mock = await Governor.new( + name, + votingDelay, + votingPeriod, + 0, + this.timelock.address, + this.token.address, + 0, + ); + this.receiver = await CallReceiver.new(); + + this.helper = new GovernorHelper(this.mock, mode); + + await web3.eth.sendTransaction({ from: owner, to: this.timelock.address, value }); + + // normal setup: governor is proposer, everyone is executor, timelock is its own admin + await this.timelock.grantRole(PROPOSER_ROLE, this.mock.address); + await this.timelock.grantRole(PROPOSER_ROLE, owner); + await this.timelock.grantRole(CANCELLER_ROLE, this.mock.address); + await this.timelock.grantRole(CANCELLER_ROLE, owner); + await this.timelock.grantRole(EXECUTOR_ROLE, constants.ZERO_ADDRESS); + await this.timelock.revokeRole(DEFAULT_ADMIN_ROLE, deployer); + + await this.token.$_mint(owner, tokenSupply); + await this.helper.delegate({ token: this.token, to: voter1, value: web3.utils.toWei('10') }, { from: owner }); + await this.helper.delegate({ token: this.token, to: voter2, value: web3.utils.toWei('7') }, { from: owner }); + await this.helper.delegate({ token: this.token, to: voter3, value: web3.utils.toWei('5') }, { from: owner }); + await this.helper.delegate({ token: this.token, to: voter4, value: web3.utils.toWei('2') }, { from: owner }); + + // default proposal + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.address, + value, + data: this.receiver.contract.methods.mockFunction().encodeABI(), + }, + ], + '', + ); + this.proposal.timelockid = await this.timelock.hashOperationBatch( + ...this.proposal.shortProposal.slice(0, 3), + '0x0', + timelockSalt(this.mock.address, this.proposal.shortProposal[3]), + ); + }); + + describe('proposal indexing', function () { + it('before propose', async function () { + expect(await this.mock.proposalCount()).to.be.bignumber.equal('0'); + + // panic code 0x32 (out-of-bound) + await expectRevert.unspecified(this.mock.proposalDetailsAt(0)); + + await expectRevertCustomError(this.mock.proposalDetails(this.proposal.id), 'GovernorNonexistentProposal', [ + this.proposal.id, + ]); + }); + + it('after propose', async function () { + await this.helper.propose(); + + expect(await this.mock.proposalCount()).to.be.bignumber.equal('1'); + + const proposalDetailsAt0 = await this.mock.proposalDetailsAt(0); + expect(proposalDetailsAt0[0]).to.be.bignumber.equal(this.proposal.id); + expect(proposalDetailsAt0[1]).to.be.deep.equal(this.proposal.targets); + expect(proposalDetailsAt0[2].map(x => x.toString())).to.be.deep.equal(this.proposal.values); + expect(proposalDetailsAt0[3]).to.be.deep.equal(this.proposal.fulldata); + expect(proposalDetailsAt0[4]).to.be.equal(this.proposal.descriptionHash); + + const proposalDetailsForId = await this.mock.proposalDetails(this.proposal.id); + expect(proposalDetailsForId[0]).to.be.deep.equal(this.proposal.targets); + expect(proposalDetailsForId[1].map(x => x.toString())).to.be.deep.equal(this.proposal.values); + expect(proposalDetailsForId[2]).to.be.deep.equal(this.proposal.fulldata); + expect(proposalDetailsForId[3]).to.be.equal(this.proposal.descriptionHash); + }); + }); + + it('queue and execute by id', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter2 }); + await this.helper.vote({ support: Enums.VoteType.Against }, { from: voter3 }); + await this.helper.vote({ support: Enums.VoteType.Abstain }, { from: voter4 }); + await this.helper.waitForDeadline(); + const txQueue = await this.mock.queue(this.proposal.id); + await this.helper.waitForEta(); + const txExecute = await this.mock.execute(this.proposal.id); + + expectEvent(txQueue, 'ProposalQueued', { proposalId: this.proposal.id }); + await expectEvent.inTransaction(txQueue.tx, this.timelock, 'CallScheduled', { id: this.proposal.timelockid }); + await expectEvent.inTransaction(txQueue.tx, this.timelock, 'CallSalt', { + id: this.proposal.timelockid, + }); + + expectEvent(txExecute, 'ProposalExecuted', { proposalId: this.proposal.id }); + await expectEvent.inTransaction(txExecute.tx, this.timelock, 'CallExecuted', { id: this.proposal.timelockid }); + await expectEvent.inTransaction(txExecute.tx, this.receiver, 'MockFunctionCalled'); + }); + + it('cancel by id', async function () { + await this.helper.propose(); + const txCancel = await this.mock.cancel(this.proposal.id); + expectEvent(txCancel, 'ProposalCanceled', { proposalId: this.proposal.id }); + }); + }); + } +}); diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 7a2cb0abd8e..0f7fd635255 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -6,8 +6,6 @@ const { GovernorHelper, proposalStatesToBitMap } = require('../../helpers/govern const { expectRevertCustomError } = require('../../helpers/customError'); const { computeCreateAddress } = require('../../helpers/create'); -const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior'); - const Timelock = artifacts.require('CompTimelock'); const Governor = artifacts.require('$GovernorTimelockCompoundMock'); const CallReceiver = artifacts.require('CallReceiverMock'); @@ -77,8 +75,6 @@ contract('GovernorTimelockCompound', function (accounts) { ); }); - shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorWithParams', 'GovernorTimelock']); - it("doesn't accept ether transfers", async function () { await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 })); }); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 8dde8acb97b..fe40d30e9e1 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -2,11 +2,9 @@ const { constants, expectEvent, expectRevert, time } = require('@openzeppelin/te const { expect } = require('chai'); const Enums = require('../../helpers/enums'); -const { GovernorHelper, proposalStatesToBitMap } = require('../../helpers/governance'); +const { GovernorHelper, proposalStatesToBitMap, timelockSalt } = require('../../helpers/governance'); const { expectRevertCustomError } = require('../../helpers/customError'); -const { shouldSupportInterfaces } = require('../../utils/introspection/SupportsInterface.behavior'); - const Timelock = artifacts.require('TimelockController'); const Governor = artifacts.require('$GovernorTimelockControlMock'); const CallReceiver = artifacts.require('CallReceiverMock'); @@ -37,9 +35,6 @@ contract('GovernorTimelockControl', function (accounts) { for (const { mode, Token } of TOKENS) { describe(`using ${Token._json.contractName}`, function () { - const timelockSalt = (address, descriptionHash) => - '0x' + web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)).toString(16, 64); - beforeEach(async function () { const [deployer] = await web3.eth.getAccounts(); @@ -97,8 +92,6 @@ contract('GovernorTimelockControl', function (accounts) { ); }); - shouldSupportInterfaces(['ERC165', 'Governor', 'GovernorWithParams', 'GovernorTimelock']); - it("doesn't accept ether transfers", async function () { await expectRevert.unspecified(web3.eth.sendTransaction({ from: owner, to: this.mock.address, value: 1 })); }); diff --git a/test/helpers/create.js b/test/helpers/create.js index e0cea66e2df..98a0d4c4787 100644 --- a/test/helpers/create.js +++ b/test/helpers/create.js @@ -1,17 +1,16 @@ -const { rlp } = require('ethereumjs-util'); +const RLP = require('rlp'); function computeCreateAddress(deployer, nonce) { - return web3.utils.toChecksumAddress(web3.utils.sha3(rlp.encode([deployer.address ?? deployer, nonce])).slice(-40)); + return web3.utils.toChecksumAddress(web3.utils.sha3(RLP.encode([deployer.address ?? deployer, nonce])).slice(-40)); } function computeCreate2Address(saltHex, bytecode, deployer) { return web3.utils.toChecksumAddress( web3.utils .sha3( - '0x' + - ['ff', deployer.address ?? deployer, saltHex, web3.utils.soliditySha3(bytecode)] - .map(x => x.replace(/0x/, '')) - .join(''), + `0x${['ff', deployer.address ?? deployer, saltHex, web3.utils.soliditySha3(bytecode)] + .map(x => x.replace(/0x/, '')) + .join('')}`, ) .slice(-40), ); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 9b3349cf81c..fc4e30095a5 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -1,3 +1,4 @@ +const { web3 } = require('hardhat'); const { forward } = require('../helpers/time'); const { ProposalState } = require('./enums'); @@ -15,6 +16,9 @@ function concatOpts(args, opts = null) { return opts ? args.concat(opts) : args; } +const timelockSalt = (address, descriptionHash) => + '0x' + web3.utils.toBN(address).shln(96).xor(web3.utils.toBN(descriptionHash)).toString(16, 64); + class GovernorHelper { constructor(governor, mode = 'blocknumber') { this.governor = governor; @@ -245,4 +249,5 @@ function proposalStatesToBitMap(proposalStates, options = {}) { module.exports = { GovernorHelper, proposalStatesToBitMap, + timelockSalt, }; diff --git a/test/proxy/Clones.test.js b/test/proxy/Clones.test.js index 14a70e20bac..0862778f786 100644 --- a/test/proxy/Clones.test.js +++ b/test/proxy/Clones.test.js @@ -1,7 +1,6 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); - +const { computeCreate2Address } = require('../helpers/create'); const { expectRevertCustomError } = require('../helpers/customError'); const shouldBehaveLikeClone = require('./Clones.behaviour'); diff --git a/test/utils/Create2.test.js b/test/utils/Create2.test.js index afbcc3db958..4215e37c0ca 100644 --- a/test/utils/Create2.test.js +++ b/test/utils/Create2.test.js @@ -1,6 +1,6 @@ const { balance, ether, expectEvent, expectRevert, send } = require('@openzeppelin/test-helpers'); -const { computeCreate2Address } = require('../helpers/create'); const { expect } = require('chai'); +const { computeCreate2Address } = require('../helpers/create'); const { expectRevertCustomError } = require('../helpers/customError'); const Create2 = artifacts.require('$Create2'); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 80e144f18cb..d254082d2da 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -56,27 +56,11 @@ const INTERFACES = { 'COUNTING_MODE()', 'hashProposal(address[],uint256[],bytes[],bytes32)', 'state(uint256)', + 'proposalThreshold()', 'proposalSnapshot(uint256)', 'proposalDeadline(uint256)', - 'votingDelay()', - 'votingPeriod()', - 'quorum(uint256)', - 'getVotes(address,uint256)', - 'hasVoted(uint256,address)', - 'propose(address[],uint256[],bytes[],string)', - 'execute(address[],uint256[],bytes[],bytes32)', - 'castVote(uint256,uint8)', - 'castVoteWithReason(uint256,uint8,string)', - 'castVoteBySig(uint256,uint8,address,bytes)', - ], - GovernorWithParams: [ - 'name()', - 'version()', - 'COUNTING_MODE()', - 'hashProposal(address[],uint256[],bytes[],bytes32)', - 'state(uint256)', - 'proposalSnapshot(uint256)', - 'proposalDeadline(uint256)', + 'proposalProposer(uint256)', + 'proposalEta(uint256)', 'votingDelay()', 'votingPeriod()', 'quorum(uint256)', @@ -84,15 +68,15 @@ const INTERFACES = { 'getVotesWithParams(address,uint256,bytes)', 'hasVoted(uint256,address)', 'propose(address[],uint256[],bytes[],string)', + 'queue(address[],uint256[],bytes[],bytes32)', 'execute(address[],uint256[],bytes[],bytes32)', + 'cancel(address[],uint256[],bytes[],bytes32)', 'castVote(uint256,uint8)', 'castVoteWithReason(uint256,uint8,string)', 'castVoteWithReasonAndParams(uint256,uint8,string,bytes)', 'castVoteBySig(uint256,uint8,address,bytes)', 'castVoteWithReasonAndParamsBySig(uint256,uint8,address,string,bytes,bytes)', ], - GovernorCancel: ['proposalProposer(uint256)', 'cancel(address[],uint256[],bytes[],bytes32)'], - GovernorTimelock: ['timelock()', 'proposalEta(uint256)', 'queue(address[],uint256[],bytes[],bytes32)'], ERC2981: ['royaltyInfo(uint256,uint256)'], };