Add a public Governor.cancel function (OpenZeppelin#3983)

Amxx · Jan 26, 2023 · 52c7a3a · 52c7a3a
1 parent e919d96
commit 52c7a3a
Show file tree

Hide file tree

Showing 12 changed files with 157 additions and 63 deletions.
diff --git a/.changeset/flat-deers-end.md b/.changeset/flat-deers-end.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`Governor`: add a public `cancel(uint256)` function.
diff --git 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,11 @@ 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 +251,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 +272,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 +316,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 +390,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
@@ -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
@@ -99,20 +99,15 @@ 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(
             _msgSender() == details.proposer || getVotes(details.proposer, block.number - 1) < proposalThreshold(),
             "GovernorBravo: proposer above threshold"
         );
 
-        _cancel(
-            details.targets,
-            details.values,
-            _encodeCalldata(details.signatures, details.calldatas),
-            details.descriptionHash
-        );
+        _cancel(proposalId);
     }
 
     /**

diff --git 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
@@ -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
@@ -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,

diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js
@@ -384,55 +384,109 @@ 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('internal'), '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('internal');
+        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('internal');
+        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('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');
+      });
 
-      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('internal'), '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('external'), 'Governor: unknown proposal id');
+      });
+
+      it('after proposal', async function () {
+        await this.helper.propose();
+
+        await this.helper.cancel('external');
+      });
+
+      it('after proposal - restricted to proposer', async function () {
+        await this.helper.propose();
+
+        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('external'), 'Governor: too late to cancel');
+      });
 
-      await expectRevert(this.helper.cancel(), 'Governor: proposal not active');
+      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('external'), '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('external'), '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('external'), 'Governor: too late to cancel');
+      });
     });
   });
 

diff --git 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
@@ -194,7 +194,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');
@@ -207,7 +207,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
@@ -186,7 +186,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');
@@ -199,7 +199,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
@@ -62,14 +62,19 @@ class GovernorHelper {
         );
   }
 
-  cancel(opts = null) {
+  cancel(visibility = 'external', opts = null) {
     const proposal = this.currentProposal;
 
-    return proposal.useCompatibilityInterface
-      ? this.governor.methods['cancel(uint256)'](...concatOpts([proposal.id], opts))
-      : this.governor.methods['$_cancel(address[],uint256[],bytes[],bytes32)'](
+    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) {