Add a public Governor.cancel function (#3983)

.changeset/flat-deers-end.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`Governor`: add a public `cancel(uint256)` function.

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(

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
      *

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(
@@ -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);
     }
 
     /**

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"_.
      */

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,

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,

test/governance/Governor.test.js

@@ -382,55 +382,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');
+      });
     });
   });
 

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');
     });
   });
 });

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');

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');

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) {