Add external cancel to OZ Governor by default

[julianmrodri]

🧐 Motivation
It would be great to allow the OZ Governor to have the cancel proposal functionality offered by default. It should be something similar like the cancel offered in Governor Bravo.

📝 Details
Even though currently an internal _cancel is offered in the OZ Governor, this cannot be used as one would probably need to store the proposer on chain, which is not part of the Governor implementation.

  function _cancel(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) internal override(Governor, GovernorTimelockControl) returns (uint256) {
        return super._cancel(targets, values, calldatas, descriptionHash);
    }

It would be great if the OZ governor provided already with the proposer on-chain and the ability to have an external cancel() that we can call:

  function cancel(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) external;
   

Default implementation could be based on the Governor Bravo cancel, by default could be something on these lines:

   function cancel( address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash) public virtual {
        
        // 1. - Get proposalID
        // 2. - Get proposer based on proposalID 
       
        // 3 - add checks for proposer
        require(
            _msgSender() == proposer || getVotes(proposer, block.number - 1) < proposalThreshold(),
            "Governor: proposer above threshold"
        );
       // 4 -  Cancel
        _cancel(
            targets,
            values,
            calldatas,
            descriptionHash
        );
    }

Also this function signature would have to be added to Tally for compatibility. Currently Tally does not allow to add external cancel to the OZ Governor as this is not part of the common interface. (As shown here: https://docs.tally.xyz/user-guides-1/supported-dao-frameworks/openzeppelin-style)

[frangio]

Thanks @julianmrodri.

I'm not sure if we want to add this by default or in an opt-in module. Any thoughts?

[jgeary]

definitely agree with this. anyone who deploys a GovernorTimelockControl and TimelockController using OZ's own contract wizard and wants to be able to kill select potential dao actions needs to grant some address the TimelockController.CANCELLER_ROLE and then that role can only cancel queued actions that have already succeeded as proposals. this is riskier (dangerous proposal has to come one step closer to being executed before it can be canceled), less gas efficient (users will spend gas on votes to pass proposal) and more contentious for communities (cancelations will be bound to upset people that successfully voted for the proposal). this is a strange default UX for something that is necessary for preventing a malicious or mistaken proposal that could cause loss of funds.

Governor or GovernorTimelockControl (or another module like GovernorCanceller) should have a function cancel(...) onlyRole(CANCELLER_ROLE) external plus AccessControl and a new CANCELLER_ROLE.

on that note, OZ should standardize how they spell words like canceled/cancelled and canceler/canceller. its inconsistent throughout the codebase and docs.

[Amxx]

Governor or GovernorTimelockControl (or another module like GovernorCanceller) should have a function cancel(...) onlyRole(CANCELLER_ROLE) external plus AccessControl and a new CANCELLER_ROLE.

This is both very opinionated, and very easy for a dev to manually add. Therefore, I think it should not be present by default.

We did not include Compound Alpha/Bravo cancel mechanism by default, because we think not everybody will want to give such power to the proposer. IMO, once created a proposal belongs to the community of voters. It feels wrong that the proposer can cancel it on his own, even though voters have started voting. We include that in the GovernorBravo compatibility module, and anyone can add this feature to their governor, but we did not want to have it by default.

Having a specific "whitelist" of address that can cancel goes even further. The accounts that have this power can sensor any proposal, basically resulting in nothing that they don't like passing through. Think of it like being able to veto a propose at the UN security council. We believe DAO should be self governed. Anyone is off course free to add this to their governance contract, but it should not be here by default.

[frangio]

In Bravo's cancel function there are two alternative conditions:

• msg.sender == proposer
• getVotes(proposer, block.number - 1) < proposalThreshold()

I would agree that giving the proposer the ability to cancel somewhat goes against the nature of decentralized governance, but the second condition doesn't have the same problem. The second condition is simply extending the concept of "proposal threshold" to say that the threshold must hold throughout the entire duration of a proposal, not just at the time of proposing. Arguably, by not implementing this condition we haven't implemented the same concept of "proposal threshold"!

[Amxx]

Even the second condition alone still gives a tremendous power to the proposer.

A proposer could just:

• de-delegate its votes
• cancel
• re-delegate.

[frangio]

We've arrived at a shared belief that giving the proposer the ability to cancel an active proposal gives it a power that we don't think it should have.

However, we think that it could make sense for the proposer to have the ability to cancel a proposal during the delay before it becomes active. This requires storing the proposer address in storage, which is not something the contract currently does. It is possible to add this to the ProposalCore struct without increasing the number of storage slots that it uses, thus without substantially increasing the cost of propose (though it may require some tricks once we move to represent timepoints by smaller integers).

We may want to reserve the space for some other value, but I don't see at the moment what that could be.

If we decided to use the space for the proposer, we could add cancel in Governor by default, with the logic that a proposal can be cancelled before it becomes active.

[Amxx]

It is possible to add this to the ProposalCore struct without increasing the number of storage slots

That is possible with the current, poorly packed, version of the structure.

In 5.0 we want to use uint48 (or at most uint64) for both timers. This will make the struct fit into 1 slot, compared to 3 today. On the other hand, this one slot will not have enough spare room for an address.

[frangio]

The obvious way to store voteStart and voteEnd is using uint48 for each. However, instead of voteEnd we can store voteEnd - voteStart (i.e. votingPeriod) which should be comfortably representable with a smaller integer. This is what I meant by "it may require some tricks".

[Amxx]

000-159 : address
160-207: uint48 (vote starts)
208-239: uint32 (duration)
240-247: bool (executed)
248-255: bool (canceled)

The 2 bool might even be merged into a single bytes1 to have a uint40 for the duration.

type(uint32).max = +136 years

uint40 would provide support even for blocknumber that happens 10k times per seconds.