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 GovernorCountingFractional #5045

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/eight-eyes-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`GovernorCountingFractional`: Add a governor counting module that allows distributing voting power amongst 3 options (For, Against, Abstain).
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Breaking changes

- `ERC1967Utils`: Removed duplicate declaration of the `Upgraded`, `AdminChanged` and `BeaconUpgraded` events. These events are still available through the `IERC1967` interface located under the `contracts/interfaces/` directory. Minimum pragma version is now 0.8.21.
- `Governor`, `GovernorCountingSimple`: The `_countVotes` virtual function now returns an `uint256` with the total votes casted. This change allows for more flexibility for partial and fractional voting. Upgrading users may get a compilation error that can be fixed by adding a return statement to the `_countVotes` function.

### Custom error changes

Expand Down
14 changes: 7 additions & 7 deletions contracts/governance/Governor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
uint256 proposalId,
address account,
uint8 support,
uint256 weight,
uint256 totalWeight,
bytes memory params
) internal virtual;
) internal virtual returns (uint256);

/**
* @dev Default additional encoded parameters used by castVote methods that don't include them
Expand Down Expand Up @@ -639,16 +639,16 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
) internal virtual returns (uint256) {
_validateStateBitmap(proposalId, _encodeStateBitmap(ProposalState.Active));

uint256 weight = _getVotes(account, proposalSnapshot(proposalId), params);
_countVote(proposalId, account, support, weight, params);
uint256 totalWeight = _getVotes(account, proposalSnapshot(proposalId), params);
uint256 votedWeight = _countVote(proposalId, account, support, totalWeight, params);

if (params.length == 0) {
emit VoteCast(account, proposalId, support, weight, reason);
emit VoteCast(account, proposalId, support, votedWeight, reason);
} else {
emit VoteCastWithParams(account, proposalId, support, weight, reason, params);
emit VoteCastWithParams(account, proposalId, support, votedWeight, reason, params);
}

return weight;
return votedWeight;
}

/**
Expand Down
5 changes: 5 additions & 0 deletions contracts/governance/IGovernor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ interface IGovernor is IERC165, IERC6372 {
*/
error GovernorInvalidVoteType();

/**
* @dev The provided params buffer is not supported by the counting module.
*/
error GovernorInvalidVoteParams();

/**
* @dev Queue operation is not implemented for this governor. Execute should be called directly.
*/
Expand Down
4 changes: 4 additions & 0 deletions contracts/governance/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ Counting modules determine valid voting options.

* {GovernorCountingSimple}: Simple voting mechanism with 3 voting options: Against, For and Abstain.

* {GovernorCountingFractional}: A more modular voting system that allows a user to vote with only part of its voting power, and to split that weight arbitrarily between the 3 different options (Against, For and Abstain).

Timelock extensions add a delay for governance decisions to be executed. The workflow is extended to require a `queue` step before execution. With these modules, proposals are executed by the external timelock contract, thus it is the timelock that has to hold the assets that are being governed.

* {GovernorTimelockAccess}: Connects with an instance of an {AccessManager}. This allows restrictions (and delays) enforced by the manager to be considered by the Governor and integrated into the AccessManager's "schedule + execute" workflow.
Expand Down Expand Up @@ -62,6 +64,8 @@ NOTE: Functions of the `Governor` contract do not include access control. If you

{{GovernorCountingSimple}}

{{GovernorCountingFractional}}

{{GovernorVotes}}

{{GovernorVotesQuorumFraction}}
Expand Down
193 changes: 193 additions & 0 deletions contracts/governance/extensions/GovernorCountingFractional.sol
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

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

