Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Extend Governor with parameterized votes #3043

Merged
merged 19 commits into from Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3bdd3ec
Add a data parameter to the _countVote interface
apbendi Dec 14, 2021
b86cbc8
linter fix
apbendi Dec 21, 2021
6aaad84
Rename governor extension data extension to params
apbendi Dec 21, 2021
287aa4f
Remove castVoteWithParams without reason parameter
apbendi Dec 22, 2021
92e939f
Add a virtual method specifying default additional voting params
apbendi Dec 22, 2021
b7eb845
Extend parameterized Governor customization to include vote weight ca…
Amxx Jan 13, 2022
ead0d51
Mock and test usage of Govenance extension params
apbendi Jan 17, 2022
0b49a65
Implement and test Govenor castVoteWithReasonAndParamsBySig method
apbendi Jan 17, 2022
0d5f887
Add VoteCastWithParams event emitted when params are included
apbendi Feb 10, 2022
d08f61a
Document changes associated with Governor extension params
apbendi Feb 10, 2022
a49eb41
Overload _castVote and curry using default parameters
apbendi Feb 15, 2022
0823a41
Silence compiler warnings by removing parameter name
apbendi Feb 15, 2022
214259c
Fix doc comments related to getting votes
apbendi Feb 15, 2022
970556e
Simplify Changelog description and include instructions for resolving…
apbendi Feb 15, 2022
6e2b447
Fix Governor linting issue
apbendi Feb 15, 2022
455efe8
Add public getVotesWithParams to Governor
apbendi Feb 26, 2022
8722c5c
Properly call super in GovernorWithParamsMock
apbendi Feb 26, 2022
b4cd7c4
Directly verify vote totals when testing parameterized voting
apbendi Feb 26, 2022
78c9968
Add a note about params to COUNTING_MODE documentation
apbendi Feb 26, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,13 @@
* `ERC1155`: Add a `_afterTokenTransfer` hook for improved extensibility. ([#3166](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3166))
* `DoubleEndedQueue`: a new data structure that supports efficient push and pop to both front and back, useful for FIFO and LIFO queues. ([#3153](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3153))
* `Governor`: improved security of `onlyGovernance` modifier when using an external executor contract (e.g. a timelock) that can operate without necessarily going through the governance protocol. ([#3147](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3147))
* `Governor`: Add a way to parameterize votes. This can be used to implement voting systems such as fractionalized voting, ERC721 based voting, or any number of other systems. The `params` argument added to `_countVote` method, and included in the newly added `_getVotes` method, can be used by counting and voting modules respectively for such purposes.

### Breaking changes
frangio marked this conversation as resolved.
Show resolved Hide resolved

* `Governor`: Adds internal virtual `_getVotes` method that must be implemented; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing voting module extension, rename `getVotes` to `_getVotes` and add a `bytes memory` argument. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043))
* `Governor`: Adds `params` parameter to internal virtual `_countVote ` method; this is a breaking change for existing concrete extensions to `Governor`. To fix this on an existing counting module extension, add a `bytes memory` argument to `_countVote`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043))
* `Governor`: Does not emit `VoteCast` event when params data is non-empty; instead emits `VoteCastWithParams` event. To fix this on an integration that consumes the `VoteCast` event, also fetch/monitor `VoteCastWithParams` events. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043))

## 4.5.0 (2022-02-09)

Expand Down
128 changes: 121 additions & 7 deletions contracts/governance/Governor.sol
Expand Up @@ -19,7 +19,7 @@ import "./IGovernor.sol";
* This contract is abstract and requires several function to be implemented in various modules:
*
* - A counting module must implement {quorum}, {_quorumReached}, {_voteSucceeded} and {_countVote}
* - A voting module must implement {getVotes}
* - A voting module must implement {_getVotes}
* - Additionanly, the {votingPeriod} must also be implemented
*
* _Available since v4.3._
Expand All @@ -30,6 +30,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
using Timers for Timers.BlockNumber;

bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)");
bytes32 public constant EXTENDED_BALLOT_TYPEHASH =
keccak256("ExtendedBallot(uint256 proposalId,uint8 support,string reason,bytes params)");

struct ProposalCore {
Timers.BlockNumber voteStart;
Expand Down Expand Up @@ -86,7 +88,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) {
return interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId);
// In addition to the current interfaceId, also support previous version of the interfaceId that did not
// include the castVoteWithReasonAndParams() function as standard
return
interfaceId ==
(type(IGovernor).interfaceId ^
this.castVoteWithReasonAndParams.selector ^
this.castVoteWithReasonAndParamsBySig.selector ^
this.getVotesWithParams.selector) ||
interfaceId == type(IGovernor).interfaceId ||
super.supportsInterface(interfaceId);
}

/**
Expand Down Expand Up @@ -193,6 +204,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
*/
function _voteSucceeded(uint256 proposalId) internal view virtual returns (bool);

/**
* @dev Get the voting weight of `account` at a specific `blockNumber`, for a vote as described by `params`.
*/
function _getVotes(
address account,
uint256 blockNumber,
bytes memory params
) internal view virtual returns (uint256);

