Add a governance extension that implements super quorum

[0xmichalis]

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: e13dc10

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

{quorum} should not be described here IMO. Its like votingDelay or votingPeriod. The IGovernor documentation marks it as @notice module:user-config

Suggested change

	* - A voting module must implement {_getVotes} and {quorum}
	* - A voting module must implement {_getVotes}

[0xmichalis]

Updated. I've added the rest of the virtual function in the third bullet point but I was also considering to remove that bullet point altogether as it can easily go out of sync.

[Amxx]

IMO we should not add this here. Not all counting systems have will implement this function (there are "grade-based" module that don't use for/against/abstain).

IMO its ok for the super quorum module to not work with them, and to only support counting module that expose that function

[0xmichalis]

Created a new IGovernorCounting interface and moved this function in it.

[Amxx]

Now that I see that, I see what you mean by "we need to consider proposalEta".

---

The current implementation comes with nasty side effect to the current implementation the superQuorum that should be document!

Lets say:

• Quorum: 10
• SuperQuorum: 30
• current votes as: 20 for, 35 against

Two person whant to vote for, one with 10 votes, one with 20 votes:

• case where the 10 votes is included first: votes are now 30 vs 35 -> vote is closed by super quorum AND is a failure
• case where the 20 votes is included first: votes are now 40 vs 35 -> vote is closed by super quorum AND is a success.

That sensitivity to ordering is not great. But worst than that, it create a situation where voting "For" would cause a proposal to fail (and no further votes can compensate that). That makes it worst than the issue where an against vote would cause quorum to be reached and a vote to pass (in that case, other against vote could still be casted)

I'd argue if status is ACTIVE and super quorum is reached but vote is unsuccessful, then we should not return ProposalState.Defeated but rather ProposalState.Active (votes can continue until deadline)

Maybe there is a similar scenario where the voters vote against that we want to study.

---

Suggested change

	bool superQuorumReached = forVotes >= superQuorum(proposalSnapshot(proposalId));
	if (!superQuorumReached) {
	return currentState;
	}
	if (!_voteSucceeded(proposalId)) {
	return ProposalState.Defeated;
	} else if (proposalEta(proposalId) == 0) {
	return ProposalState.Succeeded;
	} else {
	return ProposalState.Queued;
	}
	if (forVotes < superQuorum(proposalSnapshot(proposalId) || ! _voteSucceeded(proposalId))) {
	return ProposalState.Active;
	} else if (proposalEta(proposalId) == 0) {
	return ProposalState.Succeeded;
	} else {
	return ProposalState.Queued;
	}

[0xmichalis]

Good catch, addressed in the latest commit. I've left a comment in natspec documenting that to avoid prematurely successful proposals, users should make sure to set a high super quorum, happy to discuss alternatives.

[Amxx]

you could do without this new function, an instead "just" do

function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual override {
    if (newQuorumNumerator >= superQuorumNumerator()) {
        revert GovernorInvalidQuorumTooLarge(newQuorumNumerator, superQuorumNumerator_);
    }
    super._updateQuorumNumerator(newQuorumNumerator);
}

in GovernorVotesSuperQuorumFraction.sol

[0xmichalis]

This doesn't work because on GovernorVotesSuperQuorumFraction deploy, the GovernorVotesQuorumFraction constructor is executed first and will fail because it will try to set a quorum higher than the super quorum that has not been initialized yet. I guess if you want to get away with the new virtual function, an alternative would be to update the GovernorVotesQuorumFraction constructor to not use the internal _updateQuorumNumerator function but instead inline its body in the constructor.

[Amxx]

Super interresting

I'm only now noticing that you add the _validateQuorumNumerator call to the public function, and not to the internal one. So technically, you could still get to an inconsistent state by caling the internal _updateQuorumNumerator directly.

If we really feel this having a superQuorum lower than the quorum, we would want to avoid that.

Maybe using a different approach:

• treat superQuorumNumerator = 0 as a special case (~infinity) ?
• in superQuorumNumerator, do a max with the quorumNumerator value ?

[0xmichalis]

treat superQuorumNumerator = 0 as a special case

Do you mean that we should only treat this as a special case for the purposes of bootstrapping and not from the POV of an end user? I am not sure what it would mean for them anyway. Iow, something like the following:

function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual override {
    uint256 superQuorumNumerator_ = superQuorumNumerator();
    if (superQuorumNumerator_ != 0 && newQuorumNumerator >= superQuorumNumerator_) {
        revert GovernorInvalidQuorumTooLarge(newQuorumNumerator, superQuorumNumerator_);
    }
    super._updateQuorumNumerator(newQuorumNumerator);
}

If we do that, then we don't need to use max in superQuorumNumerator() at all, do we? _updateSuperQuorumNumerator would then ensure that the super quorum is set higher than the quorum and can never be set again to zero.

[Amxx]

The two bullet points are two quick ideas I had (mulutally exclusive?)
I'm not sure what the right approach is. It might be somethiong completelly different.

Only things I'm sure of is that is we want to ensure some invariant (superQuorumNumerator() >= quorumNumerator()) we want to make sure that this invariant cannot be broken by calling the internal function.

Ideally I'd like the invariant to always hold, but I understand the bootstrapping is painfull because of constructor ordering.

If we ensure that by the end of the constructor, bootstrapping is done, then the above code might give the right properties.

[0xmichalis]

I see. max on its own does not solve the bootstrapping issue because superQuorumNumerator would return max(0,0) on deploy so newQuorumNumerator >= superQuorumNumerator -> newQuorumNumerator >= 0 would be true, thus revert.

I have simplified by dropping _validateQuorumNumerator in favor of the zero check in e13dc10.

In a previous commit, I had introduced an invariant test that ensures superQuorum is always greater than quorum, though the only functions I am currently fuzzing are the respective setters. Allowing all contract functions slows the test down significantly but we could enable all and reduce fuzz runs to something lower for this specific test as a compromise.

I've also considered whether in general the "super quorum is greater than quorum" invariant is something valuable and honestly I don't feel very strong about it. On the one hand, the lack of it doesn't change anything fundamental in votes and anyway a low super quorum is not recommended in any case. On the other hand, keeping it is good for the extra validation it provides as I doubt anyone would want to set super quorum lower than quorum. Overall I lean slightly towards keeping it but could be convinced to drop it.

[ernestognw]

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

[ernestognw]

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

[ernestognw]

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

[ernestognw]

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

[ernestognw]

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

[ernestognw]

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.

[ernestognw]

Shouldn't be?

Suggested change

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

[ernestognw]

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.