-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Delegate override vote #5160
Delegate override vote #5160
Changes from 8 commits
9ad82b2
e5dc350
ed3da11
4bc3b1d
c6a048b
1e6e530
d7550a4
5cdcb44
45cb330
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,194 @@ | ||||||
// SPDX-License-Identifier: MIT | ||||||
|
||||||
pragma solidity ^0.8.20; | ||||||
|
||||||
import {GovernorVotes} from "./GovernorVotes.sol"; | ||||||
import {IVotes} from "../utils/IVotes.sol"; | ||||||
import {IERC5805} from "../../interfaces/IERC5805.sol"; | ||||||
import {SafeCast} from "../../utils/math/SafeCast.sol"; | ||||||
import {Time} from "../../utils/types/Time.sol"; | ||||||
import {VotesOverridable} from "../utils/VotesOverridable.sol"; | ||||||
|
||||||
/** | ||||||
* @dev Extension of {Governor} which enables delegatees to override the vote of their delegates. This module requires a | ||||||
* token token that inherits `VotesOverridable`. | ||||||
*/ | ||||||
abstract contract GovernorOverrideDelegateVote is GovernorVotes { | ||||||
/** | ||||||
* @dev Supported vote types. Matches Governor Bravo ordering. | ||||||
*/ | ||||||
enum VoteType { | ||||||
Against, | ||||||
For, | ||||||
Abstain | ||||||
} | ||||||
|
||||||
struct VoteReceipt { | ||||||
bool hasVoted; | ||||||
uint8 support; | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
|
||||||
struct ProposalVote { | ||||||
uint256 againstVotes; | ||||||
uint256 forVotes; | ||||||
uint256 abstainVotes; | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
mapping(address voter => VoteReceipt) voteReceipt; | ||||||
mapping(address voter => VoteReceipt) overrideVoteReceipt; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nobody is going to override the override ... so we should not need to track the support of the override, do we? Also, could we use a single mapping for receipt, with the receipt structure being
That would give better datalocality when we move to verkle. Note, you could even merge hasVoted + support by storing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes this is correct. Also, given that snapshots within |
||||||
} | ||||||
|
||||||
error GovernorAlreadyCastVoteOverride(address account); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO that would be more "natural" to read, what do you think ?
Suggested change
|
||||||
|
||||||
mapping(uint256 proposalId => ProposalVote) private _proposalVotes; | ||||||
mapping(address account => mapping(uint256 proposalId => uint256 votes)) private _overrideVoteWeight; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can use the same |
||||||
|
||||||
constructor(VotesOverridable tokenAddress) GovernorVotes(tokenAddress) {} | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
/** | ||||||
* @dev Fetch the past delegate for an `account` at a given `timepoint` from the token. | ||||||
*/ | ||||||
function _getPastDelegate(address account, uint256 timepoint) internal view virtual returns (address) { | ||||||
return VotesOverridable(address(token())).getPastDelegate(account, timepoint); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @dev Fetch the past `balanceOf` for an `account` at a given `timepoint` from the token. | ||||||
*/ | ||||||
function _getPastBalanceOf(address account, uint256 timepoint) internal view virtual returns (uint256) { | ||||||
return VotesOverridable(address(token())).getPastBalanceOf(account, timepoint); | ||||||
} | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
/** | ||||||
* @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,override&quorum=for,abstain¶ms=override"; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @dev See {IGovernor-hasVoted}. | ||||||
*/ | ||||||
function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { | ||||||
return _proposalVotes[proposalId].voteReceipt[account].hasVoted; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @dev Check if an `account` has overridden their delegate for a proposal. | ||||||
*/ | ||||||
function hasVotedOverride(uint256 proposalId, address account) public view virtual returns (bool) { | ||||||
return _proposalVotes[proposalId].overrideVoteReceipt[account].hasVoted; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @dev Accessor to the internal vote counts. | ||||||
*/ | ||||||
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, the forVotes must be strictly over the againstVotes. | ||||||
*/ | ||||||
function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) { | ||||||
ProposalVote storage proposalVote = _proposalVotes[proposalId]; | ||||||
|
||||||
return proposalVote.forVotes > proposalVote.againstVotes; | ||||||
} | ||||||
|
||||||
function _countVote( | ||||||
uint256 proposalId, | ||||||
address account, | ||||||
uint8 support, | ||||||
uint256 totalWeight, | ||||||
bytes memory params | ||||||
) internal virtual override returns (uint256) { | ||||||
if (keccak256(params) == keccak256(hex"23b70c8d0000000000000000000000000000000000000000")) { | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return _countVotesOverride(proposalId, account, support, params); | ||||||
} | ||||||
|
||||||
totalWeight -= _overrideVoteWeight[account][proposalId]; | ||||||
|
||||||
ProposalVote storage proposalVote = _proposalVotes[proposalId]; | ||||||
|
||||||
if (proposalVote.voteReceipt[account].hasVoted) { | ||||||
revert GovernorAlreadyCastVote(account); | ||||||
} | ||||||
proposalVote.voteReceipt[account] = VoteReceipt({hasVoted: true, support: support}); | ||||||
|
||||||
if (support == uint8(VoteType.Against)) { | ||||||
proposalVote.againstVotes += totalWeight; | ||||||
} else if (support == uint8(VoteType.For)) { | ||||||
proposalVote.forVotes += totalWeight; | ||||||
} else if (support == uint8(VoteType.Abstain)) { | ||||||
proposalVote.abstainVotes += totalWeight; | ||||||
} else { | ||||||
revert GovernorInvalidVoteType(); | ||||||
} | ||||||
|
||||||
return totalWeight; | ||||||
} | ||||||
|
||||||
function _countVotesOverride( | ||||||
uint256 proposalId, | ||||||
address account, | ||||||
uint8 support, | ||||||
bytes memory params | ||||||
) private returns (uint256) { | ||||||
ProposalVote storage proposalVote = _proposalVotes[proposalId]; | ||||||
uint256 proposalSnapshot = proposalSnapshot(proposalId); | ||||||
address delegate = _getPastDelegate(account, proposalSnapshot); | ||||||
|
||||||
if (proposalVote.overrideVoteReceipt[account].hasVoted) { | ||||||
revert GovernorAlreadyCastVoteOverride(account); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a check to ensure that the delegatee does not override vote twice. An override vote can come at any time when voting is allowed. |
||||||
|
||||||
uint256 overrideWeight = _getPastBalanceOf(account, proposalSnapshot); | ||||||
|
||||||
proposalVote.overrideVoteReceipt[account] = VoteReceipt({hasVoted: true, support: support}); | ||||||
if (support == uint8(VoteType.Against)) { | ||||||
proposalVote.againstVotes += overrideWeight; | ||||||
} else if (support == uint8(VoteType.For)) { | ||||||
proposalVote.forVotes += overrideWeight; | ||||||
} else if (support == uint8(VoteType.Abstain)) { | ||||||
proposalVote.abstainVotes += overrideWeight; | ||||||
} else { | ||||||
revert GovernorInvalidVoteType(); | ||||||
} | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
// Account for the delegate's vote | ||||||
VoteReceipt memory delegateVoteReceipt = proposalVote.voteReceipt[delegate]; | ||||||
if (delegateVoteReceipt.hasVoted) { | ||||||
// If delegate has voted, remove the delegatee's vote weight from their support | ||||||
if (delegateVoteReceipt.support == uint8(VoteType.Against)) { | ||||||
proposalVote.againstVotes -= overrideWeight; | ||||||
} else if (delegateVoteReceipt.support == uint8(VoteType.For)) { | ||||||
proposalVote.forVotes -= overrideWeight; | ||||||
} else if (delegateVoteReceipt.support == uint8(VoteType.Abstain)) { | ||||||
proposalVote.abstainVotes -= overrideWeight; | ||||||
} else { | ||||||
revert GovernorInvalidVoteType(); | ||||||
} | ||||||
|
||||||
// Write delegate into the params for event | ||||||
assembly { | ||||||
mstore(add(params, 0x20), or(mload(add(params, 0x20)), shl(64, delegate))) | ||||||
} | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} else { | ||||||
// Only write override weight if they have not voted yet | ||||||
_overrideVoteWeight[delegate][proposalId] += overrideWeight; | ||||||
} | ||||||
return overrideWeight; | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.20; | ||
|
||
import {Checkpoints} from "../../utils/structs/Checkpoints.sol"; | ||
import {Votes} from "./Votes.sol"; | ||
import {SafeCast} from "../../utils/math/SafeCast.sol"; | ||
|
||
/** | ||
* @dev Extension of {Votes} that adds support for checkpointed delegations and balances. This is required | ||
* to use the `GovernorOverrideDelegateVote` extension. | ||
*/ | ||
abstract contract VotesOverridable is Votes { | ||
using Checkpoints for Checkpoints.Trace208; | ||
|
||
error VotesOverridableFutureLookup(uint256 timepoint, uint256 currentTimepoint); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints; | ||
mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints; | ||
|
||
function _delegate(address account, address delegatee) internal virtual override { | ||
address oldDelegate = delegates(account); | ||
|
||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_delegateCheckpoints[account].push(clock(), uint160(delegatee)); | ||
|
||
emit DelegateChanged(account, oldDelegate, delegatee); | ||
_moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); | ||
} | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* @inheritdoc Votes | ||
*/ | ||
function delegates(address delegatee) public view virtual override returns (address) { | ||
return address(uint160(_delegateCheckpoints[delegatee].latest())); | ||
} | ||
|
||
/** | ||
* @dev Returns the delegate of an `account` at a specific moment in the past. If the `clock()` is | ||
* configured to use block numbers, this will return the value at the end of the corresponding block. | ||
* | ||
* Requirements: | ||
* | ||
* - `timepoint` must be in the past. If operating using block numbers, the block must be already mined. | ||
*/ | ||
function getPastDelegate(address account, uint256 timepoint) public view returns (address) { | ||
uint48 currentTimepoint = clock(); | ||
if (timepoint >= currentTimepoint) { | ||
revert VotesOverridableFutureLookup(timepoint, currentTimepoint); | ||
} | ||
return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* @dev Extend functionality of the function by checkpointing balances. | ||
*/ | ||
function _transferVotingUnits(address from, address to, uint256 amount) internal virtual override { | ||
super._transferVotingUnits(from, to, amount); | ||
if (from != to) { | ||
if (from != address(0)) { | ||
Checkpoints.Trace208 storage store = _balanceOfCheckpoints[from]; | ||
store.push(clock(), uint208(store.latest() - amount)); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if (to != address(0)) { | ||
Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; | ||
store.push(clock(), uint208(store.latest() + amount)); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* @dev Returns the `balanceOf` of an `account` at a specific moment in the past. If the `clock()` is | ||
* configured to use block numbers, this will return the value at the end of the corresponding block. | ||
* | ||
* Requirements: | ||
* | ||
* - `timepoint` must be in the past. If operating using block numbers, the block must be already mined. | ||
*/ | ||
function getPastBalanceOf(address account, uint256 timepoint) public view returns (uint256) { | ||
uint48 currentTimepoint = clock(); | ||
if (timepoint >= currentTimepoint) { | ||
// Note this ERC is not relevant to the specific error. Should probably be a different error. | ||
revert VotesOverridableFutureLookup(timepoint, currentTimepoint); | ||
} | ||
return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {VotesOverridable} from "../governance/utils/VotesOverridable.sol"; | ||
|
||
abstract contract VotesOverridableMock is VotesOverridable { | ||
mapping(address voter => uint256) private _votingUnits; | ||
|
||
function getTotalSupply() public view returns (uint256) { | ||
return _getTotalSupply(); | ||
} | ||
|
||
function delegate(address account, address newDelegation) public { | ||
return _delegate(account, newDelegation); | ||
} | ||
|
||
function _getVotingUnits(address account) internal view override returns (uint256) { | ||
return _votingUnits[account]; | ||
} | ||
|
||
function _mint(address account, uint256 votes) internal { | ||
_votingUnits[account] += votes; | ||
_transferVotingUnits(address(0), account, votes); | ||
} | ||
|
||
function _burn(address account, uint256 votes) internal { | ||
_votingUnits[account] += votes; | ||
_transferVotingUnits(account, address(0), votes); | ||
} | ||
} | ||
|
||
abstract contract VotesOverridableTimestampMock is VotesOverridableMock { | ||
function clock() public view override returns (uint48) { | ||
return uint48(block.timestamp); | ||
} | ||
|
||
// solhint-disable-next-line func-name-mixedcase | ||
function CLOCK_MODE() public view virtual override returns (string memory) { | ||
return "mode=timestamp"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.20; | ||
|
||
import {Governor} from "../../governance/Governor.sol"; | ||
import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol"; | ||
import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; | ||
import {GovernorOverrideDelegateVote, VotesOverridable} from "../../governance/extensions/GovernorOverrideDelegateVote.sol"; | ||
import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; | ||
|
||
abstract contract GovernorOverrideDelegateVoteMock is | ||
GovernorSettings, | ||
GovernorVotesQuorumFraction, | ||
GovernorOverrideDelegateVote | ||
{ | ||
function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { | ||
return super.proposalThreshold(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.20; | ||
|
||
import {ERC20Votes} from "../../token/ERC20/extensions/ERC20Votes.sol"; | ||
import {VotesOverridable, Votes} from "../../governance/utils/VotesOverridable.sol"; | ||
import {SafeCast} from "../../utils/math/SafeCast.sol"; | ||
|
||
abstract contract ERC20VotesOverridableMock is ERC20Votes, VotesOverridable { | ||
function _delegate(address account, address delegatee) internal virtual override(Votes, VotesOverridable) { | ||
return super._delegate(account, delegatee); | ||
} | ||
|
||
function _transferVotingUnits( | ||
address from, | ||
address to, | ||
uint256 amount | ||
) internal virtual override(Votes, VotesOverridable) { | ||
return super._transferVotingUnits(from, to, amount); | ||
} | ||
|
||
function delegates(address delegatee) public view virtual override(Votes, VotesOverridable) returns (address) { | ||
return super.delegates(delegatee); | ||
} | ||
} | ||
|
||
abstract contract ERC20VotesOverridableTimestampMock is ERC20VotesOverridableMock { | ||
function clock() public view virtual override returns (uint48) { | ||
return SafeCast.toUint48(block.timestamp); | ||
} | ||
|
||
// solhint-disable-next-line func-name-mixedcase | ||
function CLOCK_MODE() public view virtual override returns (string memory) { | ||
return "mode=timestamp"; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a voting module. Should be named accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, usually counting module does not inherit from GovernorVotes. Could we avoid that in heritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is both a voting module and a counting module. Maybe it could be something like
GovernorCountingAndVotesOverrideDelegate
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can be removed. The reason to inherit
GovernorVotes
is that this is strictly additional functionality to it and it makes inheritingGovernorVotesQuorumFraction
simpler since it also inheritsGovernorVotes
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's not necessary to inherit from
GovernorVotes
, a developer building on Governor would pick a votes module and a counting module, while the quorum fraction is optional.In such case, I would name it
GovernorCountingOverride
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could theoretically isolate this to be just a counting module and have a different voting module which implements
_getPastDelegate
and_getPastBalanceOf
. This module would then be dependent on the usage of that voting module. Since they would be so intertwined and relatively useless on their own, I chose this approach which is a voting and counting module in one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think inheriting from GovernorVotes to get acces to
token()
is fine.For the name I think it should be succinct yet explicit about what it achieves. I think the current name may be a bit too long.
This is just a proposition: