Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a governance extension that implements super quorum #5492

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/fuzzy-crews-poke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'openzeppelin-solidity': minor
---

- `GovernorSuperQuorum`: Add a governance extension to support a super quorum. Proposals that meet the super quorum can be executed earlier than the proposal deadline.
- `GovernorVotesSuperQuorumFraction`: Add a governance extension to enable super quorum with fractional voting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split this one into two changesets and remove the - at the beginning, each changeset gets formatted once we release a new version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing in bb9a989

4 changes: 2 additions & 2 deletions contracts/governance/Governor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import {IGovernor, IERC6372} from "./IGovernor.sol";
*
* This contract is abstract and requires several functions to be implemented in various modules:
*
* - A counting module must implement {quorum}, {_quorumReached}, {_voteSucceeded} and {_countVote}
* - A counting module must implement {_quorumReached}, {_voteSucceeded} and {_countVote}
* - A voting module must implement {_getVotes}
* - Additionally, {votingPeriod} must also be implemented
* - Additionally, {votingPeriod}, {votingDelay}, and {quorum} must also be implemented
*/
abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC721Receiver, IERC1155Receiver {
using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;
Expand Down
4 changes: 4 additions & 0 deletions contracts/governance/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ Votes modules determine the source of voting power, and sometimes quorum number.

* {GovernorVotesQuorumFraction}: Combines with `GovernorVotes` to set the quorum as a fraction of the total token supply.

* {GovernorVotesSuperQuorumFraction}: Combines with `GovernorVotesQuorumFraction` to set the super quorum as a fraction of the total token supply.

Counting modules determine valid voting options.

* {GovernorCountingSimple}: Simple voting mechanism with 3 voting options: Against, For and Abstain.
Expand All @@ -50,6 +52,8 @@ Other extensions can customize the behavior or interface in multiple ways.

* {GovernorProposalGuardian}: Adds a proposal guardian that can cancel proposals at any stage in their lifecycle--this permission is passed on to the proposers if the guardian is not set.

* {GovernorSuperQuorum}: Adds a super quorum that if reached, the proposal can proceed to the next state without waiting for the proposal deadline.

In addition to modules and extensions, the core contract requires a few virtual functions to be implemented to your particular specifications:

* <<Governor-votingDelay-,`votingDelay()`>>: Delay (in ERC-6372 clock) since the proposal is submitted until voting power is fixed and voting starts. This can be used to enforce a delay after a proposal is published for users to buy tokens, or delegate their votes.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pragma solidity ^0.8.20;

import {Governor} from "../Governor.sol";
import {GovernorCountingSimple} from "./GovernorCountingSimple.sol";
import {IGovernorCounting} from "./IGovernorCounting.sol";
import {Math} from "../../utils/math/Math.sol";

/**
Expand All @@ -27,11 +28,11 @@ import {Math} from "../../utils/math/Math.sol";
* * Voting from an L2 with tokens held by a bridge
* * Voting privately from a shielded pool using zero knowledge proofs.
*
* Based on ScopeLift's GovernorCountingFractional[https://github.com/ScopeLift/flexible-voting/blob/e5de2efd1368387b840931f19f3c184c85842761/src/GovernorCountingFractional.sol]
* Based on ScopeLift's https://github.com/ScopeLift/flexible-voting/blob/e5de2efd1368387b840931f19f3c184c85842761/src/GovernorCountingFractional.sol[`GovernorCountingFractional`]
*
* _Available since v5.1._
*/
abstract contract GovernorCountingFractional is Governor {
abstract contract GovernorCountingFractional is Governor, IGovernorCounting {
using Math for *;

uint8 internal constant VOTE_TYPE_FRACTIONAL = 255;
Expand Down Expand Up @@ -81,7 +82,7 @@ abstract contract GovernorCountingFractional is Governor {
*/
function proposalVotes(
uint256 proposalId
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
) public view virtual override returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
ProposalVote storage proposalVote = _proposalVotes[proposalId];
return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import {SignatureChecker} from "../../utils/cryptography/SignatureChecker.sol";
import {SafeCast} from "../../utils/math/SafeCast.sol";
import {VotesExtended} from "../utils/VotesExtended.sol";
import {GovernorVotes} from "./GovernorVotes.sol";
import {IGovernorCounting} from "./IGovernorCounting.sol";

/**
* @dev Extension of {Governor} which enables delegators to override the vote of their delegates. This module requires a
* token that inherits {VotesExtended}.
*/
abstract contract GovernorCountingOverridable is GovernorVotes {
abstract contract GovernorCountingOverridable is GovernorVotes, IGovernorCounting {
bytes32 public constant OVERRIDE_BALLOT_TYPEHASH =
keccak256("OverrideBallot(uint256 proposalId,uint8 support,address voter,uint256 nonce,string reason)");

Expand Down Expand Up @@ -78,7 +79,7 @@ abstract contract GovernorCountingOverridable is GovernorVotes {
*/
function proposalVotes(
uint256 proposalId
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
) public view virtual override returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one will keep compiling since overriding interface functions is not required since Solidity 0.8.14.

Suggested change
) public view virtual override returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing in bb9a989

uint256[3] storage votes = _proposalVotes[proposalId].votes;
return (votes[uint8(VoteType.Against)], votes[uint8(VoteType.For)], votes[uint8(VoteType.Abstain)]);
}
Expand Down
5 changes: 3 additions & 2 deletions contracts/governance/extensions/GovernorCountingSimple.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
pragma solidity ^0.8.20;

import {Governor} from "../Governor.sol";
import {IGovernorCounting} from "./IGovernorCounting.sol";

/**
* @dev Extension of {Governor} for simple, 3 options, vote counting.
*/
abstract contract GovernorCountingSimple is Governor {
abstract contract GovernorCountingSimple is Governor, IGovernorCounting {
/**
* @dev Supported vote types. Matches Governor Bravo ordering.
*/
Expand Down Expand Up @@ -47,7 +48,7 @@ abstract contract GovernorCountingSimple is Governor {
*/
function proposalVotes(
uint256 proposalId
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
) public view virtual override returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Suggested change
) public view virtual override returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing in bb9a989

ProposalVote storage proposalVote = _proposalVotes[proposalId];
return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes);
}
Expand Down
49 changes: 49 additions & 0 deletions contracts/governance/extensions/GovernorSuperQuorum.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {Governor} from "../Governor.sol";
import {IGovernorCounting} from "./IGovernorCounting.sol";
import {SafeCast} from "../../utils/math/SafeCast.sol";
import {Checkpoints} from "../../utils/structs/Checkpoints.sol";

/**
* @dev Extension of {Governor} with a super quorum. Proposals that meet the super quorum can be executed
* earlier than the proposal deadline. Counting modules that want to use this extension must implement
* {IGovernorCounting}.
*
* NOTE: It's up to developers to implement `superQuorum` and validate it against `quorum`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this note to the superQuorum function and make it a bit clear about what to expect. Right now the note does not enable the reader to take a decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing in bb9a989

*/
abstract contract GovernorSuperQuorum is Governor {
/**
* @dev Minimum number of cast votes required for a proposal to reach super quorum. Only FOR votes are counted towards
* the super quorum. Once the super quorum is reached, an active proposal can proceed to the next state without waiting
* for the proposal deadline.
*
* NOTE: The `timepoint` parameter corresponds to the snapshot used for counting vote. This allows to scale the
* quorum depending on values such as the totalSupply of a token at this timepoint (see {ERC20Votes}).
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following my previous comment, I think this is clearer in regards to what the developer must care and what the consequences are

Suggested change
*/
*
* NOTE: Make sure the value specified for the super quorum is greater than {quorum}, otherwise, it
* may be possible to pass a proposal with less votes than the default quorum.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing in bb9a989

function superQuorum(uint256 timepoint) public view virtual returns (uint256);

/**
* @dev Overridden version of the {Governor-state} function that checks if the proposal has reached the super quorum.
*
* NOTE: If the proposal reaches super quorum but {_voteSucceeded} returns false, eg, assuming the super quorum has been set
* low enough that both FOR and AGAINST votes have exceeded it and AGAINST votes exceed FOR votes, the proposal continues to
* be active until {_voteSucceeded} returns true or the proposal deadline is reached. This means that with a low super quorum
* it is also possible that a vote can succeed prematurely before enough AGAINST voters have a chance to vote. Hence, it is
* recommended to set a high enough super quorum to avoid these types of scenarios.
*/
function state(uint256 proposalId) public view virtual override returns (ProposalState) {
ProposalState currentState = super.state(proposalId);
if (currentState != ProposalState.Active) return currentState;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this line is forcing to replicate the logic of calculating Active, Succeeded and Queued down in the function. I would flip the logic like this:

Suggested change
if (currentState != ProposalState.Active) return currentState;
if (currentState == ProposalState.Active) {
// ... rest of the logic
// and some form of conditional return
}
return currentState

This way, the logic from Governor is completely reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to calculate all states in the overridden version. The way this is currently coded:

  • Governor calculates all states via super.state (so there is some form of reuse but not entirely. I also mentioned this code duplication that you have spotted in Slack)
  • if Governor doesn't return Active, GovernorSuperQuorum does nothing
  • if Governor returns Active, then GovernorSuperQuorum needs to be able to advance to the next state (Queued, Succeeded) or maintain Active

Not sure I understand your suggestion but from the looks of it I would still have to do this:

if (currentState == ProposalState.Active) {
        (, uint256 forVotes, ) = IGovernorCounting(address(this)).proposalVotes(proposalId);
        if (forVotes < superQuorum(proposalSnapshot(proposalId)) || !_voteSucceeded(proposalId)) {
            return currentState;
        } else if (proposalEta(proposalId) == 0) {
            return ProposalState.Succeeded;
        } else {
            return ProposalState.Queued;
        }
}


(, uint256 forVotes, ) = IGovernorCounting(address(this)).proposalVotes(proposalId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this function is external in the interface, but the intuition of calling this with proposalVotes ignores that proposalVotes is public in every Governor counting module.

Given the compiler doesn't give an error when you mark as public a function that is external in the interface, I think that should be the default since it's the least disruptive action. Otherwise I'm worried that it replaces the execution environment (i.e. msg.sender and others), which could be relevant.

I'd use the IGovernorCounting interface and:

Suggested change
(, uint256 forVotes, ) = IGovernorCounting(address(this)).proposalVotes(proposalId);
(, uint256 forVotes, ) = proposalVotes(proposalId);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what do you mean that you would use the IGovernorCounting interface? GovernorSuperQuorum has no knowledge of the counting modules/proposalVotes as that's not inherited from Governor so I cannot drop the casting from here.

if (forVotes < superQuorum(proposalSnapshot(proposalId)) || !_voteSucceeded(proposalId)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be?

Suggested change
if (forVotes < superQuorum(proposalSnapshot(proposalId)) || !_voteSucceeded(proposalId)) {
if (forVotes < superQuorum(proposalSnapshot(proposalId)) || _voteSucceeded(proposalId)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clause reads as "if FOR votes haven't reached super quorum OR the vote is not yet successful, then the proposal is active". In other words, both conditions need to be false in order for the proposal not to be active anymore: "FOR votes have reached super quorum AND the vote is successful". So no, the conditional should stay as is.

return ProposalState.Active;
} else if (proposalEta(proposalId) == 0) {
return ProposalState.Succeeded;
} else {
return ProposalState.Queued;
}
}
}
16 changes: 13 additions & 3 deletions contracts/governance/extensions/GovernorVotesQuorumFraction.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,28 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
* @dev Returns the quorum numerator at a specific timepoint. See {quorumDenominator}.
*/
function quorumNumerator(uint256 timepoint) public view virtual returns (uint256) {
uint256 length = _quorumNumeratorHistory._checkpoints.length;
return _numerator(_quorumNumeratorHistory, timepoint);
}

/**
* @dev Returns the numerator at a specific timepoint.
*/
function _numerator(
Checkpoints.Trace208 storage numeratorHistory,
uint256 timepoint
) internal view returns (uint256) {
uint256 length = numeratorHistory._checkpoints.length;

// Optimistic search, check the latest checkpoint
Checkpoints.Checkpoint208 storage latest = _quorumNumeratorHistory._checkpoints[length - 1];
Checkpoints.Checkpoint208 storage latest = numeratorHistory._checkpoints[length - 1];
uint48 latestKey = latest._key;
uint208 latestValue = latest._value;
if (latestKey <= timepoint) {
return latestValue;
}

// Otherwise, do the binary search
return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint48(timepoint));
return numeratorHistory.upperLookupRecent(SafeCast.toUint48(timepoint));
}

/**
Expand Down
130 changes: 130 additions & 0 deletions contracts/governance/extensions/GovernorVotesSuperQuorumFraction.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {Governor} from "../Governor.sol";
import {GovernorSuperQuorum} from "./GovernorSuperQuorum.sol";
import {GovernorVotesQuorumFraction} from "./GovernorVotesQuorumFraction.sol";
import {SafeCast} from "../../utils/math/SafeCast.sol";
import {Checkpoints} from "../../utils/structs/Checkpoints.sol";

/**
* @dev Extension of {GovernorVotesQuorumFraction} with a super quorum expressed as a
* fraction of the total supply. Proposals that meet the super quorum can be executed
* earlier than the proposal deadline.
*/
abstract contract GovernorVotesSuperQuorumFraction is GovernorVotesQuorumFraction, GovernorSuperQuorum {
using Checkpoints for Checkpoints.Trace208;

Checkpoints.Trace208 private _superQuorumNumeratorHistory;

event SuperQuorumNumeratorUpdated(uint256 oldSuperQuorumNumerator, uint256 newSuperQuorumNumerator);

/**
* @dev The super quorum set is not valid as it exceeds the quorum denominator.
*/
error GovernorInvalidSuperQuorumFraction(uint256 superQuorumNumerator, uint256 denominator);

/**
* @dev The super quorum set is not valid as it is smaller or equal to the quorum.
*/
error GovernorInvalidSuperQuorumTooSmall(uint256 superQuorumNumerator, uint256 quorumNumerator);

/**
* @dev The quorum set is not valid as it exceeds the super quorum.
*/
error GovernorInvalidQuorumTooLarge(uint256 quorumNumerator, uint256 superQuorumNumerator);

/**
* @dev Initialize super quorum as a fraction of the token's total supply.
*
* The super quorum is specified as a fraction of the token's total supply and has to
* be greater than the quorum.
*/
constructor(uint256 superQuorumNumeratorValue) {
_updateSuperQuorumNumerator(superQuorumNumeratorValue);
}

/**
* @dev Returns the current super quorum numerator.
*/
function superQuorumNumerator() public view virtual returns (uint256) {
return _superQuorumNumeratorHistory.latest();
}

/**
* @dev Returns the super quorum numerator at a specific timepoint.
*/
function superQuorumNumerator(uint256 timepoint) public view virtual returns (uint256) {
return _numerator(_superQuorumNumeratorHistory, timepoint);
}

/**
* @dev Returns the super quorum for a timepoint, in terms of number of votes: `supply * numerator / denominator`.
*/
function superQuorum(uint256 timepoint) public view virtual override returns (uint256) {
return (token().getPastTotalSupply(timepoint) * superQuorumNumerator(timepoint)) / quorumDenominator();
}

/**
* @dev Changes the super quorum numerator.
*
* Emits a {SuperQuorumNumeratorUpdated} event.
*
* Requirements:
*
* - Must be called through a governance proposal.
* - New super quorum numerator must be smaller or equal to the denominator.
* - New super quorum numerator must be bigger than the quorum numerator.
*/
function updateSuperQuorumNumerator(uint256 newSuperQuorumNumerator) external virtual onlyGovernance {
_updateSuperQuorumNumerator(newSuperQuorumNumerator);
}

/**
* @dev Changes the super quorum numerator.
*
* Emits a {SuperQuorumNumeratorUpdated} event.
*
* Requirements:
*
* - New super quorum numerator must be smaller or equal to the denominator.
* - New super quorum numerator must be bigger than the quorum numerator.
*/
function _updateSuperQuorumNumerator(uint256 newSuperQuorumNumerator) internal virtual {
uint256 denominator = quorumDenominator();
if (newSuperQuorumNumerator > denominator) {
revert GovernorInvalidSuperQuorumFraction(newSuperQuorumNumerator, denominator);
}
uint256 quorumNumerator = quorumNumerator();
if (newSuperQuorumNumerator <= quorumNumerator) {
revert GovernorInvalidSuperQuorumTooSmall(newSuperQuorumNumerator, quorumNumerator);
}

uint256 oldSuperQuorumNumerator = _superQuorumNumeratorHistory.latest();
_superQuorumNumeratorHistory.push(clock(), SafeCast.toUint208(newSuperQuorumNumerator));

emit SuperQuorumNumeratorUpdated(oldSuperQuorumNumerator, newSuperQuorumNumerator);
}

/**
* @dev Overrides {GovernorVotesQuorumFraction._updateQuorumNumerator} to ensure the super
* quorum numerator is bigger than the quorum numerator.
*/
function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual override {
uint256 superQuorumNumerator_ = superQuorumNumerator();
// Ignoring a super quorum of 0 as it is the initial value when the contract is deployed.
if (superQuorumNumerator_ != 0 && newQuorumNumerator >= superQuorumNumerator_) {
revert GovernorInvalidQuorumTooLarge(newQuorumNumerator, superQuorumNumerator_);
}
super._updateQuorumNumerator(newQuorumNumerator);
}

/**
* @dev Defined to resolve conflicting state() function inheritance
*/
function state(
uint256 proposalId
) public view virtual override(Governor, GovernorSuperQuorum) returns (ProposalState) {
return super.state(proposalId);
}
}
21 changes: 21 additions & 0 deletions contracts/governance/extensions/IGovernorCounting.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

/**
* @dev Interface to count votes for the following counting modules:
* - GovernorCountingFractional
* - GovernorCountingSimple
* - GovernorCountingOverridable
*
* This interface is used by GovernorSuperQuorum to get counted votes from the above counting modules.
* Other counting modules that need to work with GovernorSuperQuorum MUST implement this interface.
*/
interface IGovernorCounting {
/**
* @dev Accessor to the internal vote counts.
*/
function proposalVotes(
uint256 proposalId
) external view returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes);
}
Loading