Delegate override vote #5160
Delegate override vote #5160
Conversation
|
| * @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 { |
There was a problem hiding this comment.
This appears to be a voting module. Should be named accordingly?
There was a problem hiding this comment.
Also, usually counting module does not inherit from GovernorVotes. Could we avoid that in heritance?
There was a problem hiding this comment.
It is both a voting module and a counting module. Maybe it could be something like GovernorCountingAndVotesOverrideDelegate?
There was a problem hiding this comment.
Yes it can be removed. The reason to inherit GovernorVotes is that this is strictly additional functionality to it and it makes inheriting GovernorVotesQuorumFraction simpler since it also inherits GovernorVotes.
There was a problem hiding this comment.
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.
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
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.
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:
| abstract contract GovernorOverrideDelegateVote is GovernorVotes { | |
| abstract contract GovernorCountOverridable is GovernorVotes { |
| if (proposalVote.overrideVoteReceipt[account].hasVoted) { | ||
| revert GovernorAlreadyCastVoteOverride(account); | ||
| } |
There was a problem hiding this comment.
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 forVotes; | ||
| uint256 abstainVotes; | ||
| mapping(address voter => VoteReceipt) voteReceipt; | ||
| mapping(address voter => VoteReceipt) overrideVoteReceipt; |
There was a problem hiding this comment.
Nobody is going to override the override ... so we should not need to track the support of the override, do we?
Feels to me that the override part of the Receipt only needs to be a boolean.
Also, could we use a single mapping for receipt, with the receipt structure being
- bool hasVoted
- uint8 voteSupport
- bool hasOverriden
- uint256 (or smaller?) overrideWeight
That would give better datalocality when we move to verkle.
Note, you could even merge hasVoted + support by storing support + 1 (here we control that support is never going to be 255, so overflow are not an issue)
There was a problem hiding this comment.
Nobody is going to override the override ... so we should not need to track the support of the override, do we? Feels to me that the override part of the Receipt only needs to be a boolean.
Yes this is correct. Also, given that snapshots within Votes are stored as uint208, we only need to allocate that to overrideWeight. So we could fit it all into one slot.
|
I'd like to discuss the use of vote with params, versus using dedicated functions |
| error GovernorAlreadyCastVoteOverride(address account); | ||
|
|
||
| mapping(uint256 proposalId => ProposalVote) private _proposalVotes; | ||
| mapping(address account => mapping(uint256 proposalId => uint256 votes)) private _overrideVoteWeight; |
There was a problem hiding this comment.
I think we can use the same ProposalVote structure to include this. In the end, a voter that overrides its delegate is emitting a vote too. That also brings better data locality for Verkle as @Amxx pointed out.
contracts/governance/extensions/GovernorOverrideDelegateVote.sol
Outdated
Show resolved
Hide resolved
contracts/governance/extensions/GovernorOverrideDelegateVote.sol
Outdated
Show resolved
Hide resolved
| mapping(address voter => VoteReceipt) voteReceipt; | ||
| } | ||
|
|
||
| error GovernorAlreadyCastVoteOverride(address account); |
There was a problem hiding this comment.
IMO that would be more "natural" to read, what do you think ?
| error GovernorAlreadyCastVoteOverride(address account); | |
| error GovernorVoteAlreadyOverriden(address account); |
No description provided.