/**
* @dev Register a vote with a given support and voting weight.
*
Expand All @@ -202,9 +222,20 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
uint256 proposalId,
address account,
uint8 support,
uint256 weight
uint256 weight,
bytes memory params
frangio marked this conversation as resolved.
Show resolved Hide resolved
) internal virtual;

/**
* @dev Default additional encoded parameters used by castVote methods that don't include them
*
* Note: Should be overriden by specific implementations to use an appropriate value, the
* meaning of the additional params, in the context of that implementation
*/
function _defaultParams() internal view virtual returns (bytes memory) {
return "";
}

/**
* @dev See {IGovernor-propose}.
*/
Expand Down Expand Up @@ -355,6 +386,24 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
return proposalId;
}

/**
* @dev See {IGovernor-getVotes}.
*/
function getVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) {
frangio marked this conversation as resolved.
Show resolved Hide resolved
return _getVotes(account, blockNumber, _defaultParams());
}

/**
* @dev See {IGovernor-getVotesWithParams}.
*/
function getVotesWithParams(
address account,
uint256 blockNumber,
bytes memory params
) public view virtual override returns (uint256) {
return _getVotes(account, blockNumber, params);
}

/**
* @dev See {IGovernor-castVote}.
*/
Expand All @@ -375,6 +424,19 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
return _castVote(proposalId, voter, support, reason);
}

/**
* @dev See {IGovernor-castVoteWithReasonAndParams}.
*/
function castVoteWithReasonAndParams(
uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params
) public virtual override returns (uint256) {
address voter = _msgSender();
return _castVote(proposalId, voter, support, reason, params);
}

/**
* @dev See {IGovernor-castVoteBySig}.
*/
Expand All @@ -394,9 +456,41 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
return _castVote(proposalId, voter, support, "");
}

/**
* @dev See {IGovernor-castVoteWithReasonAndParamsBySig}.
*/
function castVoteWithReasonAndParamsBySig(
uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
) public virtual override returns (uint256) {
address voter = ECDSA.recover(
_hashTypedDataV4(
keccak256(
abi.encode(
EXTENDED_BALLOT_TYPEHASH,
proposalId,
support,
keccak256(bytes(reason)),
keccak256(params)
)
)
),
v,
r,
s
);

return _castVote(proposalId, voter, support, reason, params);
}

/**
* @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve
* voting weight using {IGovernor-getVotes} and call the {_countVote} internal function.
* voting weight using {IGovernor-getVotes} and call the {_countVote} internal function. Uses the _defaultParams().
*
* Emits a {IGovernor-VoteCast} event.
*/
Expand All @@ -405,14 +499,34 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
address account,
uint8 support,
string memory reason
) internal virtual returns (uint256) {
return _castVote(proposalId, account, support, reason, _defaultParams());
}