/**
* @dev Extension of {Governor} for fractional voting.
*
* Similar to {GovernorCountingSimple}, this contract is a votes counting module for {Governor} that supports 3 options:
* Against, For, Abstain. Additionally, it includes a fourth option: Fractional, which allows voters to split their voting
* power amongst the other 3 options.
*
* Votes cast with the Fractional support must be accompanied by a `params` argument that is three packed `uint128` values
* representing the weight the delegate assigns to Against, For, and Abstain respectively. For those votes cast for the other
* 3 options, the `params` argument must be empty.
*
* This is mostly useful when the delegate is a contract that implements its own rules for voting. These delegate-contracts
* can cast fractional votes according to the preferences of multiple entities delegating their voting power.
*
* Some example use cases include:
*
* * Voting from tokens that are held by a DeFi pool
* * 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]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any important differences that people should consider if they migrate from ScopeLift's implementation to this one, or if an interface has to support both? I think it may be worth listing them here.

Copy link
Member

@ernestognw ernestognw May 29, 2024

Choose a reason for hiding this comment

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

So far:

  • The reference implementation is in OpenZeppelin Contracts v4
  • It includes a nonce in the params object given v4 had no nonce protection
  • The COUNTING_MODDE is different

The voting format may vary after the reviews. I think it's currently fine since it reads it's "based on", so no compatibility should be assumed right away

*/
abstract contract GovernorCountingFractional is Governor {
using Math for *;

ernestognw marked this conversation as resolved.
Show resolved Hide resolved
uint8 internal constant VOTE_TYPE_FRACTIONAL = 255;

struct ProposalVote {
uint256 againstVotes;
uint256 forVotes;
uint256 abstainVotes;
mapping(address voter => uint256) usedVotes;
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @dev Mapping from proposal ID to vote tallies for that proposal.
*/
mapping(uint256 => ProposalVote) private _proposalVotes;

/**
* @dev A fractional vote params uses more votes than are available for that user.
*/
error GovernorExceedRemainingWeight(address voter, uint256 usedVotes, uint256 remainingWeight);

/**
* @dev See {IGovernor-COUNTING_MODE}.
*/
// solhint-disable-next-line func-name-mixedcase
function COUNTING_MODE() public pure virtual override returns (string memory) {
return "support=bravo,fractional&quorum=for,abstain&params=fractional";
}

/**
* @dev See {IGovernor-hasVoted}.
*/
function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) {
return usedVotes(proposalId, account) > 0;
}

/**
* @dev Get the number of votes already cast by `account` for a proposal with `proposalId`. Useful for
* integrations that allow delegates to cast rolling, partial votes.
*/
function usedVotes(uint256 proposalId, address account) public view virtual returns (uint256) {
return _proposalVotes[proposalId].usedVotes[account];
}

/**
* @dev Get current distribution of votes for a given proposal.
*/
function proposalVotes(
uint256 proposalId
) public view virtual returns (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) {
ProposalVote storage proposalVote = _proposalVotes[proposalId];
return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes);
}

/**
* @dev See {Governor-_quorumReached}.
*/
function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) {
ProposalVote storage proposalVote = _proposalVotes[proposalId];
return quorum(proposalSnapshot(proposalId)) <= proposalVote.forVotes + proposalVote.abstainVotes;
}

/**
* @dev See {Governor-_voteSucceeded}. In this module, forVotes must be > againstVotes.
*/
function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) {
ProposalVote storage proposalVote = _proposalVotes[proposalId];
return proposalVote.forVotes > proposalVote.againstVotes;
}

/**
* @dev See {Governor-_countVote}. Function that records the delegate's votes.
*
* Executing this function consumes (part of) the delegate's weight on the proposal. This weight can be
* distributed amongst the 3 options (Against, For, Abstain) by specifying a fractional `support`.
*
* This counting module supports two vote casting modes: nominal and fractional.
*
* - Nominal: A nominal vote is cast by setting `support` to one of the 3 bravo options (Against, For, Abstain).
* - Fractional: A fractional vote is cast by setting `support` to `type(uint8).max` (255).
*
* Casting a nominal vote requires `params` to be empty and consumes the delegate's full remaining weight on the
* proposal for the specified `support` option. This is similar to the {GovernorCountingSimple} module and follows
* the `VoteType` enum from Governor Bravo. As a consequence, no vote weight remains unspent so no further voting
* is possible (for this `proposalId` and this `account`).
*
* Casting a fractional vote consumes a fraction of the delegate's remaining weight on the proposal according to the
* weights the delegate assigns to each support option (Against, For, Abstain respectively). The sum total of the
* three decoded vote weights _must_ be less than or equal to the delegate's remaining weight on the proposal (i.e.
* their checkpointed total weight minus votes already cast on the proposal). This format can be produced using:
*
* `abi.encodePacked(uint128(againstVotes), uint128(forVotes), uint128(abstainVotes))`
*
* NOTE: Consider that fractional voting restricts the number of casted vote (in each category) to 128 bits.
* Depending on how many decimals the underlying token has, a single voter may require to split their vote into
* multiple vote operations. For precision higher than ~30 decimals, large token holders may require an
* potentially large number of calls to cast all their votes. The voter has the possibility to cast all the
* remaining votes in a single operation using the traditional "bravo" vote.
*/
// slither-disable-next-line cyclomatic-complexity
Copy link
Member

Choose a reason for hiding this comment

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

For the record, we both feel fine by keeping this function self contained

