-
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
Closed
Closed
Delegate override vote #5160
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9ad82b2
Initial commit
arr00 e5dc350
use only one extension. add counting
arr00 ed3da11
Update docs
arr00 4bc3b1d
Use existing castVoteWithReasonAndParams for override
arr00 c6a048b
fix: override per proposalId instead of timepoint
arr00 1e6e530
update comments
arr00 d7550a4
Use hardhat testing
arr00 5cdcb44
Inherit `GovernorVotes` to avoid code duplication
arr00 45cb330
suggestions
arr00 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
181 changes: 181 additions & 0 deletions
181
contracts/governance/extensions/GovernorOverrideDelegateVote.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,181 @@ | ||||||
// 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 { | ||||||
uint8 support; | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
bool hasOverriden; | ||||||
uint208 overrideWeight; | ||||||
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; | ||||||
} | ||||||
|
||||||
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; | ||||||
|
||||||
constructor(VotesOverridable tokenAddress) GovernorVotes(tokenAddress) {} | ||||||
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].support != 0; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @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].voteReceipt[account].hasOverriden; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @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); | ||||||
} | ||||||
|
||||||
ProposalVote storage proposalVote = _proposalVotes[proposalId]; | ||||||
|
||||||
totalWeight -= proposalVote.voteReceipt[account].overrideWeight; | ||||||
|
||||||
if (proposalVote.voteReceipt[account].support != 0) { | ||||||
revert GovernorAlreadyCastVote(account); | ||||||
} | ||||||
// Support tracks support and if a user voted. Store support + 1. | ||||||
proposalVote.voteReceipt[account].support = support + 1; | ||||||
|
||||||
_tallyVote(proposalVote, support, _add, totalWeight); | ||||||
|
||||||
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 = VotesOverridable(address(token())).getPastDelegate(account, proposalSnapshot); | ||||||
|
||||||
if (proposalVote.voteReceipt[account].hasOverriden) { | ||||||
revert GovernorAlreadyCastVoteOverride(account); | ||||||
} | ||||||
proposalVote.voteReceipt[account].hasOverriden = true; | ||||||
|
||||||
uint256 overrideWeight = VotesOverridable(address(token())).getPastBalanceOf(account, proposalSnapshot); | ||||||
_tallyVote(proposalVote, support, _add, overrideWeight); | ||||||
|
||||||
// Account for the delegate's vote | ||||||
VoteReceipt memory delegateVoteReceipt = proposalVote.voteReceipt[delegate]; | ||||||
if (delegateVoteReceipt.support != 0) { | ||||||
uint8 correctedSupport = delegateVoteReceipt.support - 1; | ||||||
// If delegate has voted, remove the delegatee's vote weight from their support | ||||||
_tallyVote(proposalVote, correctedSupport, _subtract, overrideWeight); | ||||||
|
||||||
// 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 | ||||||
proposalVote.voteReceipt[delegate].overrideWeight += uint208(overrideWeight); | ||||||
} | ||||||
return overrideWeight; | ||||||
} | ||||||
|
||||||
function _tallyVote( | ||||||
ProposalVote storage proposalVote, | ||||||
uint8 support, | ||||||
function(uint256, uint256) view returns (uint256) op, | ||||||
uint256 delta | ||||||
) private { | ||||||
if (support == uint8(VoteType.Against)) { | ||||||
proposalVote.againstVotes = op(proposalVote.againstVotes, delta); | ||||||
} else if (support == uint8(VoteType.For)) { | ||||||
proposalVote.forVotes = op(proposalVote.forVotes, delta); | ||||||
} else if (support == uint8(VoteType.Abstain)) { | ||||||
proposalVote.abstainVotes = op(proposalVote.abstainVotes, delta); | ||||||
} else { | ||||||
revert GovernorInvalidVoteType(); | ||||||
} | ||||||
} | ||||||
|
||||||
function _add(uint256 a, uint256 b) private pure returns (uint256) { | ||||||
return a + b; | ||||||
} | ||||||
|
||||||
function _subtract(uint256 a, uint256 b) private pure returns (uint256) { | ||||||
return a - b; | ||||||
} | ||||||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
// 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 { | ||
super._delegate(account, delegatee); | ||
|
||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_delegateCheckpoints[account].push(clock(), uint160(delegatee)); | ||
} | ||
|
||
/** | ||
* @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(_getVotingUnits(from))); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
if (to != address(0)) { | ||
Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; | ||
store.push(clock(), uint208(_getVotingUnits(to))); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
|
||
/** | ||
* @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)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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"; | ||
} | ||
} |
19 changes: 19 additions & 0 deletions
19
contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// 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); | ||
} | ||
} | ||
|
||
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"; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: