From 9ad82b26927c7466278e76be1536582a7bbcfee5 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 12 Aug 2024 00:49:06 -0400 Subject: [PATCH 1/9] Initial commit --- .../GovernorOverrideDelegateVote.sol | 72 +++++++++++++++++++ .../utils/VotesSnapshotDelegates.sol | 33 +++++++++ .../ERC20/extensions/ERC20SnapshotBalance.sol | 57 +++++++++++++++ .../extensions/ERC20VotesOverridable.sol | 33 +++++++++ 4 files changed, 195 insertions(+) create mode 100644 contracts/governance/extensions/GovernorOverrideDelegateVote.sol create mode 100644 contracts/governance/utils/VotesSnapshotDelegates.sol create mode 100644 contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol create mode 100644 contracts/token/ERC20/extensions/ERC20VotesOverridable.sol diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol new file mode 100644 index 00000000000..d5f1db78d2a --- /dev/null +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Governor} from "../Governor.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 {ERC20VotesOverridable} from "../../token/ERC20/extensions/ERC20VotesOverridable.sol"; + +/** + * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} + * token along with the ability to get checkpointed delegates. + */ +abstract contract GovernorOverrideDelegateVote is Governor { + ERC20VotesOverridable private immutable _token; + + constructor(ERC20VotesOverridable tokenAddress) { + _token = tokenAddress; + } + + /** + * @dev The token that voting power is sourced from. + */ + function token() public view virtual returns (ERC20VotesOverridable) { + return _token; + } + + /** + * @dev Clock (as specified in ERC-6372) is set to match the token's clock. Fallback to block numbers if the token + * does not implement ERC-6372. + */ + function clock() public view virtual override returns (uint48) { + try token().clock() returns (uint48 timepoint) { + return timepoint; + } catch { + return Time.blockNumber(); + } + } + + /** + * @dev Machine-readable description of the clock as specified in ERC-6372. + */ + // solhint-disable-next-line func-name-mixedcase + function CLOCK_MODE() public view virtual override returns (string memory) { + try token().CLOCK_MODE() returns (string memory clockmode) { + return clockmode; + } catch { + return "mode=blocknumber&from=default"; + } + } + + /** + * Read the voting weight from the token's built in snapshot mechanism (see {Governor-_getVotes}). + */ + function _getVotes( + address account, + uint256 timepoint, + bytes memory /*params*/ + ) internal view virtual override returns (uint256) { + return token().getPastVotes(account, timepoint); + } + + function _getDelegate(address account, uint256 timepoint) internal view virtual returns (address) { + return token().getPastDelegate(account, timepoint); + } + + function _getPastBalanceOf(address account, uint256 timepoint) internal view virtual returns (uint256) { + return token().getPastBalanceOf(account, timepoint); + } +} diff --git a/contracts/governance/utils/VotesSnapshotDelegates.sol b/contracts/governance/utils/VotesSnapshotDelegates.sol new file mode 100644 index 00000000000..16622b698a9 --- /dev/null +++ b/contracts/governance/utils/VotesSnapshotDelegates.sol @@ -0,0 +1,33 @@ +// 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"; + +abstract contract VotesSnapshotDelegates is Votes { + using Checkpoints for Checkpoints.Trace208; + + mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints; + + function _delegate(address account, address delegatee) internal override { + address oldDelegate = delegates(account); + + _delegateCheckpoints[account].push(clock(), uint160(delegatee)); + + emit DelegateChanged(account, oldDelegate, delegatee); + _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); + } + + function delegates(address delegatee) public view override returns (address) { + return address(uint160(_delegateCheckpoints[delegatee].latest())); + } + + function getPastDelegate(address account, uint256 timepoint) public view returns (address) { + uint48 currentTimepoint = clock(); + if (timepoint >= currentTimepoint) { + revert ERC5805FutureLookup(timepoint, currentTimepoint); + } + return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); + } +} diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol b/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol new file mode 100644 index 00000000000..b15405ca1c1 --- /dev/null +++ b/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; +import {SafeCast} from "../../../utils/math/SafeCast.sol"; + +import {IERC20Permit} from "./IERC20Permit.sol"; +import {ERC20} from "../ERC20.sol"; +import {ECDSA} from "../../../utils/cryptography/ECDSA.sol"; +import {EIP712} from "../../../utils/cryptography/EIP712.sol"; +import {Nonces} from "../../../utils/Nonces.sol"; +import {IERC6372} from "../../../interfaces/IERC6372.sol"; +import {Time} from "../../../utils/types/Time.sol"; + +abstract contract ERC20SnapshotBalance is ERC20, IERC6372 { + using Checkpoints for Checkpoints.Trace208; + mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints; + + error ERC6372FutureLookup(uint256 timepoint, uint48 clock); + + /** + * @notice Returns the balance of an account at a specific timepoint. + * @param account The address of the account to check. + * @param timepoint The timepoint to check the balance at (by default block number). + */ + function getPastBalanceOf(address account, uint256 timepoint) public view returns (uint256) { + uint48 currentTimepoint = clock(); + if (timepoint >= currentTimepoint) { + revert ERC6372FutureLookup(timepoint, currentTimepoint); + } + return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); + } + + /** + * @dev Adds historical checkpoints to the balance of each account. + */ + function _update(address from, address to, uint256 value) internal virtual override { + if (from != to) { + if (from != address(0)) { + Checkpoints.Trace208 storage store = _balanceOfCheckpoints[from]; + store.push(clock(), uint208(store.latest() - value)); + } + if (to != address(0)) { + Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; + store.push(clock(), uint208(store.latest() + value)); + } + } + super._update(from, to, value); + } + + /** + * @dev Clock used for flagging checkpoints. Can be overridden to implement timestamp based + * checkpoints (and voting), in which case {CLOCK_MODE} should be overridden as well to match. + */ + function clock() public view virtual returns (uint48); +} diff --git a/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol b/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol new file mode 100644 index 00000000000..47dfa88e6f8 --- /dev/null +++ b/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/extensions/ERC20Votes.sol) + +pragma solidity ^0.8.20; + +import {ERC20} from "../ERC20.sol"; +import {Votes} from "../../../governance/utils/Votes.sol"; +import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; +import {ERC20Votes} from "./ERC20Votes.sol"; +import {ERC20SnapshotBalance} from "./ERC20SnapshotBalance.sol"; +import {VotesSnapshotDelegates} from "../../../governance/utils/VotesSnapshotDelegates.sol"; + +abstract contract ERC20VotesOverridable is ERC20Votes, VotesSnapshotDelegates, ERC20SnapshotBalance { + function _delegate(address account, address delegatee) internal override(Votes, VotesSnapshotDelegates) { + super._delegate(account, delegatee); + } + + function delegates(address account) public view override(Votes, VotesSnapshotDelegates) returns (address) { + return super.delegates(account); + } + + function _update( + address from, + address to, + uint256 value + ) internal virtual override(ERC20Votes, ERC20SnapshotBalance) { + super._update(from, to, value); + } + + function clock() public view virtual override(Votes, ERC20SnapshotBalance) returns (uint48) { + super.clock(); + } +} From e5dc35046ddbb64b01b32e5e68419626a26de5df Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Tue, 13 Aug 2024 23:24:31 -0400 Subject: [PATCH 2/9] use only one extension. add counting --- .../GovernorOverrideDelegateVote.sol | 164 +++++++++++++++++- .../governance/utils/VotesOverridable.sol | 62 +++++++ .../utils/VotesSnapshotDelegates.sol | 33 ---- .../ERC20/extensions/ERC20SnapshotBalance.sol | 57 ------ .../extensions/ERC20VotesOverridable.sol | 23 +-- 5 files changed, 230 insertions(+), 109 deletions(-) create mode 100644 contracts/governance/utils/VotesOverridable.sol delete mode 100644 contracts/governance/utils/VotesSnapshotDelegates.sol delete mode 100644 contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index d5f1db78d2a..6ab6f8b3790 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -7,23 +7,51 @@ 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 {ERC20VotesOverridable} from "../../token/ERC20/extensions/ERC20VotesOverridable.sol"; +import {VotesOverridable} from "../utils/VotesOverridable.sol"; /** * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} * token along with the ability to get checkpointed delegates. */ abstract contract GovernorOverrideDelegateVote is Governor { - ERC20VotesOverridable private immutable _token; + /** + * @dev Supported vote types. Matches Governor Bravo ordering. + */ + enum VoteType { + Against, + For, + Abstain + } + + struct VoteReceipt { + bool hasVoted; + uint8 support; + } + + struct ProposalVote { + uint256 againstVotes; + uint256 forVotes; + uint256 abstainVotes; + mapping(address voter => VoteReceipt) voteReceipt; + mapping(address voter => VoteReceipt) overrideVoteReceipt; + } - constructor(ERC20VotesOverridable tokenAddress) { + error GovernorAlreadyCastVoteOverride(address account); + + mapping(uint256 proposalId => ProposalVote) private _proposalVotes; + mapping(address account => mapping(uint256 timepoint => uint256 votes)) private _overrideVoteWeight; + + VotesOverridable private immutable _token; + + constructor(VotesOverridable tokenAddress) { _token = tokenAddress; } + /// MARK: Votes logic /** * @dev The token that voting power is sourced from. */ - function token() public view virtual returns (ERC20VotesOverridable) { + function token() public view virtual returns (VotesOverridable) { return _token; } @@ -59,14 +87,140 @@ abstract contract GovernorOverrideDelegateVote is Governor { uint256 timepoint, bytes memory /*params*/ ) internal view virtual override returns (uint256) { + uint256 overrideVotes = _overrideVoteWeight[account][timepoint]; + if (overrideVotes != 0) { + return overrideVotes; + } return token().getPastVotes(account, timepoint); } - function _getDelegate(address account, uint256 timepoint) internal view virtual returns (address) { + function _getPastDelegate(address account, uint256 timepoint) internal view virtual returns (address) { return token().getPastDelegate(account, timepoint); } function _getPastBalanceOf(address account, uint256 timepoint) internal view virtual returns (uint256) { return token().getPastBalanceOf(account, timepoint); } + + /// MARK: Counting + /** + * @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&quorum=for,abstain"; + } + + /** + * @dev See {IGovernor-hasVoted}. + */ + function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { + return _proposalVotes[proposalId].voteReceipt[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) { + 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; + } + + /// MARK: Exposed override voting functions + function castVoteOverride(uint256 proposalId, uint8 support) public { + _castVoteOverride(proposalId, msg.sender, support, "", ""); + } + + function _castVoteOverride( + uint256 proposalId, + address account, + uint8 support, + string memory reason, + bytes memory params + ) private returns (uint256) { + ProposalVote storage proposalVote = _proposalVotes[proposalId]; + uint256 proposalSnapshot = proposalSnapshot(proposalId); + + address delegate = _getPastDelegate(account, proposalSnapshot); + uint256 totalWeight = _getPastBalanceOf(account, proposalSnapshot); + + if (proposalVote.overrideVoteReceipt[account].hasVoted) { + revert GovernorAlreadyCastVoteOverride(account); + } + + proposalVote.overrideVoteReceipt[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; + } + + // 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 -= totalWeight; + } else if (delegateVoteReceipt.support == uint8(VoteType.For)) { + proposalVote.forVotes -= totalWeight; + } else if (delegateVoteReceipt.support == uint8(VoteType.Abstain)) { + proposalVote.abstainVotes -= totalWeight; + } + } else { + // Only write override weight if they have not voted yet + _overrideVoteWeight[delegate][proposalSnapshot] = + _getVotes(delegate, proposalSnapshot, params) - + totalWeight; + } + return totalWeight; + } } diff --git a/contracts/governance/utils/VotesOverridable.sol b/contracts/governance/utils/VotesOverridable.sol new file mode 100644 index 00000000000..16f86b2b1ee --- /dev/null +++ b/contracts/governance/utils/VotesOverridable.sol @@ -0,0 +1,62 @@ +// 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"; + +abstract contract VotesOverridable is Votes { + using Checkpoints for Checkpoints.Trace208; + + 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); + + _delegateCheckpoints[account].push(clock(), uint160(delegatee)); + + emit DelegateChanged(account, oldDelegate, delegatee); + _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); + } + + function delegates(address delegatee) public view virtual override returns (address) { + return address(uint160(_delegateCheckpoints[delegatee].latest())); + } + + function getPastDelegate(address account, uint256 timepoint) public view returns (address) { + uint48 currentTimepoint = clock(); + if (timepoint >= currentTimepoint) { + revert ERC5805FutureLookup(timepoint, currentTimepoint); + } + return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); + } + + 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)); + } + if (to != address(0)) { + Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; + store.push(clock(), uint208(store.latest() + amount)); + } + } + } + + /** + * @notice Returns the balance of an account at a specific timepoint. + * @param account The address of the account to check. + * @param timepoint The timepoint to check the balance at (by default block number). + */ + 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 ERC5805FutureLookup(timepoint, currentTimepoint); + } + return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); + } +} diff --git a/contracts/governance/utils/VotesSnapshotDelegates.sol b/contracts/governance/utils/VotesSnapshotDelegates.sol deleted file mode 100644 index 16622b698a9..00000000000 --- a/contracts/governance/utils/VotesSnapshotDelegates.sol +++ /dev/null @@ -1,33 +0,0 @@ -// 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"; - -abstract contract VotesSnapshotDelegates is Votes { - using Checkpoints for Checkpoints.Trace208; - - mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints; - - function _delegate(address account, address delegatee) internal override { - address oldDelegate = delegates(account); - - _delegateCheckpoints[account].push(clock(), uint160(delegatee)); - - emit DelegateChanged(account, oldDelegate, delegatee); - _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); - } - - function delegates(address delegatee) public view override returns (address) { - return address(uint160(_delegateCheckpoints[delegatee].latest())); - } - - function getPastDelegate(address account, uint256 timepoint) public view returns (address) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); - } - return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); - } -} diff --git a/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol b/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol deleted file mode 100644 index b15405ca1c1..00000000000 --- a/contracts/token/ERC20/extensions/ERC20SnapshotBalance.sol +++ /dev/null @@ -1,57 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; -import {SafeCast} from "../../../utils/math/SafeCast.sol"; - -import {IERC20Permit} from "./IERC20Permit.sol"; -import {ERC20} from "../ERC20.sol"; -import {ECDSA} from "../../../utils/cryptography/ECDSA.sol"; -import {EIP712} from "../../../utils/cryptography/EIP712.sol"; -import {Nonces} from "../../../utils/Nonces.sol"; -import {IERC6372} from "../../../interfaces/IERC6372.sol"; -import {Time} from "../../../utils/types/Time.sol"; - -abstract contract ERC20SnapshotBalance is ERC20, IERC6372 { - using Checkpoints for Checkpoints.Trace208; - mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints; - - error ERC6372FutureLookup(uint256 timepoint, uint48 clock); - - /** - * @notice Returns the balance of an account at a specific timepoint. - * @param account The address of the account to check. - * @param timepoint The timepoint to check the balance at (by default block number). - */ - function getPastBalanceOf(address account, uint256 timepoint) public view returns (uint256) { - uint48 currentTimepoint = clock(); - if (timepoint >= currentTimepoint) { - revert ERC6372FutureLookup(timepoint, currentTimepoint); - } - return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); - } - - /** - * @dev Adds historical checkpoints to the balance of each account. - */ - function _update(address from, address to, uint256 value) internal virtual override { - if (from != to) { - if (from != address(0)) { - Checkpoints.Trace208 storage store = _balanceOfCheckpoints[from]; - store.push(clock(), uint208(store.latest() - value)); - } - if (to != address(0)) { - Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; - store.push(clock(), uint208(store.latest() + value)); - } - } - super._update(from, to, value); - } - - /** - * @dev Clock used for flagging checkpoints. Can be overridden to implement timestamp based - * checkpoints (and voting), in which case {CLOCK_MODE} should be overridden as well to match. - */ - function clock() public view virtual returns (uint48); -} diff --git a/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol b/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol index 47dfa88e6f8..d3239592e68 100644 --- a/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol +++ b/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol @@ -7,27 +7,22 @@ import {ERC20} from "../ERC20.sol"; import {Votes} from "../../../governance/utils/Votes.sol"; import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; import {ERC20Votes} from "./ERC20Votes.sol"; -import {ERC20SnapshotBalance} from "./ERC20SnapshotBalance.sol"; -import {VotesSnapshotDelegates} from "../../../governance/utils/VotesSnapshotDelegates.sol"; +import {VotesOverridable} from "../../../governance/utils/VotesOverridable.sol"; -abstract contract ERC20VotesOverridable is ERC20Votes, VotesSnapshotDelegates, ERC20SnapshotBalance { - function _delegate(address account, address delegatee) internal override(Votes, VotesSnapshotDelegates) { +abstract contract ERC20VotesOverridable is ERC20Votes, VotesOverridable { + function _delegate(address account, address delegatee) internal override(Votes, VotesOverridable) { super._delegate(account, delegatee); } - function delegates(address account) public view override(Votes, VotesSnapshotDelegates) returns (address) { - return super.delegates(account); - } - - function _update( + function _transferVotingUnits( address from, address to, - uint256 value - ) internal virtual override(ERC20Votes, ERC20SnapshotBalance) { - super._update(from, to, value); + uint256 amount + ) internal virtual override(Votes, VotesOverridable) { + super._transferVotingUnits(from, to, amount); } - function clock() public view virtual override(Votes, ERC20SnapshotBalance) returns (uint48) { - super.clock(); + function delegates(address delegatee) public view override(Votes, VotesOverridable) returns (address) { + return super.delegates(delegatee); } } From ed3da11729fd34ca9ea4c263c4d9bf2c42311749 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Sat, 17 Aug 2024 20:00:15 -0400 Subject: [PATCH 3/9] Update docs --- .../extensions/GovernorOverrideDelegateVote.sol | 6 +++--- contracts/governance/utils/VotesOverridable.sol | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index 6ab6f8b3790..1a5ad0fda31 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -10,8 +10,8 @@ import {Time} from "../../utils/types/Time.sol"; import {VotesOverridable} from "../utils/VotesOverridable.sol"; /** - * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} - * token along with the ability to get checkpointed delegates. + * @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 Governor { /** @@ -108,7 +108,7 @@ abstract contract GovernorOverrideDelegateVote is Governor { */ // solhint-disable-next-line func-name-mixedcase function COUNTING_MODE() public pure virtual override returns (string memory) { - return "support=bravo&quorum=for,abstain"; + return "support=bravo,override&quorum=for,abstain¶ms=override"; } /** diff --git a/contracts/governance/utils/VotesOverridable.sol b/contracts/governance/utils/VotesOverridable.sol index 16f86b2b1ee..61bd1de9996 100644 --- a/contracts/governance/utils/VotesOverridable.sol +++ b/contracts/governance/utils/VotesOverridable.sol @@ -20,10 +20,21 @@ abstract contract VotesOverridable is Votes { _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); } + /** + * @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) { @@ -32,6 +43,9 @@ abstract contract VotesOverridable is Votes { return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); } + /** + * @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) { From 4bc3b1db4db4687b8998ac753951eeb202bec6ae Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Sat, 17 Aug 2024 22:31:30 -0400 Subject: [PATCH 4/9] Use existing castVoteWithReasonAndParams for override --- .../GovernorOverrideDelegateVote.sol | 29 ++++++---- .../mocks/governance/GovernorOverrideMock.sol | 29 ++++++++++ .../mocks/token/ERC20VotesOverridableMock.sol | 34 ++++++++++++ .../GovernorOverrideDelegateVote.t.sol | 53 +++++++++++++++++++ test/governance/utils/VotesOverridable.t.sol | 52 ++++++++++++++++++ 5 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 contracts/mocks/governance/GovernorOverrideMock.sol create mode 100644 contracts/mocks/token/ERC20VotesOverridableMock.sol create mode 100644 test/governance/extensions/GovernorOverrideDelegateVote.t.sol create mode 100644 test/governance/utils/VotesOverridable.t.sol diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index 1a5ad0fda31..af1be49a5e8 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -85,10 +85,16 @@ abstract contract GovernorOverrideDelegateVote is Governor { function _getVotes( address account, uint256 timepoint, - bytes memory /*params*/ + bytes memory params ) internal view virtual override returns (uint256) { + // If the params are the selector `0x23b70c8d` (keccak("override")[0:8]) followed by address 0, it is an override vote. + if (keccak256(params) == keccak256(bytes(hex"23b70c8d0000000000000000000000000000000000000000"))) { + // This is an override vote + return _getPastBalanceOf(account, timepoint); + } uint256 overrideVotes = _overrideVoteWeight[account][timepoint]; if (overrideVotes != 0) { + // This delegate has been overridden for this vote. Return the reduced voting weight. return overrideVotes; } return token().getPastVotes(account, timepoint); @@ -151,8 +157,12 @@ abstract contract GovernorOverrideDelegateVote is Governor { address account, uint8 support, uint256 totalWeight, - bytes memory // params + bytes memory params ) internal virtual override returns (uint256) { + if (keccak256(params) == keccak256(hex"23b70c8d0000000000000000000000000000000000000000")) { + return _countVotesOverride(proposalId, account, support, totalWeight, params); + } + ProposalVote storage proposalVote = _proposalVotes[proposalId]; if (proposalVote.voteReceipt[account].hasVoted) { @@ -173,23 +183,17 @@ abstract contract GovernorOverrideDelegateVote is Governor { return totalWeight; } - /// MARK: Exposed override voting functions - function castVoteOverride(uint256 proposalId, uint8 support) public { - _castVoteOverride(proposalId, msg.sender, support, "", ""); - } - - function _castVoteOverride( + function _countVotesOverride( uint256 proposalId, address account, uint8 support, - string memory reason, + uint256 totalWeight, bytes memory params ) private returns (uint256) { ProposalVote storage proposalVote = _proposalVotes[proposalId]; uint256 proposalSnapshot = proposalSnapshot(proposalId); address delegate = _getPastDelegate(account, proposalSnapshot); - uint256 totalWeight = _getPastBalanceOf(account, proposalSnapshot); if (proposalVote.overrideVoteReceipt[account].hasVoted) { revert GovernorAlreadyCastVoteOverride(account); @@ -215,6 +219,11 @@ abstract contract GovernorOverrideDelegateVote is Governor { } else if (delegateVoteReceipt.support == uint8(VoteType.Abstain)) { proposalVote.abstainVotes -= totalWeight; } + + // Write delegate into the params for event + assembly { + mstore(add(params, 0x20), or(mload(add(params, 0x20)), shl(64, delegate))) + } } else { // Only write override weight if they have not voted yet _overrideVoteWeight[delegate][proposalSnapshot] = diff --git a/contracts/mocks/governance/GovernorOverrideMock.sol b/contracts/mocks/governance/GovernorOverrideMock.sol new file mode 100644 index 00000000000..19b325a020c --- /dev/null +++ b/contracts/mocks/governance/GovernorOverrideMock.sol @@ -0,0 +1,29 @@ +// 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"; + +contract GovernorOverrideMock is GovernorSettings, GovernorOverrideDelegateVote { + constructor( + VotesOverridable token, + uint48 initialVotingDelay, + uint32 initialVotingPeriod, + uint256 initialProposalThreshold + ) + Governor("Mock Override Governor") + GovernorOverrideDelegateVote(token) + GovernorSettings(initialVotingDelay, initialVotingPeriod, initialProposalThreshold) + {} + + function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { + return super.proposalThreshold(); + } + + function quorum(uint256) public pure override returns (uint256) { + return 1000e18; + } +} diff --git a/contracts/mocks/token/ERC20VotesOverridableMock.sol b/contracts/mocks/token/ERC20VotesOverridableMock.sol new file mode 100644 index 00000000000..7598293d3d4 --- /dev/null +++ b/contracts/mocks/token/ERC20VotesOverridableMock.sol @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {ERC20Votes, ERC20} from "../../token/ERC20/extensions/ERC20Votes.sol"; +import {VotesOverridable, Votes} from "../../governance/utils/VotesOverridable.sol"; +import {EIP712} from "../../utils/cryptography/EIP712.sol"; + +contract ERC20VotesOverridableMock is ERC20Votes, VotesOverridable { + constructor() ERC20("ERC20VotesOverridableMock", "E20M") EIP712("ERC20VotesOverridableMock", "1.0.0") {} + + function mint(address account, uint256 amount) external { + _mint(account, amount); + } + + function burn(address account, uint256 amount) external { + _burn(account, amount); + } + + 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); + } +} diff --git a/test/governance/extensions/GovernorOverrideDelegateVote.t.sol b/test/governance/extensions/GovernorOverrideDelegateVote.t.sol new file mode 100644 index 00000000000..8b39500bbea --- /dev/null +++ b/test/governance/extensions/GovernorOverrideDelegateVote.t.sol @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {ERC20VotesOverridableMock} from "@openzeppelin/contracts/mocks/token/ERC20VotesOverridableMock.sol"; +import {GovernorOverrideMock} from "@openzeppelin/contracts/mocks/governance/GovernorOverrideMock.sol"; + +contract GovernorOverrideDelegateVoteTest is Test { + ERC20VotesOverridableMock token; + GovernorOverrideMock governor; + + function setUp() public { + token = new ERC20VotesOverridableMock(); + governor = new GovernorOverrideMock(token, 10, 10, 10); + } + + function testOverrideVote(address tokenHolder, address delegate) external { + vm.assume(tokenHolder != address(0)); + vm.assume(delegate != address(0)); + vm.assume(delegate != tokenHolder); + + token.mint(tokenHolder, 1000); + vm.prank(tokenHolder); + token.delegate(delegate); + + vm.roll(block.number + 1); + vm.prank(delegate); + + address[] memory targets = new address[](1); + uint256[] memory values = new uint256[](1); + bytes[] memory calldatas = new bytes[](1); + uint256 proposalId = governor.propose(targets, values, calldatas, "Mock Proposal"); + + vm.roll(block.number + 20); + vm.prank(delegate); + governor.castVote(proposalId, 1); + + (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) = governor.proposalVotes(proposalId); + assertEq(forVotes, 1000); + + vm.prank(tokenHolder); + governor.castVoteWithReasonAndParams( + proposalId, + 0, + "My delegate made a bad decision", + hex"23b70c8d0000000000000000000000000000000000000000" + ); + (againstVotes, forVotes, abstainVotes) = governor.proposalVotes(proposalId); + assertEq(againstVotes, 1000); + assertEq(forVotes, 0); + } +} diff --git a/test/governance/utils/VotesOverridable.t.sol b/test/governance/utils/VotesOverridable.t.sol new file mode 100644 index 00000000000..2f78e76c1a0 --- /dev/null +++ b/test/governance/utils/VotesOverridable.t.sol @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.20; + +import {Test} from "forge-std/Test.sol"; +import {ERC20VotesOverridableMock} from "@openzeppelin/contracts/mocks/token/ERC20VotesOverridableMock.sol"; + +contract ERC20VotesOverrideMockTest is Test { + ERC20VotesOverridableMock token; + + function setUp() public { + token = new ERC20VotesOverridableMock(); + } + + function testGetPastBalanceOfCheckpointBalances(address tokenHolder) external { + vm.assume(tokenHolder != address(0)); + + uint256 timepoint1 = block.number; + vm.roll(timepoint1 + 2); + + uint256 timepoint2 = block.number; + token.mint(tokenHolder, 100); + vm.roll(timepoint2 + 2); + + uint256 timepoint3 = block.number; + token.mint(tokenHolder, 200); + vm.roll(timepoint3 + 2); + + assertEq(token.getPastBalanceOf(tokenHolder, timepoint1), 0); + assertEq(token.getPastBalanceOf(tokenHolder, timepoint2), 100); + assertEq(token.getPastBalanceOf(tokenHolder, timepoint3), 300); + } + + function testGetPastDelegateCheckpointDelegates(address delegatee, address delegate1, address delegate2) external { + uint256 timepoint1 = block.number; + vm.roll(timepoint1 + 2); + + uint256 timepoint2 = block.number; + vm.prank(delegatee); + token.delegate(delegate1); + vm.roll(timepoint2 + 2); + + uint256 timepoint3 = block.number; + vm.prank(delegatee); + token.delegate(delegate2); + vm.roll(timepoint3 + 2); + + assertEq(token.getPastDelegate(delegatee, timepoint1), address(0)); + assertEq(token.getPastDelegate(delegatee, timepoint2), delegate1); + assertEq(token.getPastDelegate(delegatee, timepoint3), delegate2); + } +} From c6a048b23929330c79099b65a2ddbc068d496031 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 19 Aug 2024 15:40:07 -0400 Subject: [PATCH 5/9] fix: override per proposalId instead of timepoint --- .../GovernorOverrideDelegateVote.sol | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index af1be49a5e8..780919a7a72 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -39,7 +39,7 @@ abstract contract GovernorOverrideDelegateVote is Governor { error GovernorAlreadyCastVoteOverride(address account); mapping(uint256 proposalId => ProposalVote) private _proposalVotes; - mapping(address account => mapping(uint256 timepoint => uint256 votes)) private _overrideVoteWeight; + mapping(address account => mapping(uint256 proposalId => uint256 votes)) private _overrideVoteWeight; VotesOverridable private immutable _token; @@ -47,7 +47,6 @@ abstract contract GovernorOverrideDelegateVote is Governor { _token = tokenAddress; } - /// MARK: Votes logic /** * @dev The token that voting power is sourced from. */ @@ -85,18 +84,8 @@ abstract contract GovernorOverrideDelegateVote is Governor { function _getVotes( address account, uint256 timepoint, - bytes memory params + bytes memory /*params*/ ) internal view virtual override returns (uint256) { - // If the params are the selector `0x23b70c8d` (keccak("override")[0:8]) followed by address 0, it is an override vote. - if (keccak256(params) == keccak256(bytes(hex"23b70c8d0000000000000000000000000000000000000000"))) { - // This is an override vote - return _getPastBalanceOf(account, timepoint); - } - uint256 overrideVotes = _overrideVoteWeight[account][timepoint]; - if (overrideVotes != 0) { - // This delegate has been overridden for this vote. Return the reduced voting weight. - return overrideVotes; - } return token().getPastVotes(account, timepoint); } @@ -160,9 +149,11 @@ abstract contract GovernorOverrideDelegateVote is Governor { bytes memory params ) internal virtual override returns (uint256) { if (keccak256(params) == keccak256(hex"23b70c8d0000000000000000000000000000000000000000")) { - return _countVotesOverride(proposalId, account, support, totalWeight, params); + return _countVotesOverride(proposalId, account, support, params); } + totalWeight -= _overrideVoteWeight[account][proposalId]; + ProposalVote storage proposalVote = _proposalVotes[proposalId]; if (proposalVote.voteReceipt[account].hasVoted) { @@ -187,25 +178,27 @@ abstract contract GovernorOverrideDelegateVote is Governor { uint256 proposalId, address account, uint8 support, - uint256 totalWeight, 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); } + uint256 overrideWeight = _getPastBalanceOf(account, proposalSnapshot); + proposalVote.overrideVoteReceipt[account] = VoteReceipt({hasVoted: true, support: support}); if (support == uint8(VoteType.Against)) { - proposalVote.againstVotes += totalWeight; + proposalVote.againstVotes += overrideWeight; } else if (support == uint8(VoteType.For)) { - proposalVote.forVotes += totalWeight; + proposalVote.forVotes += overrideWeight; } else if (support == uint8(VoteType.Abstain)) { - proposalVote.abstainVotes += totalWeight; + proposalVote.abstainVotes += overrideWeight; + } else { + revert GovernorInvalidVoteType(); } // Account for the delegate's vote @@ -213,11 +206,13 @@ abstract contract GovernorOverrideDelegateVote is Governor { if (delegateVoteReceipt.hasVoted) { // If delegate has voted, remove the delegatee's vote weight from their support if (delegateVoteReceipt.support == uint8(VoteType.Against)) { - proposalVote.againstVotes -= totalWeight; + proposalVote.againstVotes -= overrideWeight; } else if (delegateVoteReceipt.support == uint8(VoteType.For)) { - proposalVote.forVotes -= totalWeight; + proposalVote.forVotes -= overrideWeight; } else if (delegateVoteReceipt.support == uint8(VoteType.Abstain)) { - proposalVote.abstainVotes -= totalWeight; + proposalVote.abstainVotes -= overrideWeight; + } else { + revert GovernorInvalidVoteType(); } // Write delegate into the params for event @@ -226,10 +221,8 @@ abstract contract GovernorOverrideDelegateVote is Governor { } } else { // Only write override weight if they have not voted yet - _overrideVoteWeight[delegate][proposalSnapshot] = - _getVotes(delegate, proposalSnapshot, params) - - totalWeight; + _overrideVoteWeight[delegate][proposalId] += overrideWeight; } - return totalWeight; + return overrideWeight; } } From 1e6e530122ce4150e360bcf5201ddc450b7660cb Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Mon, 19 Aug 2024 21:28:09 -0400 Subject: [PATCH 6/9] update comments --- .../GovernorOverrideDelegateVote.sol | 14 +++++++++++++- .../governance/utils/VotesOverridable.sol | 19 ++++++++++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index 780919a7a72..1a4c333a06e 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -89,15 +89,20 @@ abstract contract GovernorOverrideDelegateVote is Governor { return token().getPastVotes(account, timepoint); } + /** + * @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 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 token().getPastBalanceOf(account, timepoint); } - /// MARK: Counting /** * @dev See {IGovernor-COUNTING_MODE}. */ @@ -113,6 +118,13 @@ abstract contract GovernorOverrideDelegateVote is Governor { 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. */ diff --git a/contracts/governance/utils/VotesOverridable.sol b/contracts/governance/utils/VotesOverridable.sol index 61bd1de9996..b6d1bf96efb 100644 --- a/contracts/governance/utils/VotesOverridable.sol +++ b/contracts/governance/utils/VotesOverridable.sol @@ -5,9 +5,15 @@ 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); + mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints; mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints; @@ -38,7 +44,7 @@ abstract contract VotesOverridable is Votes { function getPastDelegate(address account, uint256 timepoint) public view returns (address) { uint48 currentTimepoint = clock(); if (timepoint >= currentTimepoint) { - revert ERC5805FutureLookup(timepoint, currentTimepoint); + revert VotesOverridableFutureLookup(timepoint, currentTimepoint); } return address(uint160(_delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)))); } @@ -61,15 +67,18 @@ abstract contract VotesOverridable is Votes { } /** - * @notice Returns the balance of an account at a specific timepoint. - * @param account The address of the account to check. - * @param timepoint The timepoint to check the balance at (by default block number). + * @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 ERC5805FutureLookup(timepoint, currentTimepoint); + revert VotesOverridableFutureLookup(timepoint, currentTimepoint); } return _balanceOfCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); } From d7550a420b55933e108f59ae0442e6848d0aec56 Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 21 Aug 2024 10:16:45 -0400 Subject: [PATCH 7/9] Use hardhat testing --- contracts/mocks/VotesOverridableMock.sol | 42 ++++ ...l => GovernorOverrideDelegateVoteMock.sol} | 15 +- .../mocks/token/ERC20VotesOverridableMock.sol | 27 +-- .../extensions/ERC20VotesOverridable.sol | 28 --- .../GovernorOverrideDelegateVote.t.sol | 53 ----- .../GovernorOverrideDelegateVote.test.js | 216 ++++++++++++++++++ test/governance/utils/VotesOverridable.t.sol | 52 ----- .../governance/utils/VotesOverridable.test.js | 152 ++++++++++++ 8 files changed, 426 insertions(+), 159 deletions(-) create mode 100644 contracts/mocks/VotesOverridableMock.sol rename contracts/mocks/governance/{GovernorOverrideMock.sol => GovernorOverrideDelegateVoteMock.sol} (58%) delete mode 100644 contracts/token/ERC20/extensions/ERC20VotesOverridable.sol delete mode 100644 test/governance/extensions/GovernorOverrideDelegateVote.t.sol create mode 100644 test/governance/extensions/GovernorOverrideDelegateVote.test.js delete mode 100644 test/governance/utils/VotesOverridable.t.sol create mode 100644 test/governance/utils/VotesOverridable.test.js diff --git a/contracts/mocks/VotesOverridableMock.sol b/contracts/mocks/VotesOverridableMock.sol new file mode 100644 index 00000000000..5aee322980c --- /dev/null +++ b/contracts/mocks/VotesOverridableMock.sol @@ -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"; + } +} diff --git a/contracts/mocks/governance/GovernorOverrideMock.sol b/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol similarity index 58% rename from contracts/mocks/governance/GovernorOverrideMock.sol rename to contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol index 19b325a020c..866926d9526 100644 --- a/contracts/mocks/governance/GovernorOverrideMock.sol +++ b/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol @@ -7,23 +7,12 @@ import {GovernorSettings} from "../../governance/extensions/GovernorSettings.sol import {GovernorVotesQuorumFraction} from "../../governance/extensions/GovernorVotesQuorumFraction.sol"; import {GovernorOverrideDelegateVote, VotesOverridable} from "../../governance/extensions/GovernorOverrideDelegateVote.sol"; -contract GovernorOverrideMock is GovernorSettings, GovernorOverrideDelegateVote { - constructor( - VotesOverridable token, - uint48 initialVotingDelay, - uint32 initialVotingPeriod, - uint256 initialProposalThreshold - ) - Governor("Mock Override Governor") - GovernorOverrideDelegateVote(token) - GovernorSettings(initialVotingDelay, initialVotingPeriod, initialProposalThreshold) - {} - +abstract contract GovernorOverrideDelegateVoteMock is GovernorSettings, GovernorOverrideDelegateVote { function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { return super.proposalThreshold(); } function quorum(uint256) public pure override returns (uint256) { - return 1000e18; + return 10e18; } } diff --git a/contracts/mocks/token/ERC20VotesOverridableMock.sol b/contracts/mocks/token/ERC20VotesOverridableMock.sol index 7598293d3d4..29f4c98e2d6 100644 --- a/contracts/mocks/token/ERC20VotesOverridableMock.sol +++ b/contracts/mocks/token/ERC20VotesOverridableMock.sol @@ -1,21 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.20; -import {ERC20Votes, ERC20} from "../../token/ERC20/extensions/ERC20Votes.sol"; +import {ERC20Votes} from "../../token/ERC20/extensions/ERC20Votes.sol"; import {VotesOverridable, Votes} from "../../governance/utils/VotesOverridable.sol"; -import {EIP712} from "../../utils/cryptography/EIP712.sol"; - -contract ERC20VotesOverridableMock is ERC20Votes, VotesOverridable { - constructor() ERC20("ERC20VotesOverridableMock", "E20M") EIP712("ERC20VotesOverridableMock", "1.0.0") {} - - function mint(address account, uint256 amount) external { - _mint(account, amount); - } - - function burn(address account, uint256 amount) external { - _burn(account, amount); - } +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); } @@ -32,3 +22,14 @@ contract ERC20VotesOverridableMock is ERC20Votes, VotesOverridable { 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"; + } +} diff --git a/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol b/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol deleted file mode 100644 index d3239592e68..00000000000 --- a/contracts/token/ERC20/extensions/ERC20VotesOverridable.sol +++ /dev/null @@ -1,28 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/extensions/ERC20Votes.sol) - -pragma solidity ^0.8.20; - -import {ERC20} from "../ERC20.sol"; -import {Votes} from "../../../governance/utils/Votes.sol"; -import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; -import {ERC20Votes} from "./ERC20Votes.sol"; -import {VotesOverridable} from "../../../governance/utils/VotesOverridable.sol"; - -abstract contract ERC20VotesOverridable is ERC20Votes, VotesOverridable { - function _delegate(address account, address delegatee) internal override(Votes, VotesOverridable) { - super._delegate(account, delegatee); - } - - function _transferVotingUnits( - address from, - address to, - uint256 amount - ) internal virtual override(Votes, VotesOverridable) { - super._transferVotingUnits(from, to, amount); - } - - function delegates(address delegatee) public view override(Votes, VotesOverridable) returns (address) { - return super.delegates(delegatee); - } -} diff --git a/test/governance/extensions/GovernorOverrideDelegateVote.t.sol b/test/governance/extensions/GovernorOverrideDelegateVote.t.sol deleted file mode 100644 index 8b39500bbea..00000000000 --- a/test/governance/extensions/GovernorOverrideDelegateVote.t.sol +++ /dev/null @@ -1,53 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Test} from "forge-std/Test.sol"; -import {ERC20VotesOverridableMock} from "@openzeppelin/contracts/mocks/token/ERC20VotesOverridableMock.sol"; -import {GovernorOverrideMock} from "@openzeppelin/contracts/mocks/governance/GovernorOverrideMock.sol"; - -contract GovernorOverrideDelegateVoteTest is Test { - ERC20VotesOverridableMock token; - GovernorOverrideMock governor; - - function setUp() public { - token = new ERC20VotesOverridableMock(); - governor = new GovernorOverrideMock(token, 10, 10, 10); - } - - function testOverrideVote(address tokenHolder, address delegate) external { - vm.assume(tokenHolder != address(0)); - vm.assume(delegate != address(0)); - vm.assume(delegate != tokenHolder); - - token.mint(tokenHolder, 1000); - vm.prank(tokenHolder); - token.delegate(delegate); - - vm.roll(block.number + 1); - vm.prank(delegate); - - address[] memory targets = new address[](1); - uint256[] memory values = new uint256[](1); - bytes[] memory calldatas = new bytes[](1); - uint256 proposalId = governor.propose(targets, values, calldatas, "Mock Proposal"); - - vm.roll(block.number + 20); - vm.prank(delegate); - governor.castVote(proposalId, 1); - - (uint256 againstVotes, uint256 forVotes, uint256 abstainVotes) = governor.proposalVotes(proposalId); - assertEq(forVotes, 1000); - - vm.prank(tokenHolder); - governor.castVoteWithReasonAndParams( - proposalId, - 0, - "My delegate made a bad decision", - hex"23b70c8d0000000000000000000000000000000000000000" - ); - (againstVotes, forVotes, abstainVotes) = governor.proposalVotes(proposalId); - assertEq(againstVotes, 1000); - assertEq(forVotes, 0); - } -} diff --git a/test/governance/extensions/GovernorOverrideDelegateVote.test.js b/test/governance/extensions/GovernorOverrideDelegateVote.test.js new file mode 100644 index 00000000000..dfe419472c0 --- /dev/null +++ b/test/governance/extensions/GovernorOverrideDelegateVote.test.js @@ -0,0 +1,216 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture, mine } = require('@nomicfoundation/hardhat-network-helpers'); + +const { GovernorHelper } = require('../../helpers/governance'); +const { VoteType } = require('../../helpers/enums'); + +const TOKENS = [ + { Token: '$ERC20VotesOverridableMock', mode: 'blocknumber' }, + { Token: '$ERC20VotesOverridableTimestampMock', mode: 'timestamp' }, +]; + +const name = 'Override Governor'; +const version = '1'; +const tokenName = 'MockToken'; +const tokenSymbol = 'MTKN'; +const tokenSupply = ethers.parseEther('100'); +const votingDelay = 4n; +const votingPeriod = 16n; +const value = ethers.parseEther('1'); + +describe.only('GovernorOverrideDelegateVote', function () { + for (const { Token, mode } of TOKENS) { + const fixture = async () => { + const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); + const receiver = await ethers.deployContract('CallReceiverMock'); + + const token = await ethers.deployContract(Token, [tokenName, tokenSymbol, tokenName, version]); + const mock = await ethers.deployContract('$GovernorOverrideDelegateVoteMock', [ + name, // name + votingDelay, // initialVotingDelay + votingPeriod, // initialVotingPeriod + 0n, // initialProposalThreshold + token, // tokenAddress + ]); + + await owner.sendTransaction({ to: mock, value }); + await token.$_mint(owner, tokenSupply); + + const helper = new GovernorHelper(mock, mode); + await helper.connect(owner).delegate({ token, to: voter1, value: ethers.parseEther('10') }); + await helper.connect(owner).delegate({ token, to: voter2, value: ethers.parseEther('7') }); + await helper.connect(owner).delegate({ token, to: voter3, value: ethers.parseEther('5') }); + await helper.connect(owner).delegate({ token, to: voter4, value: ethers.parseEther('2') }); + + return { owner, proposer, voter1, voter2, voter3, voter4, other, receiver, token, mock, helper }; + }; + + describe(`using ${Token}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + + // default proposal + this.proposal = this.helper.setProposal( + [ + { + target: this.receiver.target, + value, + data: this.receiver.interface.encodeFunctionData('mockFunction'), + }, + ], + '', + ); + }); + + it('deployment check', async function () { + expect(await this.mock.name()).to.equal(name); + expect(await this.mock.token()).to.equal(this.token); + expect(await this.mock.votingDelay()).to.equal(votingDelay); + expect(await this.mock.votingPeriod()).to.equal(votingPeriod); + expect(await this.mock.COUNTING_MODE()).to.equal('support=bravo,override&quorum=for,abstain¶ms=override'); + }); + + it('nominal is unaffected', async function () { + await this.helper.connect(this.proposer).propose(); + await this.helper.waitForSnapshot(); + await this.helper.connect(this.voter1).vote({ support: VoteType.For, reason: 'This is nice' }); + await this.helper.connect(this.voter2).vote({ support: VoteType.For }); + await this.helper.connect(this.voter3).vote({ support: VoteType.Against }); + await this.helper.connect(this.voter4).vote({ support: VoteType.Abstain }); + await this.helper.waitForDeadline(); + await this.helper.execute(); + + expect(await this.mock.hasVoted(this.proposal.id, this.owner)).to.be.false; + expect(await this.mock.hasVoted(this.proposal.id, this.voter1)).to.be.true; + expect(await this.mock.hasVoted(this.proposal.id, this.voter2)).to.be.true; + expect(await ethers.provider.getBalance(this.mock)).to.equal(0n); + expect(await ethers.provider.getBalance(this.receiver)).to.equal(value); + }); + + describe('override vote', async function () { + beforeEach(async function () { + await this.token.connect(this.voter1).delegate(this.voter2); + await this.token.connect(this.voter4).delegate(this.voter2); + await mine(); + + await this.helper.connect(this.proposer).propose(); + await this.helper.waitForSnapshot(); + }); + it('override delegate after delegate vote', async function () { + await expect(this.helper.connect(this.voter2).vote({ support: VoteType.For })) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter2, this.helper.id, VoteType.For, ethers.parseEther('19'), ''); + expect(await this.mock.proposalVotes(this.helper.id)).to.deep.eq([0, ethers.parseEther('19'), 0]); + + await expect( + this.helper + .connect(this.voter1) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }), + ) + .to.emit(this.mock, 'VoteCastWithParams') + .withArgs( + this.voter1, + this.helper.id, + VoteType.Against, + ethers.parseEther('10'), + '', + ethers.solidityPacked(['bytes4', 'address'], ['0x23b70c8d', this.voter2.address]), + ); + + expect(await this.mock.proposalVotes(this.helper.id)).to.deep.eq([ + ethers.parseEther('10'), + ethers.parseEther('9'), + 0, + ]); + }); + + it('override delegate before delegate vote', async function () { + await expect( + this.helper + .connect(this.voter1) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }), + ) + .to.emit(this.mock, 'VoteCastWithParams') + .withArgs( + this.voter1, + this.helper.id, + VoteType.Against, + ethers.parseEther('10'), + '', + ethers.solidityPacked(['bytes4', 'address'], ['0x23b70c8d', ethers.ZeroAddress]), + ); + expect(await this.mock.proposalVotes(this.helper.id)).to.deep.eq([ethers.parseEther('10'), 0, 0]); + expect(await this.mock.hasVotedOverride(this.helper.id, this.voter1)).to.be.true; + expect(await this.mock.hasVoted(this.helper.id, this.voter1)).to.be.false; + + await expect(this.helper.connect(this.voter2).vote({ support: VoteType.For })) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter2, this.helper.id, VoteType.For, ethers.parseEther('9'), ''); + expect(await this.mock.proposalVotes(this.helper.id)).to.deep.eq([ + ethers.parseEther('10'), + ethers.parseEther('9'), + 0, + ]); + expect(await this.mock.hasVotedOverride(this.helper.id, this.voter2)).to.be.false; + expect(await this.mock.hasVoted(this.helper.id, this.voter2)).to.be.true; + }); + + it('override delegate before and after delegate vote', async function () { + await expect( + this.helper + .connect(this.voter1) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }), + ) + .to.emit(this.mock, 'VoteCastWithParams') + .withArgs( + this.voter1, + this.helper.id, + VoteType.Against, + ethers.parseEther('10'), + '', + ethers.solidityPacked(['bytes4', 'address'], ['0x23b70c8d', ethers.ZeroAddress]), + ); + + await expect(this.helper.connect(this.voter2).vote({ support: VoteType.For })) + .to.emit(this.mock, 'VoteCast') + .withArgs(this.voter2, this.helper.id, VoteType.For, ethers.parseEther('9'), ''); + + await expect( + this.helper + .connect(this.voter4) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }), + ) + .to.emit(this.mock, 'VoteCastWithParams') + .withArgs( + this.voter4, + this.helper.id, + VoteType.Against, + ethers.parseEther('2'), + '', + ethers.solidityPacked(['bytes4', 'address'], ['0x23b70c8d', this.voter2.address]), + ); + + expect(await this.mock.proposalVotes(this.helper.id)).to.deep.eq([ + ethers.parseEther('12'), + ethers.parseEther('7'), + 0, + ]); + }); + + it('can not override vote twice', async function () { + await this.helper + .connect(this.voter1) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }); + await expect( + this.helper + .connect(this.voter1) + .vote({ support: VoteType.Against, params: '0x23b70c8d0000000000000000000000000000000000000000' }), + ) + .to.be.revertedWithCustomError(this.mock, 'GovernorAlreadyCastVoteOverride') + .withArgs(this.voter1.address); + }); + }); + }); + } +}); diff --git a/test/governance/utils/VotesOverridable.t.sol b/test/governance/utils/VotesOverridable.t.sol deleted file mode 100644 index 2f78e76c1a0..00000000000 --- a/test/governance/utils/VotesOverridable.t.sol +++ /dev/null @@ -1,52 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {Test} from "forge-std/Test.sol"; -import {ERC20VotesOverridableMock} from "@openzeppelin/contracts/mocks/token/ERC20VotesOverridableMock.sol"; - -contract ERC20VotesOverrideMockTest is Test { - ERC20VotesOverridableMock token; - - function setUp() public { - token = new ERC20VotesOverridableMock(); - } - - function testGetPastBalanceOfCheckpointBalances(address tokenHolder) external { - vm.assume(tokenHolder != address(0)); - - uint256 timepoint1 = block.number; - vm.roll(timepoint1 + 2); - - uint256 timepoint2 = block.number; - token.mint(tokenHolder, 100); - vm.roll(timepoint2 + 2); - - uint256 timepoint3 = block.number; - token.mint(tokenHolder, 200); - vm.roll(timepoint3 + 2); - - assertEq(token.getPastBalanceOf(tokenHolder, timepoint1), 0); - assertEq(token.getPastBalanceOf(tokenHolder, timepoint2), 100); - assertEq(token.getPastBalanceOf(tokenHolder, timepoint3), 300); - } - - function testGetPastDelegateCheckpointDelegates(address delegatee, address delegate1, address delegate2) external { - uint256 timepoint1 = block.number; - vm.roll(timepoint1 + 2); - - uint256 timepoint2 = block.number; - vm.prank(delegatee); - token.delegate(delegate1); - vm.roll(timepoint2 + 2); - - uint256 timepoint3 = block.number; - vm.prank(delegatee); - token.delegate(delegate2); - vm.roll(timepoint3 + 2); - - assertEq(token.getPastDelegate(delegatee, timepoint1), address(0)); - assertEq(token.getPastDelegate(delegatee, timepoint2), delegate1); - assertEq(token.getPastDelegate(delegatee, timepoint3), delegate2); - } -} diff --git a/test/governance/utils/VotesOverridable.test.js b/test/governance/utils/VotesOverridable.test.js new file mode 100644 index 00000000000..59a655c4924 --- /dev/null +++ b/test/governance/utils/VotesOverridable.test.js @@ -0,0 +1,152 @@ +const { ethers } = require('hardhat'); +const { expect } = require('chai'); +const { loadFixture, mine } = require('@nomicfoundation/hardhat-network-helpers'); + +const { sum } = require('../../helpers/math'); +const { zip } = require('../../helpers/iterate'); +const time = require('../../helpers/time'); + +const { shouldBehaveLikeVotes } = require('./Votes.behavior'); + +const MODES = { + blocknumber: '$VotesOverridableMock', + timestamp: '$VotesOverridableTimestampMock', +}; + +const AMOUNTS = [ethers.parseEther('10000000'), 10n, 20n]; + +describe('VotesOverridable', function () { + for (const [mode, artifact] of Object.entries(MODES)) { + const fixture = async () => { + const accounts = await ethers.getSigners(); + + const amounts = Object.fromEntries( + zip( + accounts.slice(0, AMOUNTS.length).map(({ address }) => address), + AMOUNTS, + ), + ); + + const name = 'Override Votes'; + const version = '1'; + const votes = await ethers.deployContract(artifact, [name, version]); + + return { accounts, amounts, votes, name, version }; + }; + + describe(`vote with ${mode}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + shouldBehaveLikeVotes(AMOUNTS, { mode, fungible: true }); + + it('starts with zero votes', async function () { + expect(await this.votes.getTotalSupply()).to.equal(0n); + }); + + describe('performs voting operations', function () { + beforeEach(async function () { + this.txs = []; + for (const [account, amount] of Object.entries(this.amounts)) { + this.txs.push(await this.votes.$_mint(account, amount)); + } + }); + + it('reverts if block number >= current block', async function () { + const lastTxTimepoint = await time.clockFromReceipt[mode](this.txs.at(-1)); + const clock = await this.votes.clock(); + await expect(this.votes.getPastTotalSupply(lastTxTimepoint)) + .to.be.revertedWithCustomError(this.votes, 'ERC5805FutureLookup') + .withArgs(lastTxTimepoint, clock); + }); + + it('delegates', async function () { + expect(await this.votes.getVotes(this.accounts[0])).to.equal(0n); + expect(await this.votes.getVotes(this.accounts[1])).to.equal(0n); + expect(await this.votes.delegates(this.accounts[0])).to.equal(ethers.ZeroAddress); + expect(await this.votes.delegates(this.accounts[1])).to.equal(ethers.ZeroAddress); + + await this.votes.delegate(this.accounts[0], ethers.Typed.address(this.accounts[0])); + + expect(await this.votes.getVotes(this.accounts[0])).to.equal(this.amounts[this.accounts[0].address]); + expect(await this.votes.getVotes(this.accounts[1])).to.equal(0n); + expect(await this.votes.delegates(this.accounts[0])).to.equal(this.accounts[0]); + expect(await this.votes.delegates(this.accounts[1])).to.equal(ethers.ZeroAddress); + + await this.votes.delegate(this.accounts[1], ethers.Typed.address(this.accounts[0])); + + expect(await this.votes.getVotes(this.accounts[0])).to.equal( + this.amounts[this.accounts[0].address] + this.amounts[this.accounts[1].address], + ); + expect(await this.votes.getVotes(this.accounts[1])).to.equal(0n); + expect(await this.votes.delegates(this.accounts[0])).to.equal(this.accounts[0]); + expect(await this.votes.delegates(this.accounts[1])).to.equal(this.accounts[0]); + }); + + it('cross delegates', async function () { + await this.votes.delegate(this.accounts[0], ethers.Typed.address(this.accounts[1])); + await this.votes.delegate(this.accounts[1], ethers.Typed.address(this.accounts[0])); + + expect(await this.votes.getVotes(this.accounts[0])).to.equal(this.amounts[this.accounts[1].address]); + expect(await this.votes.getVotes(this.accounts[1])).to.equal(this.amounts[this.accounts[0].address]); + }); + + it('returns total amount of votes', async function () { + const totalSupply = sum(...Object.values(this.amounts)); + expect(await this.votes.getTotalSupply()).to.equal(totalSupply); + }); + }); + }); + + describe(`checkpoint delegates with ${mode}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + it('checkpoint delegates', async function () { + const tx = await this.votes.delegate(this.accounts[0], ethers.Typed.address(this.accounts[1])); + const timepoint = await time.clockFromReceipt[mode](tx); + await mine(2); + + expect(await this.votes.getPastDelegate(this.accounts[0], timepoint - 1n)).to.equal(ethers.ZeroAddress); + expect(await this.votes.getPastDelegate(this.accounts[0], timepoint)).to.equal(this.accounts[1].address); + expect(await this.votes.getPastDelegate(this.accounts[0], timepoint + 1n)).to.equal(this.accounts[1].address); + }); + + it('reverts if current timepoint <= timepoint', async function () { + const tx = await this.votes.delegate(this.accounts[0], ethers.Typed.address(this.accounts[1])); + const timepoint = await time.clockFromReceipt[mode](tx); + + await expect(this.votes.getPastDelegate(this.accounts[0], timepoint + 1n)) + .to.be.revertedWithCustomError(this.votes, 'VotesOverridableFutureLookup') + .withArgs(timepoint + 1n, timepoint); + }); + }); + + describe(`checkpoint balances with ${mode}`, function () { + beforeEach(async function () { + Object.assign(this, await loadFixture(fixture)); + }); + + it('checkpoint balances', async function () { + const tx = await this.votes.$_mint(this.accounts[0].address, 100n); + const timepoint = await time.clockFromReceipt[mode](tx); + await mine(2); + + expect(await this.votes.getPastBalanceOf(this.accounts[0].address, timepoint - 1n)).to.equal(0n); + expect(await this.votes.getPastBalanceOf(this.accounts[0].address, timepoint)).to.equal(100n); + expect(await this.votes.getPastBalanceOf(this.accounts[0].address, timepoint + 1n)).to.equal(100n); + }); + + it('reverts if current timepoint <= timepoint', async function () { + const tx = await this.votes.$_mint(this.accounts[0].address, 100n); + const timepoint = await time.clockFromReceipt[mode](tx); + + await expect(this.votes.getPastBalanceOf(this.accounts[0], timepoint + 1n)) + .to.be.revertedWithCustomError(this.votes, 'VotesOverridableFutureLookup') + .withArgs(timepoint + 1n, timepoint); + }); + }); + } +}); From 5cdcb440cfa15e8e520b4027ccd1f6f9acc8dc0f Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Wed, 21 Aug 2024 18:55:55 -0400 Subject: [PATCH 8/9] Inherit `GovernorVotes` to avoid code duplication --- .../GovernorOverrideDelegateVote.sol | 56 ++----------------- .../GovernorOverrideDelegateVoteMock.sol | 11 ++-- .../GovernorOverrideDelegateVote.test.js | 3 +- 3 files changed, 13 insertions(+), 57 deletions(-) diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index 1a4c333a06e..7eb8d8d9bf5 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; -import {Governor} from "../Governor.sol"; +import {GovernorVotes} from "./GovernorVotes.sol"; import {IVotes} from "../utils/IVotes.sol"; import {IERC5805} from "../../interfaces/IERC5805.sol"; import {SafeCast} from "../../utils/math/SafeCast.sol"; @@ -13,7 +13,7 @@ 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 Governor { +abstract contract GovernorOverrideDelegateVote is GovernorVotes { /** * @dev Supported vote types. Matches Governor Bravo ordering. */ @@ -41,66 +41,20 @@ abstract contract GovernorOverrideDelegateVote is Governor { mapping(uint256 proposalId => ProposalVote) private _proposalVotes; mapping(address account => mapping(uint256 proposalId => uint256 votes)) private _overrideVoteWeight; - VotesOverridable private immutable _token; - - constructor(VotesOverridable tokenAddress) { - _token = tokenAddress; - } - - /** - * @dev The token that voting power is sourced from. - */ - function token() public view virtual returns (VotesOverridable) { - return _token; - } - - /** - * @dev Clock (as specified in ERC-6372) is set to match the token's clock. Fallback to block numbers if the token - * does not implement ERC-6372. - */ - function clock() public view virtual override returns (uint48) { - try token().clock() returns (uint48 timepoint) { - return timepoint; - } catch { - return Time.blockNumber(); - } - } - - /** - * @dev Machine-readable description of the clock as specified in ERC-6372. - */ - // solhint-disable-next-line func-name-mixedcase - function CLOCK_MODE() public view virtual override returns (string memory) { - try token().CLOCK_MODE() returns (string memory clockmode) { - return clockmode; - } catch { - return "mode=blocknumber&from=default"; - } - } - - /** - * Read the voting weight from the token's built in snapshot mechanism (see {Governor-_getVotes}). - */ - function _getVotes( - address account, - uint256 timepoint, - bytes memory /*params*/ - ) internal view virtual override returns (uint256) { - return token().getPastVotes(account, timepoint); - } + constructor(VotesOverridable tokenAddress) GovernorVotes(tokenAddress) {} /** * @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 token().getPastDelegate(account, timepoint); + 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 token().getPastBalanceOf(account, timepoint); + return VotesOverridable(address(token())).getPastBalanceOf(account, timepoint); } /** diff --git a/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol b/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol index 866926d9526..1bc762e0981 100644 --- a/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol +++ b/contracts/mocks/governance/GovernorOverrideDelegateVoteMock.sol @@ -6,13 +6,14 @@ 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, GovernorOverrideDelegateVote { +abstract contract GovernorOverrideDelegateVoteMock is + GovernorSettings, + GovernorVotesQuorumFraction, + GovernorOverrideDelegateVote +{ function proposalThreshold() public view override(Governor, GovernorSettings) returns (uint256) { return super.proposalThreshold(); } - - function quorum(uint256) public pure override returns (uint256) { - return 10e18; - } } diff --git a/test/governance/extensions/GovernorOverrideDelegateVote.test.js b/test/governance/extensions/GovernorOverrideDelegateVote.test.js index dfe419472c0..c441770495f 100644 --- a/test/governance/extensions/GovernorOverrideDelegateVote.test.js +++ b/test/governance/extensions/GovernorOverrideDelegateVote.test.js @@ -19,7 +19,7 @@ const votingDelay = 4n; const votingPeriod = 16n; const value = ethers.parseEther('1'); -describe.only('GovernorOverrideDelegateVote', function () { +describe('GovernorOverrideDelegateVote', function () { for (const { Token, mode } of TOKENS) { const fixture = async () => { const [owner, proposer, voter1, voter2, voter3, voter4, other] = await ethers.getSigners(); @@ -31,6 +31,7 @@ describe.only('GovernorOverrideDelegateVote', function () { votingDelay, // initialVotingDelay votingPeriod, // initialVotingPeriod 0n, // initialProposalThreshold + 10n, // quorumNumeratorValue token, // tokenAddress ]); From 45cb3302af886f7e99db823c4f2ad9ba651946ce Mon Sep 17 00:00:00 2001 From: Arr00 <13561405+arr00@users.noreply.github.com> Date: Thu, 22 Aug 2024 17:33:55 -0400 Subject: [PATCH 9/9] suggestions --- .../GovernorOverrideDelegateVote.sol | 101 ++++++++---------- .../governance/utils/VotesOverridable.sol | 16 +-- .../mocks/token/ERC20VotesOverridableMock.sol | 4 - 3 files changed, 47 insertions(+), 74 deletions(-) diff --git a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol index 7eb8d8d9bf5..029b0e82b7c 100644 --- a/contracts/governance/extensions/GovernorOverrideDelegateVote.sol +++ b/contracts/governance/extensions/GovernorOverrideDelegateVote.sol @@ -24,8 +24,9 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { } struct VoteReceipt { - bool hasVoted; uint8 support; + bool hasOverriden; + uint208 overrideWeight; } struct ProposalVote { @@ -33,30 +34,14 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { uint256 forVotes; uint256 abstainVotes; mapping(address voter => VoteReceipt) voteReceipt; - mapping(address voter => VoteReceipt) overrideVoteReceipt; } error GovernorAlreadyCastVoteOverride(address account); mapping(uint256 proposalId => ProposalVote) private _proposalVotes; - mapping(address account => mapping(uint256 proposalId => uint256 votes)) private _overrideVoteWeight; constructor(VotesOverridable tokenAddress) GovernorVotes(tokenAddress) {} - /** - * @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); - } - /** * @dev See {IGovernor-COUNTING_MODE}. */ @@ -69,14 +54,14 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { * @dev See {IGovernor-hasVoted}. */ function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { - return _proposalVotes[proposalId].voteReceipt[account].hasVoted; + 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].overrideVoteReceipt[account].hasVoted; + return _proposalVotes[proposalId].voteReceipt[account].hasOverriden; } /** @@ -118,24 +103,17 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { return _countVotesOverride(proposalId, account, support, params); } - totalWeight -= _overrideVoteWeight[account][proposalId]; - ProposalVote storage proposalVote = _proposalVotes[proposalId]; - if (proposalVote.voteReceipt[account].hasVoted) { + totalWeight -= proposalVote.voteReceipt[account].overrideWeight; + + if (proposalVote.voteReceipt[account].support != 0) { revert GovernorAlreadyCastVote(account); } - proposalVote.voteReceipt[account] = VoteReceipt({hasVoted: true, support: support}); + // Support tracks support and if a user voted. Store support + 1. + proposalVote.voteReceipt[account].support = support + 1; - 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(); - } + _tallyVote(proposalVote, support, _add, totalWeight); return totalWeight; } @@ -148,38 +126,22 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { ) private returns (uint256) { ProposalVote storage proposalVote = _proposalVotes[proposalId]; uint256 proposalSnapshot = proposalSnapshot(proposalId); - address delegate = _getPastDelegate(account, proposalSnapshot); + address delegate = VotesOverridable(address(token())).getPastDelegate(account, proposalSnapshot); - if (proposalVote.overrideVoteReceipt[account].hasVoted) { + if (proposalVote.voteReceipt[account].hasOverriden) { revert GovernorAlreadyCastVoteOverride(account); } + proposalVote.voteReceipt[account].hasOverriden = true; - 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(); - } + 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.hasVoted) { + if (delegateVoteReceipt.support != 0) { + uint8 correctedSupport = delegateVoteReceipt.support - 1; // 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(); - } + _tallyVote(proposalVote, correctedSupport, _subtract, overrideWeight); // Write delegate into the params for event assembly { @@ -187,8 +149,33 @@ abstract contract GovernorOverrideDelegateVote is GovernorVotes { } } else { // Only write override weight if they have not voted yet - _overrideVoteWeight[delegate][proposalId] += overrideWeight; + 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; + } } diff --git a/contracts/governance/utils/VotesOverridable.sol b/contracts/governance/utils/VotesOverridable.sol index b6d1bf96efb..aeaac5b07a3 100644 --- a/contracts/governance/utils/VotesOverridable.sol +++ b/contracts/governance/utils/VotesOverridable.sol @@ -18,19 +18,9 @@ abstract contract VotesOverridable is Votes { mapping(address account => Checkpoints.Trace208) private _balanceOfCheckpoints; function _delegate(address account, address delegatee) internal virtual override { - address oldDelegate = delegates(account); + super._delegate(account, delegatee); _delegateCheckpoints[account].push(clock(), uint160(delegatee)); - - emit DelegateChanged(account, oldDelegate, delegatee); - _moveDelegateVotes(oldDelegate, delegatee, _getVotingUnits(account)); - } - - /** - * @inheritdoc Votes - */ - function delegates(address delegatee) public view virtual override returns (address) { - return address(uint160(_delegateCheckpoints[delegatee].latest())); } /** @@ -57,11 +47,11 @@ abstract contract VotesOverridable is Votes { if (from != to) { if (from != address(0)) { Checkpoints.Trace208 storage store = _balanceOfCheckpoints[from]; - store.push(clock(), uint208(store.latest() - amount)); + store.push(clock(), uint208(_getVotingUnits(from))); } if (to != address(0)) { Checkpoints.Trace208 storage store = _balanceOfCheckpoints[to]; - store.push(clock(), uint208(store.latest() + amount)); + store.push(clock(), uint208(_getVotingUnits(to))); } } } diff --git a/contracts/mocks/token/ERC20VotesOverridableMock.sol b/contracts/mocks/token/ERC20VotesOverridableMock.sol index 29f4c98e2d6..52f4dd0ea38 100644 --- a/contracts/mocks/token/ERC20VotesOverridableMock.sol +++ b/contracts/mocks/token/ERC20VotesOverridableMock.sol @@ -17,10 +17,6 @@ abstract contract ERC20VotesOverridableMock is ERC20Votes, VotesOverridable { ) 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 {