function _countVote(
uint256 proposalId,
address account,
uint8 support,
uint256 totalWeight,
bytes memory params
) internal virtual override returns (uint256) {
// Compute number of remaining votes. Returns 0 on overflow.
(, uint256 remainingWeight) = totalWeight.trySub(usedVotes(proposalId, account));
if (remainingWeight == 0) {
revert GovernorAlreadyCastVote(account);
}

uint256 againstVotes = 0;
uint256 forVotes = 0;
uint256 abstainVotes = 0;
uint256 usedWeight;

// For clarity of event indexing, fractional voting must be clearly advertised in the "support" field.
//
// Supported `support` value must be:
// - "Full" voting: `support = 0` (Against), `1` (For) or `2` (Abstain), with empty params.
// - "Fractional" voting: `support = 255`, with 48 bytes params.
if (support == uint8(GovernorCountingSimple.VoteType.Against)) {
if (params.length != 0) revert GovernorInvalidVoteParams();
usedWeight = againstVotes = remainingWeight;
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we agreed that this double assignment is clear enough

} else if (support == uint8(GovernorCountingSimple.VoteType.For)) {
if (params.length != 0) revert GovernorInvalidVoteParams();
usedWeight = forVotes = remainingWeight;
} else if (support == uint8(GovernorCountingSimple.VoteType.Abstain)) {
if (params.length != 0) revert GovernorInvalidVoteParams();
usedWeight = abstainVotes = remainingWeight;
} else if (support == VOTE_TYPE_FRACTIONAL) {
// The `params` argument is expected to be three packed `uint128`:
// `abi.encodePacked(uint128(againstVotes), uint128(forVotes), uint128(abstainVotes))`
if (params.length != 0x30) revert GovernorInvalidVoteParams();

assembly ("memory-safe") {
againstVotes := shr(128, mload(add(params, 0x20)))
forVotes := shr(128, mload(add(params, 0x30)))
abstainVotes := shr(128, mload(add(params, 0x40)))
usedWeight := add(add(againstVotes, forVotes), abstainVotes) // inputs are uint128: cannot overflow
}

// check parsed arguments are valid
if (usedWeight > remainingWeight) {
revert GovernorExceedRemainingWeight(account, usedWeight, remainingWeight);
}
} else {
revert GovernorInvalidVoteType();
}

// update votes tracking
ProposalVote storage details = _proposalVotes[proposalId];
if (againstVotes > 0) details.againstVotes += againstVotes;
if (forVotes > 0) details.forVotes += forVotes;
if (abstainVotes > 0) details.abstainVotes += abstainVotes;
details.usedVotes[account] += usedWeight;

return usedWeight;
}
}
12 changes: 7 additions & 5 deletions contracts/governance/extensions/GovernorCountingSimple.sol
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ abstract contract GovernorCountingSimple is Governor {
uint256 proposalId,
address account,
uint8 support,
uint256 weight,
uint256 totalWeight,
bytes memory // params
) internal virtual override {
) internal virtual override returns (uint256) {
ProposalVote storage proposalVote = _proposalVotes[proposalId];

if (proposalVote.hasVoted[account]) {
Expand All @@ -88,13 +88,15 @@ abstract contract GovernorCountingSimple is Governor {
proposalVote.hasVoted[account] = true;

if (support == uint8(VoteType.Against)) {
proposalVote.againstVotes += weight;
proposalVote.againstVotes += totalWeight;
} else if (support == uint8(VoteType.For)) {
proposalVote.forVotes += weight;
proposalVote.forVotes += totalWeight;
} else if (support == uint8(VoteType.Abstain)) {
proposalVote.abstainVotes += weight;
proposalVote.abstainVotes += totalWeight;
} else {
revert GovernorInvalidVoteType();
}

return totalWeight;
}
}
14 changes: 14 additions & 0 deletions contracts/mocks/governance/GovernorFractionalMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {Governor} from "../../governance/Governor.sol";
import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol";
import {GovernorCountingFractional} from "../../governance/extensions/GovernorCountingFractional.sol";
import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol";

abstract contract GovernorFractionalMock is GovernorSettings, GovernorVotesQuorumFraction, GovernorCountingFractional {
function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) {
return super.proposalThreshold();
}
}
2 changes: 1 addition & 1 deletion contracts/mocks/governance/GovernorWithParamsMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ abstract contract GovernorWithParamsMock is GovernorVotes, GovernorCountingSimpl
uint8 support,
uint256 weight,
bytes memory params
) internal override(Governor, GovernorCountingSimple) {
) internal override(Governor, GovernorCountingSimple) returns (uint256) {
if (params.length > 0) {
(uint256 _uintParam, string memory _strParam) = abi.decode(params, (uint256, string));
emit CountParams(_uintParam, _strParam);
Expand Down
2 changes: 1 addition & 1 deletion test/governance/Governor.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,5 @@ contract GovernorInternalTest is Test, Governor {

function _getVotes(address, uint256, bytes memory) internal pure virtual override returns (uint256) {}

function _countVote(uint256, address, uint8, uint256, bytes memory) internal virtual override {}
function _countVote(uint256, address, uint8, uint256, bytes memory) internal virtual override returns (uint256) {}
}
Loading