/**
* @dev Internal vote casting mechanism: Check that the vote is pending, that it has not been cast yet, retrieve
* voting weight using {IGovernor-getVotes} and call the {_countVote} internal function.
*
* Emits a {IGovernor-VoteCast} event.
*/
function _castVote(
uint256 proposalId,
address account,
uint8 support,
string memory reason,
bytes memory params
) internal virtual returns (uint256) {
ProposalCore storage proposal = _proposals[proposalId];
require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active");

uint256 weight = getVotes(account, proposal.voteStart.getDeadline());
_countVote(proposalId, account, support, weight);
uint256 weight = _getVotes(account, proposal.voteStart.getDeadline(), params);
_countVote(proposalId, account, support, weight, params);

emit VoteCast(account, proposalId, support, weight, reason);
if (params.length == 0) {
emit VoteCast(account, proposalId, support, weight, reason);
} else {
emit VoteCastWithParams(account, proposalId, support, weight, reason, params);
}

return weight;
}
Expand Down
64 changes: 61 additions & 3 deletions contracts/governance/IGovernor.sol
Expand Up @@ -48,12 +48,27 @@ abstract contract IGovernor is IERC165 {
event ProposalExecuted(uint256 proposalId);

/**
* @dev Emitted when a vote is cast.
* @dev Emitted when a vote is cast without params.
*
* Note: `support` values should be seen as buckets. There interpretation depends on the voting module used.
* Note: `support` values should be seen as buckets. Their interpretation depends on the voting module used.
*/
event VoteCast(address indexed voter, uint256 proposalId, uint8 support, uint256 weight, string reason);

/**
* @dev Emitted when a vote is cast with params.
*
* Note: `support` values should be seen as buckets. Their interpretation depends on the voting module used.
* `params` are additional encoded parameters. Their intepepretation also depends on the voting module used.
*/
event VoteCastWithParams(
address indexed voter,
uint256 proposalId,
uint8 support,
uint256 weight,
string reason,
bytes params
);

/**
* @notice module:core
* @dev Name of the governor instance (used in building the ERC712 domain separator).
Expand All @@ -78,6 +93,12 @@ abstract contract IGovernor is IERC165 {
* - `quorum=bravo` means that only For votes are counted towards quorum.
* - `quorum=for,abstain` means that both For and Abstain votes are counted towards quorum.
*
* If a counting module makes use of encoded `params`, it should include this under a `params` key with a unique
* name that describes the behavior. For example:
*
* - `params=fractional` might refer to a scheme where votes are divided fractionally between for/against/abstain.
* - `params=erc721` might refer to a scheme where specific NFTs are delegated to vote.
*
* NOTE: The string can be decoded by the standard
* https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams[`URLSearchParams`]
* JavaScript class.
Expand Down Expand Up @@ -151,6 +172,16 @@ abstract contract IGovernor is IERC165 {
*/
function getVotes(address account, uint256 blockNumber) public view virtual returns (uint256);

/**
* @notice module:reputation
* @dev Voting power of an `account` at a specific `blockNumber` given additional encoded parameters.
*/
function getVotesWithParams(
address account,
uint256 blockNumber,
bytes memory params
) public view virtual returns (uint256);

/**
* @notice module:voting
* @dev Returns weither `account` has cast a vote on `proposalId`.
Expand Down Expand Up @@ -204,7 +235,19 @@ abstract contract IGovernor is IERC165 {
) public virtual returns (uint256 balance);

/**
* @dev Cast a vote using the user cryptographic signature.
* @dev Cast a vote with a reason and additional encoded parameters
*
* Emits a {VoteCast} event.
*/
function castVoteWithReasonAndParams(
uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params
) public virtual returns (uint256 balance);

/**
* @dev Cast a vote using the user's cryptographic signature.
*
* Emits a {VoteCast} event.
*/
Expand All @@ -215,4 +258,19 @@ abstract contract IGovernor is IERC165 {
bytes32 r,
bytes32 s
) public virtual returns (uint256 balance);

/**
* @dev Cast a vote with a reason and additional encoded parameters using the user's cryptographic signature.
*
* Emits a {VoteCast} event.
*/
function castVoteWithReasonAndParamsBySig(
uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
) public virtual returns (uint256 balance);
}
Expand Up @@ -265,7 +265,8 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp
uint256 proposalId,
address account,
uint8 support,
uint256 weight
uint256 weight,
bytes memory // params
) internal virtual override {
ProposalDetails storage details = _proposalDetails[proposalId];
Receipt storage receipt = details.receipts[account];
Expand Down
3 changes: 2 additions & 1 deletion contracts/governance/extensions/GovernorCountingSimple.sol
Expand Up @@ -86,7 +86,8 @@ abstract contract GovernorCountingSimple is Governor {
uint256 proposalId,
address account,
uint8 support,
uint256 weight
uint256 weight,
bytes memory // params
) internal virtual override {
ProposalVote storage proposalvote = _proposalVotes[proposalId];

Expand Down
5 changes: 3 additions & 2 deletions contracts/governance/extensions/GovernorPreventLateQuorum.sol
Expand Up @@ -57,9 +57,10 @@ abstract contract GovernorPreventLateQuorum is Governor {
uint256 proposalId,
address account,
uint8 support,
string memory reason
string memory reason,
bytes memory params
) internal virtual override returns (uint256) {
uint256 result = super._castVote(proposalId, account, support, reason);
uint256 result = super._castVote(proposalId, account, support, reason, params);

Timers.BlockNumber storage extendedDeadline = _extendedDeadlines[proposalId];

Expand Down
8 changes: 6 additions & 2 deletions contracts/governance/extensions/GovernorVotes.sol
Expand Up @@ -19,9 +19,13 @@ abstract contract GovernorVotes is Governor {
}

/**
* Read the voting weight from the token's built in snapshot mechanism (see {IGovernor-getVotes}).
* Read the voting weight from the token's built in snapshot mechanism (see {Governor-_getVotes}).
*/
function getVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) {
function _getVotes(
address account,
uint256 blockNumber,
bytes memory /*params*/
) internal view virtual override returns (uint256) {
return token.getPastVotes(account, blockNumber);
}
}
8 changes: 6 additions & 2 deletions contracts/governance/extensions/GovernorVotesComp.sol
Expand Up @@ -19,9 +19,13 @@ abstract contract GovernorVotesComp is Governor {
}

/**
* Read the voting weight from the token's built in snapshot mechanism (see {IGovernor-getVotes}).
* Read the voting weight from the token's built in snapshot mechanism (see {Governor-_getVotes}).
*/
function getVotes(address account, uint256 blockNumber) public view virtual override returns (uint256) {
function _getVotes(
address account,
uint256 blockNumber,
bytes memory /*params*/
) internal view virtual override returns (uint256) {
return token.getPriorVotes(account, blockNumber);
}
}
10 changes: 0 additions & 10 deletions contracts/mocks/GovernorCompMock.sol
Expand Up @@ -28,14 +28,4 @@ contract GovernorCompMock is GovernorVotesComp, GovernorCountingSimple {
) public returns (uint256 proposalId) {
return _cancel(targets, values, calldatas, salt);
}

function getVotes(address account, uint256 blockNumber)
public
view
virtual
override(IGovernor, GovernorVotesComp)
returns (uint256)
{
return super.getVotes(account, blockNumber);
}
}