From 8ae6dd48014c540524437ea34bbea6194128649a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 20 Jan 2023 16:47:55 +0100 Subject: [PATCH 1/7] store proposer and expose a public cancel function --- contracts/governance/Governor.sol | 32 +++++++++++++++++-- contracts/governance/IGovernor.sol | 8 +++++ .../GovernorCompatibilityBravo.sol | 2 +- .../IGovernorCompatibilityBravo.sol | 5 --- .../GovernorCompatibilityBravoMock.sol | 4 +++ contracts/mocks/wizard/MyGovernor3.sol | 4 +++ 6 files changed, 46 insertions(+), 9 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 8235fb5ecb1..e55764b47a4 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -40,6 +40,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive Timers.BlockNumber voteEnd; bool executed; bool canceled; + address proposer; } string private _name; @@ -95,9 +96,13 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive return interfaceId == (type(IGovernor).interfaceId ^ + this.cancel.selector ^ this.castVoteWithReasonAndParams.selector ^ this.castVoteWithReasonAndParamsBySig.selector ^ this.getVotesWithParams.selector) || + interfaceId == + (type(IGovernor).interfaceId ^ + this.cancel.selector) || interfaceId == type(IGovernor).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); @@ -248,8 +253,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { + address proposer = _msgSender(); + require( - getVotes(_msgSender(), block.number - 1) >= proposalThreshold(), + getVotes(proposer, block.number - 1) >= proposalThreshold(), "Governor: proposer votes below proposal threshold" ); @@ -267,10 +274,11 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive proposal.voteStart.setDeadline(snapshot); proposal.voteEnd.setDeadline(deadline); + proposal.proposer = proposer; emit ProposalCreated( proposalId, - _msgSender(), + proposer, targets, values, new string[](targets.length), @@ -310,6 +318,15 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive return proposalId; } + /** + * @dev See {IGovernor-cancel}. + */ + function cancel(uint256 proposalId) public virtual override { + require(state(proposalId) == ProposalState.Pending, "Governor: too late to cancel"); + require(_msgSender() == _proposals[proposalId].proposer, "Governor: only proposer can cancel"); + _cancel(proposalId); + } + /** * @dev Internal execution mechanism. Can be overridden to implement different execution mechanism */ @@ -375,7 +392,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive bytes[] memory calldatas, bytes32 descriptionHash ) internal virtual returns (uint256) { - uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash); + return _cancel(hashProposal(targets, values, calldatas, descriptionHash)); + } + + /** + * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as + * canceled to allow distinguishing it from executed proposals. + * + * Emits a {IGovernor-ProposalCanceled} event. + */ + function _cancel(uint256 proposalId) internal virtual returns (uint256) { ProposalState status = state(proposalId); require( diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index eb3d1fc0500..18cf57fb163 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -216,6 +216,14 @@ abstract contract IGovernor is IERC165 { bytes32 descriptionHash ) public payable virtual returns (uint256 proposalId); + /** + * @dev Cancel a proposal. This is restricted to Pending proposal (before the vote starts) and is restricted to + * the proposal's proposer. + * + * Emits a {ProposalCanceled} event. + */ + function cancel(uint256 proposalId) public virtual; + /** * @dev Cast a vote * diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 8d74742c5bb..33fa07e7cc7 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -99,7 +99,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp ); } - function cancel(uint256 proposalId) public virtual override { + function cancel(uint256 proposalId) public virtual override(IGovernor, Governor) { ProposalDetails storage details = _proposalDetails[proposalId]; require( diff --git a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol index d1ec0337d00..159c5510081 100644 --- a/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/IGovernorCompatibilityBravo.sol @@ -90,11 +90,6 @@ abstract contract IGovernorCompatibilityBravo is IGovernor { */ function execute(uint256 proposalId) public payable virtual; - /** - * @dev Cancels a proposal only if sender is the proposer, or proposer delegates dropped below proposal threshold. - */ - function cancel(uint256 proposalId) public virtual; - /** * @dev Part of the Governor Bravo's interface: _"Gets actions of a proposal"_. */ diff --git a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol index 4356bce7f26..2727794f62c 100644 --- a/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol +++ b/contracts/mocks/governance/GovernorCompatibilityBravoMock.sol @@ -66,6 +66,10 @@ abstract contract GovernorCompatibilityBravoMock is return super.execute(targets, values, calldatas, salt); } + function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { + super.cancel(proposalId); + } + function _execute( uint256 proposalId, address[] memory targets, diff --git a/contracts/mocks/wizard/MyGovernor3.sol b/contracts/mocks/wizard/MyGovernor3.sol index 320342290a5..4192cae9442 100644 --- a/contracts/mocks/wizard/MyGovernor3.sol +++ b/contracts/mocks/wizard/MyGovernor3.sol @@ -54,6 +54,10 @@ contract MyGovernor is return super.propose(targets, values, calldatas, description); } + function cancel(uint256 proposalId) public override(Governor, GovernorCompatibilityBravo, IGovernor) { + super.cancel(proposalId); + } + function _execute( uint256 proposalId, address[] memory targets, From a6aba302378c0acfd6cb572109efdb2b8ea94559 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 20 Jan 2023 16:51:47 +0100 Subject: [PATCH 2/7] optimize compatibility layers's cancel --- .../compatibility/GovernorCompatibilityBravo.sol | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 33fa07e7cc7..912d02f8a51 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -107,12 +107,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp "GovernorBravo: proposer above threshold" ); - _cancel( - details.targets, - details.values, - _encodeCalldata(details.signatures, details.calldatas), - details.descriptionHash - ); + _cancel(proposalId); } /** From 000437d9e1c955fb73e46888eb5761bd8484039f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 20 Jan 2023 16:51:56 +0100 Subject: [PATCH 3/7] add changeset entry --- .changeset/flat-deers-end.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/flat-deers-end.md diff --git a/.changeset/flat-deers-end.md b/.changeset/flat-deers-end.md new file mode 100644 index 00000000000..61895f2cfe7 --- /dev/null +++ b/.changeset/flat-deers-end.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Governor`: add a public `cancel(uint256)` function. From d8dbb064e0c115ae274e286a24ec337776a222bb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 20 Jan 2023 17:08:25 +0100 Subject: [PATCH 4/7] test both internal and public cancel functions --- test/governance/Governor.test.js | 129 ++++++++++++++++++++++--------- test/helpers/governance.js | 4 +- 2 files changed, 94 insertions(+), 39 deletions(-) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index cc3cc17ef45..48c5b8276d4 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -382,58 +382,113 @@ contract('Governor', function (accounts) { }); describe('cancel', function () { - it('before proposal', async function () { - await expectRevert(this.helper.cancel(), 'Governor: unknown proposal id'); - }); + describe('internal', function () { + it('before proposal', async function () { + await expectRevert(this.helper.cancel(), 'Governor: unknown proposal id'); + }); - it('after proposal', async function () { - await this.helper.propose(); + it('after proposal', async function () { + await this.helper.propose(); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + await this.helper.cancel(); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); - await this.helper.waitForSnapshot(); - await expectRevert( - this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }), - 'Governor: vote not currently active', - ); - }); + await this.helper.waitForSnapshot(); + await expectRevert( + this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }), + 'Governor: vote not currently active', + ); + }); - it('after vote', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + it('after vote', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + await this.helper.cancel(); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); - await this.helper.waitForDeadline(); - await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); - }); + await this.helper.waitForDeadline(); + await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); + }); - it('after deadline', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.waitForDeadline(); + it('after deadline', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + + await this.helper.cancel(); + expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + + await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); + }); - await this.helper.cancel(); - expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); + it('after execution', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await this.helper.execute(); - await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); + await expectRevert(this.helper.cancel(), 'Governor: proposal not active'); + }); }); - it('after execution', async function () { - await this.helper.propose(); - await this.helper.waitForSnapshot(); - await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.waitForDeadline(); - await this.helper.execute(); + describe('public', function () { + it('before proposal', async function () { + await expectRevert(this.helper.cancel(true), 'Governor: unknown proposal id'); + }); + + it('after proposal', async function () { + await this.helper.propose(); + + await this.helper.cancel(true); + }); + + it('after proposal - restricted to proposer', async function () { + await this.helper.propose(); + + await expectRevert(this.helper.cancel(true, { from: owner }), "Governor: only proposer can cancel"); + }); + + it('after vote started', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(1); // snapshot + 1 block + + await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + }); + + it('after vote', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + + await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + }); + + it('after deadline', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); - await expectRevert(this.helper.cancel(), 'Governor: proposal not active'); + await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + }); + + it('after execution', async function () { + await this.helper.propose(); + await this.helper.waitForSnapshot(); + await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); + await this.helper.waitForDeadline(); + await this.helper.execute(); + + await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + }); }); }); + describe('proposal length', function () { it('empty', async function () { this.helper.setProposal([], ''); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index ff341aa12f3..55c88fbb62d 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -62,10 +62,10 @@ class GovernorHelper { ); } - cancel(opts = null) { + cancel(usePublic = false, opts = null) { const proposal = this.currentProposal; - return proposal.useCompatibilityInterface + return (proposal.useCompatibilityInterface || usePublic) ? this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)) : this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( ...concatOpts(proposal.shortProposal, opts), From 6f99f8c05457acba1c3109e42278dd5323cdf5b3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 20 Jan 2023 17:10:41 +0100 Subject: [PATCH 5/7] fix lint --- contracts/governance/Governor.sol | 4 +--- test/governance/Governor.test.js | 11 +++++------ test/helpers/governance.js | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index e55764b47a4..f82f9fea891 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -100,9 +100,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive this.castVoteWithReasonAndParams.selector ^ this.castVoteWithReasonAndParamsBySig.selector ^ this.getVotesWithParams.selector) || - interfaceId == - (type(IGovernor).interfaceId ^ - this.cancel.selector) || + interfaceId == (type(IGovernor).interfaceId ^ this.cancel.selector) || interfaceId == type(IGovernor).interfaceId || interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 48c5b8276d4..1d95a82968b 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -449,14 +449,14 @@ contract('Governor', function (accounts) { it('after proposal - restricted to proposer', async function () { await this.helper.propose(); - await expectRevert(this.helper.cancel(true, { from: owner }), "Governor: only proposer can cancel"); + await expectRevert(this.helper.cancel(true, { from: owner }), 'Governor: only proposer can cancel'); }); it('after vote started', async function () { await this.helper.propose(); await this.helper.waitForSnapshot(1); // snapshot + 1 block - await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); }); it('after vote', async function () { @@ -464,7 +464,7 @@ contract('Governor', function (accounts) { await this.helper.waitForSnapshot(); await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); }); it('after deadline', async function () { @@ -473,7 +473,7 @@ contract('Governor', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); }); it('after execution', async function () { @@ -483,12 +483,11 @@ contract('Governor', function (accounts) { await this.helper.waitForDeadline(); await this.helper.execute(); - await expectRevert(this.helper.cancel(true), "Governor: too late to cancel"); + await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); }); }); }); - describe('proposal length', function () { it('empty', async function () { this.helper.setProposal([], ''); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 55c88fbb62d..35962eb056f 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -65,7 +65,7 @@ class GovernorHelper { cancel(usePublic = false, opts = null) { const proposal = this.currentProposal; - return (proposal.useCompatibilityInterface || usePublic) + return proposal.useCompatibilityInterface || usePublic ? this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)) : this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( ...concatOpts(proposal.shortProposal, opts), From fd629580b01b2f53ca7f884e1fae3bbc151ec146 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 26 Jan 2023 11:21:17 +0100 Subject: [PATCH 6/7] refactor goernance helper --- test/governance/Governor.test.js | 24 +++++++++---------- .../GovernorCompatibilityBravo.test.js | 6 ++--- .../GovernorTimelockCompound.test.js | 4 ++-- .../GovernorTimelockControl.test.js | 4 ++-- test/helpers/governance.js | 15 +++++++----- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 1d95a82968b..4c083ed92c4 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -384,13 +384,13 @@ contract('Governor', function (accounts) { describe('cancel', function () { describe('internal', function () { it('before proposal', async function () { - await expectRevert(this.helper.cancel(), 'Governor: unknown proposal id'); + await expectRevert(this.helper.cancel('internal'), 'Governor: unknown proposal id'); }); it('after proposal', async function () { await this.helper.propose(); - await this.helper.cancel(); + await this.helper.cancel('internal'); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await this.helper.waitForSnapshot(); @@ -405,7 +405,7 @@ contract('Governor', function (accounts) { await this.helper.waitForSnapshot(); await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await this.helper.cancel(); + await this.helper.cancel('internal'); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await this.helper.waitForDeadline(); @@ -418,7 +418,7 @@ contract('Governor', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - await this.helper.cancel(); + await this.helper.cancel('internal'); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); @@ -431,32 +431,32 @@ contract('Governor', function (accounts) { await this.helper.waitForDeadline(); await this.helper.execute(); - await expectRevert(this.helper.cancel(), 'Governor: proposal not active'); + await expectRevert(this.helper.cancel('internal'), 'Governor: proposal not active'); }); }); describe('public', function () { it('before proposal', async function () { - await expectRevert(this.helper.cancel(true), 'Governor: unknown proposal id'); + await expectRevert(this.helper.cancel('external'), 'Governor: unknown proposal id'); }); it('after proposal', async function () { await this.helper.propose(); - await this.helper.cancel(true); + await this.helper.cancel('external'); }); it('after proposal - restricted to proposer', async function () { await this.helper.propose(); - await expectRevert(this.helper.cancel(true, { from: owner }), 'Governor: only proposer can cancel'); + await expectRevert(this.helper.cancel('external', { from: owner }), 'Governor: only proposer can cancel'); }); it('after vote started', async function () { await this.helper.propose(); await this.helper.waitForSnapshot(1); // snapshot + 1 block - await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); }); it('after vote', async function () { @@ -464,7 +464,7 @@ contract('Governor', function (accounts) { await this.helper.waitForSnapshot(); await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); - await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); }); it('after deadline', async function () { @@ -473,7 +473,7 @@ contract('Governor', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); }); it('after execution', async function () { @@ -483,7 +483,7 @@ contract('Governor', function (accounts) { await this.helper.waitForDeadline(); await this.helper.execute(); - await expectRevert(this.helper.cancel(true), 'Governor: too late to cancel'); + await expectRevert(this.helper.cancel('external'), 'Governor: too late to cancel'); }); }); }); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 231d57b2799..3666c45487b 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -226,18 +226,18 @@ contract('GovernorCompatibilityBravo', function (accounts) { describe('cancel', function () { it('proposer can cancel', async function () { await this.helper.propose({ from: proposer }); - await this.helper.cancel({ from: proposer }); + await this.helper.cancel('external', { from: proposer }); }); it('anyone can cancel if proposer drop below threshold', async function () { await this.helper.propose({ from: proposer }); await this.token.transfer(voter1, web3.utils.toWei('1'), { from: proposer }); - await this.helper.cancel(); + await this.helper.cancel('external'); }); it('cannot cancel is proposer is still above threshold', async function () { await this.helper.propose({ from: proposer }); - await expectRevert(this.helper.cancel(), 'GovernorBravo: proposer above threshold'); + await expectRevert(this.helper.cancel('external'), 'GovernorBravo: proposer above threshold'); }); }); }); diff --git a/test/governance/extensions/GovernorTimelockCompound.test.js b/test/governance/extensions/GovernorTimelockCompound.test.js index 334659e07cb..fb680338360 100644 --- a/test/governance/extensions/GovernorTimelockCompound.test.js +++ b/test/governance/extensions/GovernorTimelockCompound.test.js @@ -193,7 +193,7 @@ contract('GovernorTimelockCompound', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.queue(), 'Governor: proposal not successful'); @@ -206,7 +206,7 @@ contract('GovernorTimelockCompound', function (accounts) { await this.helper.waitForDeadline(); await this.helper.queue(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); diff --git a/test/governance/extensions/GovernorTimelockControl.test.js b/test/governance/extensions/GovernorTimelockControl.test.js index 2e8c7588815..4f9b9df73cc 100644 --- a/test/governance/extensions/GovernorTimelockControl.test.js +++ b/test/governance/extensions/GovernorTimelockControl.test.js @@ -185,7 +185,7 @@ contract('GovernorTimelockControl', function (accounts) { await this.helper.vote({ support: Enums.VoteType.For }, { from: voter1 }); await this.helper.waitForDeadline(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.queue(), 'Governor: proposal not successful'); @@ -198,7 +198,7 @@ contract('GovernorTimelockControl', function (accounts) { await this.helper.waitForDeadline(); await this.helper.queue(); - expectEvent(await this.helper.cancel(), 'ProposalCanceled', { proposalId: this.proposal.id }); + expectEvent(await this.helper.cancel('internal'), 'ProposalCanceled', { proposalId: this.proposal.id }); expect(await this.mock.state(this.proposal.id)).to.be.bignumber.equal(Enums.ProposalState.Canceled); await expectRevert(this.helper.execute(), 'Governor: proposal not successful'); diff --git a/test/helpers/governance.js b/test/helpers/governance.js index 35962eb056f..e266fe530b2 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -62,14 +62,17 @@ class GovernorHelper { ); } - cancel(usePublic = false, opts = null) { + cancel(visibility = 'external', opts = null) { const proposal = this.currentProposal; - return proposal.useCompatibilityInterface || usePublic - ? this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)) - : this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( - ...concatOpts(proposal.shortProposal, opts), - ); + switch (visibility) { + case 'external': + return this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)); + case 'internal': + return this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)'](...concatOpts(proposal.shortProposal, opts)); + default: + throw new Error(`unsuported visibility "${visibility}"`); + } } vote(vote = {}, opts = null) { From 2cfce4664b7172946e69542b12c342d468c00d3f Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 26 Jan 2023 11:22:39 +0100 Subject: [PATCH 7/7] fix lint --- test/helpers/governance.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/helpers/governance.js b/test/helpers/governance.js index e266fe530b2..f933f389d0a 100644 --- a/test/helpers/governance.js +++ b/test/helpers/governance.js @@ -69,7 +69,9 @@ class GovernorHelper { case 'external': return this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts)); case 'internal': - return this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)'](...concatOpts(proposal.shortProposal, opts)); + return this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)']( + ...concatOpts(proposal.shortProposal, opts), + ); default: throw new Error(`unsuported visibility "${visibility}"`); }