From 3bdd3ecd7b585e6bdde011c2c457a62a739e8b51 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 14 Dec 2021 16:12:42 -0500 Subject: [PATCH 01/19] Add a data parameter to the _countVote interface --- contracts/governance/Governor.sol | 44 ++++++++++++++++--- contracts/governance/IGovernor.sol | 23 ++++++++++ .../GovernorCompatibilityBravo.sol | 5 ++- .../extensions/GovernorCountingSimple.sol | 4 +- .../extensions/GovernorPreventLateQuorum.sol | 5 ++- .../mocks/GovernorPreventLateQuorumMock.sol | 5 ++- test/governance/Governor.test.js | 1 + .../GovernorTimelockCompound.test.js | 1 + .../GovernorTimelockControl.test.js | 1 + .../SupportsInterface.behavior.js | 21 +++++++++ 10 files changed, 97 insertions(+), 13 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 0da3f11706a..65c12126702 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -86,7 +86,11 @@ 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 castVoteWithData() and castVoteWithReasonAndData() functions as standard + return interfaceId == (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndData.selector ^ this.castVoteWithData.selector) + || interfaceId == type(IGovernor).interfaceId + || super.supportsInterface(interfaceId); } /** @@ -202,7 +206,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint256 proposalId, address account, uint8 support, - uint256 weight + uint256 weight, + bytes memory data ) internal virtual; /** @@ -360,7 +365,19 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function castVote(uint256 proposalId, uint8 support) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, ""); + return _castVote(proposalId, voter, support, "", ""); + } + + /** + * @dev See {IGovernor-castVoteWithData}. + */ + function castVoteWithData( + uint256 proposalId, + uint8 support, + bytes memory data + ) public virtual override returns (uint256) { + address voter = _msgSender(); + return _castVote(proposalId, voter, support, "", data); } /** @@ -370,9 +387,21 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint256 proposalId, uint8 support, string calldata reason + ) public virtual override returns (uint256) { + return castVoteWithReasonAndData(proposalId, support, reason, ""); + } + + /** + * @dev See {IGovernor-castVoteWithReasonAndData}. + */ + function castVoteWithReasonAndData( + uint256 proposalId, + uint8 support, + string calldata reason, + bytes memory data ) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, reason); + return _castVote(proposalId, voter, support, reason, data); } /** @@ -391,7 +420,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { r, s ); - return _castVote(proposalId, voter, support, ""); + return _castVote(proposalId, voter, support, "", ""); } /** @@ -404,13 +433,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint256 proposalId, address account, uint8 support, - string memory reason + string memory reason, + bytes memory data ) 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); + _countVote(proposalId, account, support, weight, data); emit VoteCast(account, proposalId, support, weight, reason); diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 3c65f40daae..7d0c22ca1cf 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -192,6 +192,17 @@ abstract contract IGovernor is IERC165 { */ function castVote(uint256 proposalId, uint8 support) public virtual returns (uint256 balance); + /** + * @dev Cast a vote with contextual data + * + * Emits a {VoteCast} event. + */ + function castVoteWithData( + uint256 proposalId, + uint8 support, + bytes memory data + ) public virtual returns (uint256 balance); + /** * @dev Cast a vote with a reason * @@ -203,6 +214,18 @@ abstract contract IGovernor is IERC165 { string calldata reason ) public virtual returns (uint256 balance); + /** + * @dev Cast a vote with a reason and contextual data + * + * Emits a {VoteCast} event. + */ + function castVoteWithReasonAndData( + uint256 proposalId, + uint8 support, + string calldata reason, + bytes memory data + ) public virtual returns (uint256 balance); + /** * @dev Cast a vote using the user cryptographic signature. * diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index cbb2200b5f6..513c68b6913 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -265,8 +265,11 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp uint256 proposalId, address account, uint8 support, - uint256 weight + uint256 weight, + bytes memory data ) internal virtual override { + (data); // silence compiler warnings + ProposalDetails storage details = _proposalDetails[proposalId]; Receipt storage receipt = details.receipts[account]; diff --git a/contracts/governance/extensions/GovernorCountingSimple.sol b/contracts/governance/extensions/GovernorCountingSimple.sol index 38054d91789..9e106cad249 100644 --- a/contracts/governance/extensions/GovernorCountingSimple.sol +++ b/contracts/governance/extensions/GovernorCountingSimple.sol @@ -86,8 +86,10 @@ abstract contract GovernorCountingSimple is Governor { uint256 proposalId, address account, uint8 support, - uint256 weight + uint256 weight, + bytes memory data ) internal virtual override { + (data); // silence compiler warnings ProposalVote storage proposalvote = _proposalVotes[proposalId]; require(!proposalvote.hasVoted[account], "GovernorVotingSimple: vote already cast"); diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index 6d96dc33d26..3ecd8dc61aa 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -57,9 +57,10 @@ abstract contract GovernorPreventLateQuorum is Governor { uint256 proposalId, address account, uint8 support, - string memory reason + string memory reason, + bytes memory data ) internal virtual override returns (uint256) { - uint256 result = super._castVote(proposalId, account, support, reason); + uint256 result = super._castVote(proposalId, account, support, reason, data); Timers.BlockNumber storage extendedDeadline = _extendedDeadlines[proposalId]; diff --git a/contracts/mocks/GovernorPreventLateQuorumMock.sol b/contracts/mocks/GovernorPreventLateQuorumMock.sol index 7de50a01f94..ea192f90ff5 100644 --- a/contracts/mocks/GovernorPreventLateQuorumMock.sol +++ b/contracts/mocks/GovernorPreventLateQuorumMock.sol @@ -53,8 +53,9 @@ contract GovernorPreventLateQuorumMock is uint256 proposalId, address account, uint8 support, - string memory reason + string memory reason, + bytes memory data ) internal virtual override(Governor, GovernorPreventLateQuorum) returns (uint256) { - return super._castVote(proposalId, account, support, reason); + return super._castVote(proposalId, account, support, reason, data); } } diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 6017db0507b..290e0bf6944 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -41,6 +41,7 @@ contract('Governor', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', + 'GovernorWithData', ]); it('deployment check', async function () { diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index bc4f15c99b5..3f566296bab 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -48,6 +48,7 @@ contract('GovernorTimelockCompound', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', + 'GovernorWithData', 'GovernorTimelock', ]); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 9efeb463518..9604504d205 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -43,6 +43,7 @@ contract('GovernorTimelockControl', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', + 'GovernorWithData', 'GovernorTimelock', ]); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 4f29f0e00d9..1915a9a2946 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -69,6 +69,27 @@ const INTERFACES = { 'castVoteWithReason(uint256,uint8,string)', 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', ], + GovernorWithData: [ + 'name()', + 'version()', + 'COUNTING_MODE()', + 'hashProposal(address[],uint256[],bytes[],bytes32)', + 'state(uint256)', + '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)', + 'castVoteWithData(uint256,uint8,bytes)', + 'castVoteWithReason(uint256,uint8,string)', + 'castVoteWithReasonAndData(uint256,uint8,string,bytes)', + 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', + ], GovernorTimelock: [ 'timelock()', 'proposalEta(uint256)', From b86cbc8ad880d23efeaa8dfbcbf98bc313b22498 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 21 Dec 2021 16:45:26 -0500 Subject: [PATCH 02/19] linter fix --- contracts/governance/Governor.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 65c12126702..62600855204 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -88,9 +88,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { // In addition to the current interfaceId, also support previous version of the interfaceId that did not // include the castVoteWithData() and castVoteWithReasonAndData() functions as standard - return interfaceId == (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndData.selector ^ this.castVoteWithData.selector) - || interfaceId == type(IGovernor).interfaceId - || super.supportsInterface(interfaceId); + return + interfaceId == + (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndData.selector ^ this.castVoteWithData.selector) || + interfaceId == type(IGovernor).interfaceId || + super.supportsInterface(interfaceId); } /** From 6aaad8483384312b91d73ce68e0540fe278c5e40 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 21 Dec 2021 18:00:58 -0500 Subject: [PATCH 03/19] Rename governor extension data extension to params --- contracts/governance/Governor.sol | 28 +++++++++---------- contracts/governance/IGovernor.sol | 12 ++++---- .../GovernorCompatibilityBravo.sol | 4 +-- .../extensions/GovernorCountingSimple.sol | 4 +-- .../extensions/GovernorPreventLateQuorum.sol | 4 +-- .../mocks/GovernorPreventLateQuorumMock.sol | 4 +-- test/governance/Governor.test.js | 2 +- .../GovernorTimelockCompound.test.js | 2 +- .../GovernorTimelockControl.test.js | 2 +- .../SupportsInterface.behavior.js | 6 ++-- 10 files changed, 34 insertions(+), 34 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 62600855204..9a7b2d8ecc0 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -87,10 +87,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { // In addition to the current interfaceId, also support previous version of the interfaceId that did not - // include the castVoteWithData() and castVoteWithReasonAndData() functions as standard + // include the castVoteWithParams() and castVoteWithReasonAndParams() functions as standard return interfaceId == - (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndData.selector ^ this.castVoteWithData.selector) || + (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndParams.selector ^ this.castVoteWithParams.selector) || interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId); } @@ -209,7 +209,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { address account, uint8 support, uint256 weight, - bytes memory data + bytes memory params ) internal virtual; /** @@ -371,15 +371,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { } /** - * @dev See {IGovernor-castVoteWithData}. + * @dev See {IGovernor-castVoteWithParams}. */ - function castVoteWithData( + function castVoteWithParams( uint256 proposalId, uint8 support, - bytes memory data + bytes memory params ) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, "", data); + return _castVote(proposalId, voter, support, "", params); } /** @@ -390,20 +390,20 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint8 support, string calldata reason ) public virtual override returns (uint256) { - return castVoteWithReasonAndData(proposalId, support, reason, ""); + return castVoteWithReasonAndParams(proposalId, support, reason, ""); } /** - * @dev See {IGovernor-castVoteWithReasonAndData}. + * @dev See {IGovernor-castVoteWithReasonAndParams}. */ - function castVoteWithReasonAndData( + function castVoteWithReasonAndParams( uint256 proposalId, uint8 support, string calldata reason, - bytes memory data + bytes memory params ) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, reason, data); + return _castVote(proposalId, voter, support, reason, params); } /** @@ -436,13 +436,13 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { address account, uint8 support, string memory reason, - bytes memory data + 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, data); + _countVote(proposalId, account, support, weight, params); emit VoteCast(account, proposalId, support, weight, reason); diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 7d0c22ca1cf..32777d5831a 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -193,14 +193,14 @@ abstract contract IGovernor is IERC165 { function castVote(uint256 proposalId, uint8 support) public virtual returns (uint256 balance); /** - * @dev Cast a vote with contextual data + * @dev Cast a vote with additional encoded parameters * * Emits a {VoteCast} event. */ - function castVoteWithData( + function castVoteWithParams( uint256 proposalId, uint8 support, - bytes memory data + bytes memory params ) public virtual returns (uint256 balance); /** @@ -215,15 +215,15 @@ abstract contract IGovernor is IERC165 { ) public virtual returns (uint256 balance); /** - * @dev Cast a vote with a reason and contextual data + * @dev Cast a vote with a reason and additional encoded parameters * * Emits a {VoteCast} event. */ - function castVoteWithReasonAndData( + function castVoteWithReasonAndParams( uint256 proposalId, uint8 support, string calldata reason, - bytes memory data + bytes memory params ) public virtual returns (uint256 balance); /** diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 513c68b6913..5648744463d 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -266,9 +266,9 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp address account, uint8 support, uint256 weight, - bytes memory data + bytes memory params ) internal virtual override { - (data); // silence compiler warnings + (params); // silence compiler warnings ProposalDetails storage details = _proposalDetails[proposalId]; Receipt storage receipt = details.receipts[account]; diff --git a/contracts/governance/extensions/GovernorCountingSimple.sol b/contracts/governance/extensions/GovernorCountingSimple.sol index 9e106cad249..117ae3953e9 100644 --- a/contracts/governance/extensions/GovernorCountingSimple.sol +++ b/contracts/governance/extensions/GovernorCountingSimple.sol @@ -87,9 +87,9 @@ abstract contract GovernorCountingSimple is Governor { address account, uint8 support, uint256 weight, - bytes memory data + bytes memory params ) internal virtual override { - (data); // silence compiler warnings + (params); // silence compiler warnings ProposalVote storage proposalvote = _proposalVotes[proposalId]; require(!proposalvote.hasVoted[account], "GovernorVotingSimple: vote already cast"); diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index 3ecd8dc61aa..5b58f6032bf 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -58,9 +58,9 @@ abstract contract GovernorPreventLateQuorum is Governor { address account, uint8 support, string memory reason, - bytes memory data + bytes memory params ) internal virtual override returns (uint256) { - uint256 result = super._castVote(proposalId, account, support, reason, data); + uint256 result = super._castVote(proposalId, account, support, reason, params); Timers.BlockNumber storage extendedDeadline = _extendedDeadlines[proposalId]; diff --git a/contracts/mocks/GovernorPreventLateQuorumMock.sol b/contracts/mocks/GovernorPreventLateQuorumMock.sol index ea192f90ff5..35bddc03b42 100644 --- a/contracts/mocks/GovernorPreventLateQuorumMock.sol +++ b/contracts/mocks/GovernorPreventLateQuorumMock.sol @@ -54,8 +54,8 @@ contract GovernorPreventLateQuorumMock is address account, uint8 support, string memory reason, - bytes memory data + bytes memory params ) internal virtual override(Governor, GovernorPreventLateQuorum) returns (uint256) { - return super._castVote(proposalId, account, support, reason, data); + return super._castVote(proposalId, account, support, reason, params); } } diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 290e0bf6944..7156abeabff 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -41,7 +41,7 @@ contract('Governor', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', - 'GovernorWithData', + 'GovernorWithParams', ]); it('deployment check', async function () { diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 3f566296bab..e623c4571dd 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -48,7 +48,7 @@ contract('GovernorTimelockCompound', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', - 'GovernorWithData', + 'GovernorWithParams', 'GovernorTimelock', ]); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 9604504d205..ade1c545a7c 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -43,7 +43,7 @@ contract('GovernorTimelockControl', function (accounts) { shouldSupportInterfaces([ 'ERC165', 'Governor', - 'GovernorWithData', + 'GovernorWithParams', 'GovernorTimelock', ]); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 1915a9a2946..054b58452b2 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -69,7 +69,7 @@ const INTERFACES = { 'castVoteWithReason(uint256,uint8,string)', 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', ], - GovernorWithData: [ + GovernorWithParams: [ 'name()', 'version()', 'COUNTING_MODE()', @@ -85,9 +85,9 @@ const INTERFACES = { 'propose(address[],uint256[],bytes[],string)', 'execute(address[],uint256[],bytes[],bytes32)', 'castVote(uint256,uint8)', - 'castVoteWithData(uint256,uint8,bytes)', + 'castVoteWithParams(uint256,uint8,bytes)', 'castVoteWithReason(uint256,uint8,string)', - 'castVoteWithReasonAndData(uint256,uint8,string,bytes)', + 'castVoteWithReasonAndParams(uint256,uint8,string,bytes)', 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', ], GovernorTimelock: [ From 287aa4fe70fd7b23ecb34a238fd6c59a170db272 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Wed, 22 Dec 2021 10:16:43 -0500 Subject: [PATCH 04/19] Remove castVoteWithParams without reason parameter --- contracts/governance/Governor.sol | 17 ++--------------- contracts/governance/IGovernor.sol | 11 ----------- .../introspection/SupportsInterface.behavior.js | 1 - 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 9a7b2d8ecc0..37713779855 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -87,10 +87,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { // In addition to the current interfaceId, also support previous version of the interfaceId that did not - // include the castVoteWithParams() and castVoteWithReasonAndParams() functions as standard + // include the castVoteWithReasonAndParams() function as standard return - interfaceId == - (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndParams.selector ^ this.castVoteWithParams.selector) || + interfaceId == (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndParams.selector) || interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId); } @@ -370,18 +369,6 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return _castVote(proposalId, voter, support, "", ""); } - /** - * @dev See {IGovernor-castVoteWithParams}. - */ - function castVoteWithParams( - uint256 proposalId, - uint8 support, - bytes memory params - ) public virtual override returns (uint256) { - address voter = _msgSender(); - return _castVote(proposalId, voter, support, "", params); - } - /** * @dev See {IGovernor-castVoteWithReason}. */ diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 32777d5831a..6991ca7587e 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -192,17 +192,6 @@ abstract contract IGovernor is IERC165 { */ function castVote(uint256 proposalId, uint8 support) public virtual returns (uint256 balance); - /** - * @dev Cast a vote with additional encoded parameters - * - * Emits a {VoteCast} event. - */ - function castVoteWithParams( - uint256 proposalId, - uint8 support, - bytes memory params - ) public virtual returns (uint256 balance); - /** * @dev Cast a vote with a reason * diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 054b58452b2..5c1b6d4f02d 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -85,7 +85,6 @@ const INTERFACES = { 'propose(address[],uint256[],bytes[],string)', 'execute(address[],uint256[],bytes[],bytes32)', 'castVote(uint256,uint8)', - 'castVoteWithParams(uint256,uint8,bytes)', 'castVoteWithReason(uint256,uint8,string)', 'castVoteWithReasonAndParams(uint256,uint8,string,bytes)', 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', From 92e939ffe222884ba36f0321ec0ed22c6713c271 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Wed, 22 Dec 2021 10:26:28 -0500 Subject: [PATCH 05/19] Add a virtual method specifying default additional voting params --- contracts/governance/Governor.sol | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 37713779855..bdb047ed4f0 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -211,6 +211,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { bytes memory params ) 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}. */ @@ -366,7 +376,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function castVote(uint256 proposalId, uint8 support) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, "", ""); + return _castVote(proposalId, voter, support, "", _defaultParams()); } /** @@ -377,7 +387,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint8 support, string calldata reason ) public virtual override returns (uint256) { - return castVoteWithReasonAndParams(proposalId, support, reason, ""); + return castVoteWithReasonAndParams(proposalId, support, reason, _defaultParams()); } /** @@ -409,7 +419,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { r, s ); - return _castVote(proposalId, voter, support, "", ""); + return _castVote(proposalId, voter, support, "", _defaultParams()); } /** From b7eb845bbab2cab1399dec4ad087feba2f3cd53d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Jan 2022 16:35:17 +0100 Subject: [PATCH 06/19] Extend parameterized Governor customization to include vote weight calculation --- contracts/governance/Governor.sol | 18 +++++++++++++++++- .../governance/extensions/GovernorVotes.sol | 8 ++++++-- .../extensions/GovernorVotesComp.sol | 8 ++++++-- contracts/mocks/GovernorCompMock.sol | 10 ---------- .../mocks/GovernorCompatibilityBravoMock.sol | 10 ---------- contracts/mocks/GovernorMock.sol | 10 ---------- .../mocks/GovernorTimelockCompoundMock.sol | 10 ---------- .../mocks/GovernorTimelockControlMock.sol | 10 ---------- contracts/mocks/GovernorVoteMock.sol | 10 ---------- contracts/mocks/wizard/MyGovernor1.sol | 9 --------- contracts/mocks/wizard/MyGovernor2.sol | 9 --------- contracts/mocks/wizard/MyGovernor3.sol | 9 --------- 12 files changed, 29 insertions(+), 92 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index bdb047ed4f0..eaa1256d9d9 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -198,6 +198,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function _voteSucceeded(uint256 proposalId) internal view virtual returns (bool); + /** + * @dev Check the voting weight of an account. + */ + 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. * @@ -371,6 +380,13 @@ 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) { + return _getVotes(account, blockNumber, _defaultParams()); + } + /** * @dev See {IGovernor-castVote}. */ @@ -438,7 +454,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { ProposalCore storage proposal = _proposals[proposalId]; require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active"); - uint256 weight = getVotes(account, proposal.voteStart.getDeadline()); + uint256 weight = _getVotes(account, proposal.voteStart.getDeadline(), params); _countVote(proposalId, account, support, weight, params); emit VoteCast(account, proposalId, support, weight, reason); diff --git a/contracts/governance/extensions/GovernorVotes.sol b/contracts/governance/extensions/GovernorVotes.sol index 75f56356f3c..f0a2276fb38 100644 --- a/contracts/governance/extensions/GovernorVotes.sol +++ b/contracts/governance/extensions/GovernorVotes.sol @@ -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); } } diff --git a/contracts/governance/extensions/GovernorVotesComp.sol b/contracts/governance/extensions/GovernorVotesComp.sol index 9eb87a35fa4..c31c9583b5a 100644 --- a/contracts/governance/extensions/GovernorVotesComp.sol +++ b/contracts/governance/extensions/GovernorVotesComp.sol @@ -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); } } diff --git a/contracts/mocks/GovernorCompMock.sol b/contracts/mocks/GovernorCompMock.sol index 9dcbc536d0e..c2d8733e032 100644 --- a/contracts/mocks/GovernorCompMock.sol +++ b/contracts/mocks/GovernorCompMock.sol @@ -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); - } } diff --git a/contracts/mocks/GovernorCompatibilityBravoMock.sol b/contracts/mocks/GovernorCompatibilityBravoMock.sol index 60afbb918e9..8f295f62faa 100644 --- a/contracts/mocks/GovernorCompatibilityBravoMock.sol +++ b/contracts/mocks/GovernorCompatibilityBravoMock.sol @@ -124,16 +124,6 @@ contract GovernorCompatibilityBravoMock is return super._cancel(targets, values, calldatas, salt); } - function getVotes(address account, uint256 blockNumber) - public - view - virtual - override(IGovernor, GovernorVotesComp) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function _executor() internal view virtual override(Governor, GovernorTimelockCompound) returns (address) { return super._executor(); } diff --git a/contracts/mocks/GovernorMock.sol b/contracts/mocks/GovernorMock.sol index 85233f55951..dafb0a0a7a5 100644 --- a/contracts/mocks/GovernorMock.sol +++ b/contracts/mocks/GovernorMock.sol @@ -35,16 +35,6 @@ contract GovernorMock is return _cancel(targets, values, calldatas, salt); } - function getVotes(address account, uint256 blockNumber) - public - view - virtual - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { return super.proposalThreshold(); } diff --git a/contracts/mocks/GovernorTimelockCompoundMock.sol b/contracts/mocks/GovernorTimelockCompoundMock.sol index aeba5b86a80..88a8829280c 100644 --- a/contracts/mocks/GovernorTimelockCompoundMock.sol +++ b/contracts/mocks/GovernorTimelockCompoundMock.sol @@ -92,16 +92,6 @@ contract GovernorTimelockCompoundMock is return super._cancel(targets, values, calldatas, salt); } - function getVotes(address account, uint256 blockNumber) - public - view - virtual - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function _executor() internal view virtual override(Governor, GovernorTimelockCompound) returns (address) { return super._executor(); } diff --git a/contracts/mocks/GovernorTimelockControlMock.sol b/contracts/mocks/GovernorTimelockControlMock.sol index 6150b7b3c6d..455eac9bacc 100644 --- a/contracts/mocks/GovernorTimelockControlMock.sol +++ b/contracts/mocks/GovernorTimelockControlMock.sol @@ -92,16 +92,6 @@ contract GovernorTimelockControlMock is return super._cancel(targets, values, calldatas, descriptionHash); } - function getVotes(address account, uint256 blockNumber) - public - view - virtual - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function _executor() internal view virtual override(Governor, GovernorTimelockControl) returns (address) { return super._executor(); } diff --git a/contracts/mocks/GovernorVoteMock.sol b/contracts/mocks/GovernorVoteMock.sol index 23ccf6bc09c..60a3d41355e 100644 --- a/contracts/mocks/GovernorVoteMock.sol +++ b/contracts/mocks/GovernorVoteMock.sol @@ -28,14 +28,4 @@ contract GovernorVoteMocks is GovernorVotes, GovernorCountingSimple { ) public returns (uint256 proposalId) { return _cancel(targets, values, calldatas, salt); } - - function getVotes(address account, uint256 blockNumber) - public - view - virtual - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } } diff --git a/contracts/mocks/wizard/MyGovernor1.sol b/contracts/mocks/wizard/MyGovernor1.sol index bd524ee55a2..a80d8400c5e 100644 --- a/contracts/mocks/wizard/MyGovernor1.sol +++ b/contracts/mocks/wizard/MyGovernor1.sol @@ -40,15 +40,6 @@ contract MyGovernor1 is return super.quorum(blockNumber); } - function getVotes(address account, uint256 blockNumber) - public - view - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function state(uint256 proposalId) public view override(Governor, GovernorTimelockControl) returns (ProposalState) { return super.state(proposalId); } diff --git a/contracts/mocks/wizard/MyGovernor2.sol b/contracts/mocks/wizard/MyGovernor2.sol index 3a5c983e0b3..34c608c5cc2 100644 --- a/contracts/mocks/wizard/MyGovernor2.sol +++ b/contracts/mocks/wizard/MyGovernor2.sol @@ -46,15 +46,6 @@ contract MyGovernor2 is return super.quorum(blockNumber); } - function getVotes(address account, uint256 blockNumber) - public - view - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function state(uint256 proposalId) public view override(Governor, GovernorTimelockControl) returns (ProposalState) { return super.state(proposalId); } diff --git a/contracts/mocks/wizard/MyGovernor3.sol b/contracts/mocks/wizard/MyGovernor3.sol index 835a893a3a4..70e4e87f046 100644 --- a/contracts/mocks/wizard/MyGovernor3.sol +++ b/contracts/mocks/wizard/MyGovernor3.sol @@ -44,15 +44,6 @@ contract MyGovernor is return super.quorum(blockNumber); } - function getVotes(address account, uint256 blockNumber) - public - view - override(IGovernor, GovernorVotes) - returns (uint256) - { - return super.getVotes(account, blockNumber); - } - function state(uint256 proposalId) public view From ead0d51b8f12b9dfb659685809a852068e5e3d25 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Mon, 17 Jan 2022 10:28:24 -0500 Subject: [PATCH 07/19] Mock and test usage of Govenance extension params --- contracts/mocks/GovernorWithParamsMock.sol | 61 ++++++++ .../extensions/GovernorWithParams.test.js | 146 ++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 contracts/mocks/GovernorWithParamsMock.sol create mode 100644 test/governance/extensions/GovernorWithParams.test.js diff --git a/contracts/mocks/GovernorWithParamsMock.sol b/contracts/mocks/GovernorWithParamsMock.sol new file mode 100644 index 00000000000..0b3dc133906 --- /dev/null +++ b/contracts/mocks/GovernorWithParamsMock.sol @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../governance/extensions/GovernorCountingSimple.sol"; +import "../governance/extensions/GovernorVotes.sol"; + +contract GovernorWithParamsMock is GovernorVotes, GovernorCountingSimple { + event CountParams(uint256 uintParam, string strParam); + + constructor(string memory name_, IVotes token_) Governor(name_) GovernorVotes(token_) {} + + function quorum(uint256) public pure override returns (uint256) { + return 0; + } + + function votingDelay() public pure override returns (uint256) { + return 4; + } + + function votingPeriod() public pure override returns (uint256) { + return 16; + } + + function _getVotes( + address account, + uint256 blockNumber, + bytes memory params + ) internal view virtual override(Governor, GovernorVotes) returns (uint256) { + uint256 reduction = 0; + // If the user provides parameters, we reduce the voting weight by the amount of the integer param + if (params.length > 0) { + (reduction, ) = abi.decode(params, (uint256, string)); + } + // reverts on overflow + return GovernorVotes._getVotes(account, blockNumber, params) - reduction; + } + + function _countVote( + uint256 proposalId, + address account, + uint8 support, + uint256 weight, + bytes memory params + ) internal virtual override(Governor, GovernorCountingSimple) { + if (params.length > 0) { + (uint256 _uintParam, string memory _strParam) = abi.decode(params, (uint256, string)); + emit CountParams(_uintParam, _strParam); + } + return GovernorCountingSimple._countVote(proposalId, account, support, weight, params); + } + + function cancel( + address[] memory targets, + uint256[] memory values, + bytes[] memory calldatas, + bytes32 salt + ) public returns (uint256 proposalId) { + return _cancel(targets, values, calldatas, salt); + } +} diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js new file mode 100644 index 00000000000..14dfbaf35de --- /dev/null +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -0,0 +1,146 @@ +const { BN, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { web3 } = require('@openzeppelin/test-helpers/src/setup'); +const Enums = require('../../helpers/enums'); + +const { runGovernorWorkflow } = require('../GovernorWorkflow.behavior'); + +const Token = artifacts.require('ERC20VotesCompMock'); +const Governor = artifacts.require('GovernorWithParamsMock'); +const CallReceiver = artifacts.require('CallReceiverMock'); + +contract('GovernorWithParams', function (accounts) { + const [owner, proposer, voter1, voter2, voter3, voter4] = accounts; + + const name = 'OZ-Governor'; + const tokenName = 'MockToken'; + const tokenSymbol = 'MTKN'; + const tokenSupply = web3.utils.toWei('100'); + const votingDelay = new BN(4); + const votingPeriod = new BN(16); + + beforeEach(async function () { + this.owner = owner; + this.token = await Token.new(tokenName, tokenSymbol); + this.mock = await Governor.new(name, this.token.address); + this.receiver = await CallReceiver.new(); + await this.token.mint(owner, tokenSupply); + await this.token.delegate(voter1, { from: voter1 }); + await this.token.delegate(voter2, { from: voter2 }); + await this.token.delegate(voter3, { from: voter3 }); + await this.token.delegate(voter4, { from: voter4 }); + }); + + 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); + }); + + describe('nominal is unaffected', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + [this.receiver.address], + [0], + [this.receiver.contract.methods.mockFunction().encodeABI()], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('1'), support: Enums.VoteType.For, reason: 'This is nice' }, + { voter: voter2, weight: web3.utils.toWei('7'), support: Enums.VoteType.For }, + { voter: voter3, weight: web3.utils.toWei('5'), support: Enums.VoteType.Against }, + { voter: voter4, weight: web3.utils.toWei('2'), support: Enums.VoteType.Abstain }, + ], + }; + }); + + afterEach(async function () { + expect(await this.mock.hasVoted(this.id, owner)).to.be.equal(false); + expect(await this.mock.hasVoted(this.id, voter1)).to.be.equal(true); + expect(await this.mock.hasVoted(this.id, voter2)).to.be.equal(true); + + await this.mock.proposalVotes(this.id).then((result) => { + for (const [key, value] of Object.entries(Enums.VoteType)) { + expect(result[`${key.toLowerCase()}Votes`]).to.be.bignumber.equal( + Object.values(this.settings.voters) + .filter(({ support }) => support === value) + .reduce((acc, { weight }) => acc.add(new BN(weight)), new BN('0')), + ); + } + }); + + const startBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay); + const endBlock = new BN(this.receipts.propose.blockNumber).add(votingDelay).add(votingPeriod); + expect(await this.mock.proposalSnapshot(this.id)).to.be.bignumber.equal(startBlock); + expect(await this.mock.proposalDeadline(this.id)).to.be.bignumber.equal(endBlock); + + expectEvent(this.receipts.propose, 'ProposalCreated', { + proposalId: this.id, + proposer, + targets: this.settings.proposal[0], + // values: this.settings.proposal[1].map(value => new BN(value)), + signatures: this.settings.proposal[2].map(() => ''), + calldatas: this.settings.proposal[2], + startBlock, + endBlock, + description: this.settings.proposal[3], + }); + + this.receipts.castVote.filter(Boolean).forEach((vote) => { + const { voter } = vote.logs.filter(({ event }) => event === 'VoteCast').find(Boolean).args; + expectEvent( + vote, + 'VoteCast', + this.settings.voters.find(({ address }) => address === voter), + ); + }); + expectEvent(this.receipts.execute, 'ProposalExecuted', { proposalId: this.id }); + await expectEvent.inTransaction(this.receipts.execute.transactionHash, this.receiver, 'MockFunctionCalled'); + }); + runGovernorWorkflow(); + }); + + describe('Voting with params is properly supported', function () { + const voter2Weight = web3.utils.toWei('1.0'); + beforeEach(async function () { + this.settings = { + proposal: [ + [this.receiver.address], + [0], + [this.receiver.contract.methods.mockFunction().encodeABI()], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('0.2'), support: Enums.VoteType.Against }, + { voter: voter2, weight: voter2Weight }, // do not actually vote, only getting tokens + { voter: voter3, weight: web3.utils.toWei('0.9') }, // do not actually vote, only getting tokens + ], + steps: { + wait: { enable: false }, + execute: { enable: false }, + }, + }; + }); + + afterEach(async function () { + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); + + const uintParam = new BN(1); + const strParam = 'These are my params'; + const reducedWeight = (new BN(voter2Weight)).sub(uintParam); + const params = web3.eth.abi.encodeParameters(['uint256', 'string'], [uintParam, strParam]); + const tx = await this.mock.castVoteWithReasonAndParams(this.id, Enums.VoteType.For, '', params, { from: voter2 }); + + expectEvent(tx, 'CountParams', { uintParam, strParam }); + expectEvent(tx, 'VoteCast', {voter: voter2, weight: reducedWeight}); + + // TODO: Cast vote with voter3 using params & signature; confirm events exist in tx receipt + }); + runGovernorWorkflow(); + }); +}); From 0b49a653d3e3507154e47c38e3140f1cfdf93757 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Mon, 17 Jan 2022 12:36:49 -0500 Subject: [PATCH 08/19] Implement and test Govenor castVoteWithReasonAndParamsBySig method --- contracts/governance/Governor.sol | 39 ++++++- contracts/governance/IGovernor.sol | 17 ++- .../extensions/GovernorWithParams.test.js | 101 ++++++++++++++++-- .../SupportsInterface.behavior.js | 1 + 4 files changed, 149 insertions(+), 9 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index eaa1256d9d9..4cbd8909d4d 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -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; @@ -89,7 +91,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { // 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) || + interfaceId == + (type(IGovernor).interfaceId ^ + this.castVoteWithReasonAndParams.selector ^ + this.castVoteWithReasonAndParamsBySig.selector) || interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId); } @@ -438,6 +443,38 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return _castVote(proposalId, voter, support, "", _defaultParams()); } + /** + * @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. diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 6991ca7587e..d975d2c99e3 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -216,7 +216,7 @@ abstract contract IGovernor is IERC165 { ) public virtual returns (uint256 balance); /** - * @dev Cast a vote using the user cryptographic signature. + * @dev Cast a vote using the user's cryptographic signature. * * Emits a {VoteCast} event. */ @@ -227,4 +227,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); } diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 14dfbaf35de..6afb7dd5ed5 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -1,6 +1,10 @@ -const { BN, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { BN, constants, expectEvent } = require('@openzeppelin/test-helpers'); const { web3 } = require('@openzeppelin/test-helpers/src/setup'); const Enums = require('../../helpers/enums'); +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { EIP712Domain } = require('../../helpers/eip712'); +const { fromRpcSig } = require('ethereumjs-util'); const { runGovernorWorkflow } = require('../GovernorWorkflow.behavior'); @@ -12,6 +16,7 @@ contract('GovernorWithParams', function (accounts) { const [owner, proposer, voter1, voter2, voter3, voter4] = accounts; const name = 'OZ-Governor'; + const version = '1'; const tokenName = 'MockToken'; const tokenSymbol = 'MTKN'; const tokenSupply = web3.utils.toWei('100'); @@ -104,7 +109,7 @@ contract('GovernorWithParams', function (accounts) { }); describe('Voting with params is properly supported', function () { - const voter2Weight = web3.utils.toWei('1.0'); + const voter2Weight = web3.utils.toWei('1.0'); beforeEach(async function () { this.settings = { proposal: [ @@ -117,8 +122,7 @@ contract('GovernorWithParams', function (accounts) { tokenHolder: owner, voters: [ { voter: voter1, weight: web3.utils.toWei('0.2'), support: Enums.VoteType.Against }, - { voter: voter2, weight: voter2Weight }, // do not actually vote, only getting tokens - { voter: voter3, weight: web3.utils.toWei('0.9') }, // do not actually vote, only getting tokens + { voter: voter2, weight: voter2Weight }, // do not actually vote, only getting tokenss ], steps: { wait: { enable: false }, @@ -132,14 +136,97 @@ contract('GovernorWithParams', function (accounts) { const uintParam = new BN(1); const strParam = 'These are my params'; - const reducedWeight = (new BN(voter2Weight)).sub(uintParam); + const reducedWeight = new BN(voter2Weight).sub(uintParam); const params = web3.eth.abi.encodeParameters(['uint256', 'string'], [uintParam, strParam]); const tx = await this.mock.castVoteWithReasonAndParams(this.id, Enums.VoteType.For, '', params, { from: voter2 }); expectEvent(tx, 'CountParams', { uintParam, strParam }); - expectEvent(tx, 'VoteCast', {voter: voter2, weight: reducedWeight}); + expectEvent(tx, 'VoteCast', { voter: voter2, weight: reducedWeight }); + }); + runGovernorWorkflow(); + }); + + describe('Voting with params by signature is propoerly supported', function () { + const voterBySig = Wallet.generate(); // generate voter by signature wallet + const sigVoterWeight = web3.utils.toWei('1.0'); + + beforeEach(async function () { + this.chainId = await web3.eth.getChainId(); + this.voter = web3.utils.toChecksumAddress(voterBySig.getAddressString()); + + // use delegateBySig to enable vote delegation sig voting wallet + const { v, r, s } = fromRpcSig( + ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { + data: { + types: { + EIP712Domain, + Delegation: [ + { name: 'delegatee', type: 'address' }, + { name: 'nonce', type: 'uint256' }, + { name: 'expiry', type: 'uint256' }, + ], + }, + domain: { name: tokenName, version: '1', chainId: this.chainId, verifyingContract: this.token.address }, + primaryType: 'Delegation', + message: { delegatee: this.voter, nonce: 0, expiry: constants.MAX_UINT256 }, + }, + }), + ); + await this.token.delegateBySig(this.voter, 0, constants.MAX_UINT256, v, r, s); + + this.settings = { + proposal: [ + [this.receiver.address], + [0], + [this.receiver.contract.methods.mockFunction().encodeABI()], + '', + ], + proposer, + tokenHolder: owner, + voters: [ + { voter: voter1, weight: web3.utils.toWei('0.2'), support: Enums.VoteType.Against }, + { voter: this.voter, weight: sigVoterWeight }, // do not actually vote, only getting tokens + ], + steps: { + wait: { enable: false }, + execute: { enable: false }, + }, + }; + }); - // TODO: Cast vote with voter3 using params & signature; confirm events exist in tx receipt + afterEach(async function () { + expect(await this.mock.state(this.id)).to.be.bignumber.equal(Enums.ProposalState.Active); + + const reason = 'This is my reason'; + const uintParam = new BN(1); + const strParam = 'These are my params'; + const reducedWeight = new BN(sigVoterWeight).sub(uintParam); + const params = web3.eth.abi.encodeParameters(['uint256', 'string'], [uintParam, strParam]); + + // prepare signature for vote by signature + const { v, r, s } = fromRpcSig( + ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { + data: { + types: { + EIP712Domain, + ExtendedBallot: [ + { name: 'proposalId', type: 'uint256' }, + { name: 'support', type: 'uint8' }, + { name: 'reason', type: 'string' }, + { name: 'params', type: 'bytes' }, + ], + }, + domain: { name, version, chainId: this.chainId, verifyingContract: this.mock.address }, + primaryType: 'ExtendedBallot', + message: { proposalId: this.id, support: Enums.VoteType.For, reason, params }, + }, + }), + ); + + const tx = await this.mock.castVoteWithReasonAndParamsBySig(this.id, Enums.VoteType.For, reason, params, v, r, s); + + expectEvent(tx, 'CountParams', { uintParam, strParam }); + expectEvent(tx, 'VoteCast', { voter: this.voter, weight: reducedWeight }); }); runGovernorWorkflow(); }); diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 5c1b6d4f02d..20281b2101a 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -88,6 +88,7 @@ const INTERFACES = { 'castVoteWithReason(uint256,uint8,string)', 'castVoteWithReasonAndParams(uint256,uint8,string,bytes)', 'castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)', + 'castVoteWithReasonAndParamsBySig(uint256,uint8,string,bytes,uint8,bytes32,bytes32)', ], GovernorTimelock: [ 'timelock()', From 0d5f887a94b2d91ddfb8e84be638354e99c5ccf8 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Thu, 10 Feb 2022 11:17:54 -0500 Subject: [PATCH 09/19] Add VoteCastWithParams event emitted when params are included --- contracts/governance/Governor.sol | 6 +++++- contracts/governance/IGovernor.sol | 19 +++++++++++++++++-- .../extensions/GovernorWithParams.test.js | 6 +++--- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 4cbd8909d4d..215f0118a25 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -494,7 +494,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { 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; } diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index d975d2c99e3..5eb08d9848b 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -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). diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index 6afb7dd5ed5..eca66a28ec1 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -141,12 +141,12 @@ contract('GovernorWithParams', function (accounts) { const tx = await this.mock.castVoteWithReasonAndParams(this.id, Enums.VoteType.For, '', params, { from: voter2 }); expectEvent(tx, 'CountParams', { uintParam, strParam }); - expectEvent(tx, 'VoteCast', { voter: voter2, weight: reducedWeight }); + expectEvent(tx, 'VoteCastWithParams', { voter: voter2, weight: reducedWeight, params }); }); runGovernorWorkflow(); }); - describe('Voting with params by signature is propoerly supported', function () { + describe('Voting with params by signature is properly supported', function () { const voterBySig = Wallet.generate(); // generate voter by signature wallet const sigVoterWeight = web3.utils.toWei('1.0'); @@ -226,7 +226,7 @@ contract('GovernorWithParams', function (accounts) { const tx = await this.mock.castVoteWithReasonAndParamsBySig(this.id, Enums.VoteType.For, reason, params, v, r, s); expectEvent(tx, 'CountParams', { uintParam, strParam }); - expectEvent(tx, 'VoteCast', { voter: this.voter, weight: reducedWeight }); + expectEvent(tx, 'VoteCastWithParams', { voter: this.voter, weight: reducedWeight, params }); }); runGovernorWorkflow(); }); From d08f61afa9af7051224d1356f040ebf0ffedcce7 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Thu, 10 Feb 2022 11:42:38 -0500 Subject: [PATCH 10/19] Document changes associated with Governor extension params --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 950e6b24926..5fedc773238 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ * `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)) + * `IGovernor`: Add two new methods to the interface, `castVoteWithReasonAndParams` and `castVoteWithReasonAndParamsBySig`, allowing voting extensions to include more data about how the vote is to be interpreted. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + * `IGovernor`: Add a new event `VoteCastWithParams` that is emitted when a vote is cast that includes non-empty params data. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + * `Governor`: Add internal `_defaultParams` method and default implementation. The return value is passed as params data when `castVote` methods without params are called. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + * `Governor`: Add default `getVotes` implementation which calls virtual internal `_getVotes` method. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + * `Governor`: Add `EXTENDED_BALLOT_TYPEHASH` used for signature validation in `castVoteWithReasonAndParamsBySig`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + +### Breaking changes + +* `Governor`: Adds internal virtual `_getVotes` method that must be implemented; this is a breaking change for existing concrete extensions to `Governor`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) +* `Governor`: Adds `params` parameter to internal virtual `_countVote ` method that; this is a breaking change for existing concrete extensions to `Governor`. ([#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. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) ## 4.5.0 (2022-02-09) From a49eb41c87e39e26ca5c24c7e8a38516597b6a99 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 15 Feb 2022 16:08:45 -0500 Subject: [PATCH 11/19] Overload _castVote and curry using default parameters --- contracts/governance/Governor.sol | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 215f0118a25..f01fff2743e 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -397,7 +397,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { */ function castVote(uint256 proposalId, uint8 support) public virtual override returns (uint256) { address voter = _msgSender(); - return _castVote(proposalId, voter, support, "", _defaultParams()); + return _castVote(proposalId, voter, support, ""); } /** @@ -408,7 +408,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint8 support, string calldata reason ) public virtual override returns (uint256) { - return castVoteWithReasonAndParams(proposalId, support, reason, _defaultParams()); + address voter = _msgSender(); + return _castVote(proposalId, voter, support, reason); } /** @@ -440,7 +441,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { r, s ); - return _castVote(proposalId, voter, support, "", _defaultParams()); + return _castVote(proposalId, voter, support, ""); } /** @@ -474,6 +475,20 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { 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. Uses the _defaultParams(). + * + * Emits a {IGovernor-VoteCast} event. + */ + function _castVote( + uint256 proposalId, + 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 From 0823a4141aabf3719d6608623f2d365eaed75443 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 15 Feb 2022 16:14:54 -0500 Subject: [PATCH 12/19] Silence compiler warnings by removing parameter name --- .../governance/compatibility/GovernorCompatibilityBravo.sol | 4 +--- contracts/governance/extensions/GovernorCountingSimple.sol | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 5648744463d..1d639f436a0 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -266,10 +266,8 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp address account, uint8 support, uint256 weight, - bytes memory params + bytes memory // params ) internal virtual override { - (params); // silence compiler warnings - ProposalDetails storage details = _proposalDetails[proposalId]; Receipt storage receipt = details.receipts[account]; diff --git a/contracts/governance/extensions/GovernorCountingSimple.sol b/contracts/governance/extensions/GovernorCountingSimple.sol index 117ae3953e9..a262ce4d836 100644 --- a/contracts/governance/extensions/GovernorCountingSimple.sol +++ b/contracts/governance/extensions/GovernorCountingSimple.sol @@ -87,9 +87,8 @@ abstract contract GovernorCountingSimple is Governor { address account, uint8 support, uint256 weight, - bytes memory params + bytes memory // params ) internal virtual override { - (params); // silence compiler warnings ProposalVote storage proposalvote = _proposalVotes[proposalId]; require(!proposalvote.hasVoted[account], "GovernorVotingSimple: vote already cast"); From 214259cd0aed6592025a79dec87de3bf8db868c1 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 15 Feb 2022 16:25:03 -0500 Subject: [PATCH 13/19] Fix doc comments related to getting votes --- contracts/governance/Governor.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index f01fff2743e..4f58a298c68 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -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._ @@ -204,7 +204,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { function _voteSucceeded(uint256 proposalId) internal view virtual returns (bool); /** - * @dev Check the voting weight of an account. + * @dev Get the voting weight of `account` at a specific `blockNumber`, for a vote as described by `params`. */ function _getVotes( address account, From 970556e9b727f39004d5b2ef9d8c13a7479e89cd Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 15 Feb 2022 17:00:51 -0500 Subject: [PATCH 14/19] Simplify Changelog description and include instructions for resolving breaking changes --- CHANGELOG.md | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fedc773238..365c4014639 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,17 +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)) - * `IGovernor`: Add two new methods to the interface, `castVoteWithReasonAndParams` and `castVoteWithReasonAndParamsBySig`, allowing voting extensions to include more data about how the vote is to be interpreted. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) - * `IGovernor`: Add a new event `VoteCastWithParams` that is emitted when a vote is cast that includes non-empty params data. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) - * `Governor`: Add internal `_defaultParams` method and default implementation. The return value is passed as params data when `castVote` methods without params are called. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) - * `Governor`: Add default `getVotes` implementation which calls virtual internal `_getVotes` method. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) - * `Governor`: Add `EXTENDED_BALLOT_TYPEHASH` used for signature validation in `castVoteWithReasonAndParamsBySig`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) + * `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 -* `Governor`: Adds internal virtual `_getVotes` method that must be implemented; this is a breaking change for existing concrete extensions to `Governor`. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) -* `Governor`: Adds `params` parameter to internal virtual `_countVote ` method that; this is a breaking change for existing concrete extensions to `Governor`. ([#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. ([#3043](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3043)) +* `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) From 6e2b447283ea014c85a4bd1b700588cd508cda46 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Tue, 15 Feb 2022 17:08:11 -0500 Subject: [PATCH 15/19] Fix Governor linting issue --- contracts/governance/Governor.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 4f58a298c68..17ec086df0d 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -475,6 +475,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { 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. Uses the _defaultParams(). From 455efe84dc5a8f2f1cd4481961722f880775f1d5 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Fri, 25 Feb 2022 22:43:39 -0500 Subject: [PATCH 16/19] Add public getVotesWithParams to Governor --- contracts/governance/Governor.sol | 14 +++++++++++++- contracts/governance/IGovernor.sol | 10 ++++++++++ .../introspection/SupportsInterface.behavior.js | 1 + 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 17ec086df0d..8907d9468d5 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -94,7 +94,8 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { interfaceId == (type(IGovernor).interfaceId ^ this.castVoteWithReasonAndParams.selector ^ - this.castVoteWithReasonAndParamsBySig.selector) || + this.castVoteWithReasonAndParamsBySig.selector ^ + this.getVotesWithParams.selector) || interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId); } @@ -392,6 +393,17 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { 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}. */ diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 5eb08d9848b..2992c0bf7a7 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -166,6 +166,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`. diff --git a/test/utils/introspection/SupportsInterface.behavior.js b/test/utils/introspection/SupportsInterface.behavior.js index 20281b2101a..a0aa3d7e50e 100644 --- a/test/utils/introspection/SupportsInterface.behavior.js +++ b/test/utils/introspection/SupportsInterface.behavior.js @@ -81,6 +81,7 @@ const INTERFACES = { 'votingPeriod()', 'quorum(uint256)', 'getVotes(address,uint256)', + 'getVotesWithParams(address,uint256,bytes)', 'hasVoted(uint256,address)', 'propose(address[],uint256[],bytes[],string)', 'execute(address[],uint256[],bytes[],bytes32)', From 8722c5cade8f3de36bebd14aa1ffdc50cf32572d Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Fri, 25 Feb 2022 22:51:29 -0500 Subject: [PATCH 17/19] Properly call super in GovernorWithParamsMock --- contracts/mocks/GovernorWithParamsMock.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/mocks/GovernorWithParamsMock.sol b/contracts/mocks/GovernorWithParamsMock.sol index 0b3dc133906..35eb7ade25a 100644 --- a/contracts/mocks/GovernorWithParamsMock.sol +++ b/contracts/mocks/GovernorWithParamsMock.sol @@ -33,7 +33,7 @@ contract GovernorWithParamsMock is GovernorVotes, GovernorCountingSimple { (reduction, ) = abi.decode(params, (uint256, string)); } // reverts on overflow - return GovernorVotes._getVotes(account, blockNumber, params) - reduction; + return super._getVotes(account, blockNumber, params) - reduction; } function _countVote( @@ -47,7 +47,7 @@ contract GovernorWithParamsMock is GovernorVotes, GovernorCountingSimple { (uint256 _uintParam, string memory _strParam) = abi.decode(params, (uint256, string)); emit CountParams(_uintParam, _strParam); } - return GovernorCountingSimple._countVote(proposalId, account, support, weight, params); + return super._countVote(proposalId, account, support, weight, params); } function cancel( From b4cd7c4caaf5f87a445125cb6be117857559d5ce Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Fri, 25 Feb 2022 23:14:23 -0500 Subject: [PATCH 18/19] Directly verify vote totals when testing parameterized voting --- test/governance/extensions/GovernorWithParams.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js index eca66a28ec1..5b4ac8901d9 100644 --- a/test/governance/extensions/GovernorWithParams.test.js +++ b/test/governance/extensions/GovernorWithParams.test.js @@ -7,6 +7,7 @@ const { EIP712Domain } = require('../../helpers/eip712'); const { fromRpcSig } = require('ethereumjs-util'); const { runGovernorWorkflow } = require('../GovernorWorkflow.behavior'); +const { expect } = require('chai'); const Token = artifacts.require('ERC20VotesCompMock'); const Governor = artifacts.require('GovernorWithParamsMock'); @@ -142,6 +143,8 @@ contract('GovernorWithParams', function (accounts) { expectEvent(tx, 'CountParams', { uintParam, strParam }); expectEvent(tx, 'VoteCastWithParams', { voter: voter2, weight: reducedWeight, params }); + const votes = await this.mock.proposalVotes(this.id); + expect(votes.forVotes).to.be.bignumber.equal(reducedWeight); }); runGovernorWorkflow(); }); @@ -227,6 +230,8 @@ contract('GovernorWithParams', function (accounts) { expectEvent(tx, 'CountParams', { uintParam, strParam }); expectEvent(tx, 'VoteCastWithParams', { voter: this.voter, weight: reducedWeight, params }); + const votes = await this.mock.proposalVotes(this.id); + expect(votes.forVotes).to.be.bignumber.equal(reducedWeight); }); runGovernorWorkflow(); }); From 78c99685ed9431b9e2a22a401b5f0a1ae683c53f Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Fri, 25 Feb 2022 23:33:29 -0500 Subject: [PATCH 19/19] Add a note about params to COUNTING_MODE documentation --- contracts/governance/IGovernor.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 2992c0bf7a7..eaf443a97d1 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -93,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.