From eb3d234afd8c5570a5df7ae8a200aa45a0d1808c Mon Sep 17 00:00:00 2001 From: keating Date: Tue, 9 Jan 2024 15:47:57 -0500 Subject: [PATCH 01/13] WIP --- .gitmodules | 2 +- lib/openzeppelin-contracts | 2 +- src/FlexVotingClient.sol | 8 +++---- src/GovernorCountingFractional.sol | 19 +++++++++++----- test/FractionalGovernor.sol | 8 ++++--- test/GovToken.sol | 2 +- test/GovernorCountingFractional.t.sol | 32 +++++++++++++++++---------- 7 files changed, 46 insertions(+), 27 deletions(-) diff --git a/.gitmodules b/.gitmodules index dd8ab1e..ef87f89 100644 --- a/.gitmodules +++ b/.gitmodules @@ -6,4 +6,4 @@ url = https://github.com/foundry-rs/forge-std [submodule "lib/openzeppelin-contracts"] path = lib/openzeppelin-contracts - url = https://github.com/openzeppelin/openzeppelin-contracts + url = https://github.com/OpenZeppelin/openzeppelin-contracts diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts index fd81a96..01ef448 160000 --- a/lib/openzeppelin-contracts +++ b/lib/openzeppelin-contracts @@ -1 +1 @@ -Subproject commit fd81a96f01cc42ef1c9a5399364968d0e07e9e90 +Subproject commit 01ef448981be9d20ca85f2faf6ebdf591ce409f3 diff --git a/src/FlexVotingClient.sol b/src/FlexVotingClient.sol index 3bd99bb..e97464e 100644 --- a/src/FlexVotingClient.sol +++ b/src/FlexVotingClient.sol @@ -2,7 +2,7 @@ pragma solidity >=0.8.10; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; -import {Checkpoints} from "@openzeppelin/contracts/utils/Checkpoints.sol"; +import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol"; import {IFractionalGovernor} from "./interfaces/IFractionalGovernor.sol"; import {IVotingToken} from "./interfaces/IVotingToken.sol"; @@ -48,7 +48,7 @@ import {IVotingToken} from "./interfaces/IVotingToken.sol"; /// of the rest. abstract contract FlexVotingClient { using SafeCast for uint256; - using Checkpoints for Checkpoints.History; + using Checkpoints for Checkpoints.Trace224; /// @notice The voting options corresponding to those used in the Governor. enum VoteType { @@ -76,12 +76,12 @@ abstract contract FlexVotingClient { /// @dev Mapping from address to the checkpoint history of raw balances /// of that address. - mapping(address => Checkpoints.History) private balanceCheckpoints; + mapping(address => Checkpoints.Trace224) private balanceCheckpoints; /// @dev History of the sum total of raw balances in the system. May or may /// not be equivalent to this contract's balance of `GOVERNOR`s token at a /// given time. - Checkpoints.History internal totalBalanceCheckpoints; + Checkpoints.Trace224 internal totalBalanceCheckpoints; /// @param _governor The address of the flex-voting-compatible governance contract. constructor(address _governor) { diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index b118ef7..877a267 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.0; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; -import {GovernorCompatibilityBravo} from "@openzeppelin/contracts/governance/compatibility/GovernorCompatibilityBravo.sol"; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; /** @@ -20,6 +19,15 @@ import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; */ abstract contract GovernorCountingFractional is Governor { + /** + * @dev Supported vote types. Matches Governor Bravo ordering. + */ + enum VoteType { + Against, + For, + Abstain + } + struct ProposalVote { uint128 againstVotes; uint128 forVotes; @@ -166,11 +174,11 @@ abstract contract GovernorCountingFractional is Governor { _proposalVotersWeightCast[proposalId][account] = totalWeight; - if (support == uint8(GovernorCompatibilityBravo.VoteType.Against)) { + if (support == uint8(VoteType.Against)) { _proposalVotes[proposalId].againstVotes += totalWeight; - } else if (support == uint8(GovernorCompatibilityBravo.VoteType.For)) { + } else if (support == uint8(VoteType.For)) { _proposalVotes[proposalId].forVotes += totalWeight; - } else if (support == uint8(GovernorCompatibilityBravo.VoteType.Abstain)) { + } else if (support == uint8(VoteType.Abstain)) { _proposalVotes[proposalId].abstainVotes += totalWeight; } else { revert("GovernorCountingFractional: invalid support value, must be included in VoteType enum"); @@ -265,6 +273,7 @@ abstract contract GovernorCountingFractional is Governor { * * See {fractionalVoteNonce} and {_castVote} for more information. */ + // TODO signature is differnt. Need to update signature function castVoteWithReasonAndParamsBySig( uint256 proposalId, uint8 support, @@ -273,7 +282,7 @@ abstract contract GovernorCountingFractional is Governor { uint8 v, bytes32 r, bytes32 s - ) public virtual override returns (uint256) { + ) public virtual returns (uint256) { // Signature-based fractional voting requires `params` be two full words // in length: // 16 bytes for againstVotes. diff --git a/test/FractionalGovernor.sol b/test/FractionalGovernor.sol index a53dfe3..59545a3 100644 --- a/test/FractionalGovernor.sol +++ b/test/FractionalGovernor.sol @@ -1,8 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.8.10; -import "../src/GovernorCountingFractional.sol"; -import "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; +import {GovernorCountingFractional} from "../src/GovernorCountingFractional.sol"; +import {GovernorVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; +import {IVotes} from "@openzeppelin/contracts/governance/utils/IVotes.sol"; +import {Governor} from "@openzeppelin/contracts/governance//Governor.sol"; contract FractionalGovernor is GovernorVotes, GovernorCountingFractional { constructor(string memory name_, IVotes token_) Governor(name_) GovernorVotes(token_) {} @@ -15,7 +17,7 @@ contract FractionalGovernor is GovernorVotes, GovernorCountingFractional { uint8 v, bytes32 r, bytes32 s - ) public virtual override(GovernorCountingFractional, Governor) returns (uint256) { + ) public virtual override(GovernorCountingFractional) returns (uint256) { return GovernorCountingFractional.castVoteWithReasonAndParamsBySig( proposalId, support, reason, params, v, r, s ); diff --git a/test/GovToken.sol b/test/GovToken.sol index 5cb3f61..92b6453 100644 --- a/test/GovToken.sol +++ b/test/GovToken.sol @@ -2,7 +2,7 @@ pragma solidity >=0.8.10; import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol"; -import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/draft-ERC20Permit.sol"; +import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; contract GovToken is ERC20Votes { diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index cd3f029..05d7474 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -4,13 +4,13 @@ pragma solidity >=0.8.10; import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; import {FractionalPool, IVotingToken, IFractionalGovernor} from "../src/FractionalPool.sol"; -import {GovernorCompatibilityBravo} from - "@openzeppelin/contracts/governance/compatibility/GovernorCompatibilityBravo.sol"; import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol"; import {GovToken} from "./GovToken.sol"; -import {FractionalGovernor, IVotes, IGovernor} from "./FractionalGovernor.sol"; +import {FractionalGovernor} from "./FractionalGovernor.sol"; import {ProposalReceiverMock} from "./ProposalReceiverMock.sol"; +import {IVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; +import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; contract GovernorCountingFractionalTest is Test { using FixedPointMathLib for uint256; @@ -40,6 +40,14 @@ contract GovernorCountingFractionalTest is Test { string description ); + enum VoteType { + Against, + For, + Abstain + } + + + // We use a min of 1e4 to avoid flooring votes to 0. uint256 constant MIN_VOTE_WEIGHT = 1e4; // This is the vote storage slot size on the Fractional Governor contract. @@ -196,7 +204,7 @@ contract GovernorCountingFractionalTest is Test { } function _randomSupportType(uint256 salt) public view returns (uint8) { - return uint8(bound(salt, 0, uint8(GovernorCompatibilityBravo.VoteType.Abstain))); + return uint8(bound(salt, 0, uint8(VoteType.Abstain))); } function _randomVoteSplit(FractionalVoteSplit memory _voteSplit) @@ -253,13 +261,13 @@ contract GovernorCountingFractionalTest is Test { againstVotes += uint128(voter.weight.mulWadDown(voter.voteSplit.percentAgainst)); abstainVotes += uint128(voter.weight.mulWadDown(voter.voteSplit.percentAbstain)); } else { - if (voter.support == uint8(GovernorCompatibilityBravo.VoteType.For)) { + if (voter.support == uint8(VoteType.For)) { forVotes += voter.weight; } - if (voter.support == uint8(GovernorCompatibilityBravo.VoteType.Against)) { + if (voter.support == uint8(VoteType.Against)) { againstVotes += voter.weight; } - if (voter.support == uint8(GovernorCompatibilityBravo.VoteType.Abstain)) { + if (voter.support == uint8(VoteType.Abstain)) { abstainVotes += voter.weight; } } @@ -417,13 +425,13 @@ contract GovernorCountingFractionalTest is Test { (uint256 _actualAgainstVotes, uint256 _actualForVotes, uint256 _actualAbstainVotes) = governor.proposalVotes(_proposalId); - if (_voter.support == uint8(GovernorCompatibilityBravo.VoteType.For)) { + if (_voter.support == uint8(VoteType.For)) { assertEq(_voter.weight, _actualForVotes); } - if (_voter.support == uint8(GovernorCompatibilityBravo.VoteType.Against)) { + if (_voter.support == uint8(VoteType.Against)) { assertEq(_voter.weight, _actualAgainstVotes); } - if (_voter.support == uint8(GovernorCompatibilityBravo.VoteType.Abstain)) { + if (_voter.support == uint8(VoteType.Abstain)) { assertEq(_voter.weight, _actualAbstainVotes); } @@ -641,7 +649,7 @@ contract GovernorCountingFractionalTest is Test { vm.expectRevert("GovernorCountingFractional: no weight"); governor.castVoteWithReasonAndParams( _proposalId, - uint8(GovernorCompatibilityBravo.VoteType.For), + uint8(VoteType.For), "I hope no one catches me doing this!", new bytes(0) // No data, this is a nominal vote. ); @@ -651,7 +659,7 @@ contract GovernorCountingFractionalTest is Test { vm.expectRevert("GovernorCountingFractional: no weight"); governor.castVoteWithReasonAndParams( _proposalId, - uint8(GovernorCompatibilityBravo.VoteType.For), + uint8(VoteType.For), "I'm so bad", abi.encodePacked(type(uint128).max, type(uint128).max, type(uint128).max) ); From 415b8c8efddc5cf8cf0e2032cccebac312666c90 Mon Sep 17 00:00:00 2001 From: keating Date: Wed, 10 Jan 2024 14:03:20 -0500 Subject: [PATCH 02/13] Can compile --- src/FlexVotingClient.sol | 10 +++++---- test/FractionalGovernor.sol | 2 +- test/GovToken.sol | 17 +++++++++++++- test/GovernorCountingFractional.t.sol | 32 ++++++++------------------- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/FlexVotingClient.sol b/src/FlexVotingClient.sol index e97464e..a260af5 100644 --- a/src/FlexVotingClient.sol +++ b/src/FlexVotingClient.sol @@ -92,7 +92,7 @@ abstract contract FlexVotingClient { /// token that `_user` has claim to in this system. It may or may not be /// equivalent to the withdrawable balance of `GOVERNOR`s token for `user`, /// e.g. if the internal representation of balance has been scaled down. - function _rawBalanceOf(address _user) internal view virtual returns (uint256); + function _rawBalanceOf(address _user) internal view virtual returns (uint224); /// @dev Used as the `reason` param when submitting a vote to `GOVERNOR`. function _castVoteReasonString() internal virtual returns (string memory) { @@ -194,19 +194,21 @@ abstract contract FlexVotingClient { /// @dev Checkpoints the _user's current raw balance. function _checkpointRawBalanceOf(address _user) internal { - balanceCheckpoints[_user].push(_rawBalanceOf(_user)); + balanceCheckpoints[_user].push(SafeCast.toUint32(block.number), _rawBalanceOf(_user)); } /// @notice Returns the `_user`'s raw balance at `_blockNumber`. /// @param _user The account that's historical raw balance will be looked up. /// @param _blockNumber The block at which to lookup the _user's raw balance. function getPastRawBalance(address _user, uint256 _blockNumber) public view returns (uint256) { - return balanceCheckpoints[_user].getAtProbablyRecentBlock(_blockNumber); + uint32 key = SafeCast.toUint32(_blockNumber); + return balanceCheckpoints[_user].upperLookup(key); } /// @notice Returns the sum total of raw balances of all users at `_blockNumber`. /// @param _blockNumber The block at which to lookup the total balance. function getPastTotalBalance(uint256 _blockNumber) public view returns (uint256) { - return totalBalanceCheckpoints.getAtProbablyRecentBlock(_blockNumber); + uint32 key = SafeCast.toUint32(_blockNumber); + return totalBalanceCheckpoints.upperLookup(key); } } diff --git a/test/FractionalGovernor.sol b/test/FractionalGovernor.sol index 59545a3..c7f107e 100644 --- a/test/FractionalGovernor.sol +++ b/test/FractionalGovernor.sol @@ -4,7 +4,7 @@ pragma solidity >=0.8.10; import {GovernorCountingFractional} from "../src/GovernorCountingFractional.sol"; import {GovernorVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; import {IVotes} from "@openzeppelin/contracts/governance/utils/IVotes.sol"; -import {Governor} from "@openzeppelin/contracts/governance//Governor.sol"; +import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; contract FractionalGovernor is GovernorVotes, GovernorCountingFractional { constructor(string memory name_, IVotes token_) Governor(name_) GovernorVotes(token_) {} diff --git a/test/GovToken.sol b/test/GovToken.sol index 92b6453..2fb6cc5 100644 --- a/test/GovToken.sol +++ b/test/GovToken.sol @@ -4,8 +4,9 @@ pragma solidity >=0.8.10; import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol"; import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import {Nonces} from "@openzeppelin/contracts/utils/Nonces.sol"; -contract GovToken is ERC20Votes { +contract GovToken is ERC20, ERC20Permit, ERC20Votes { constructor() ERC20("Governance Token", "GOV") ERC20Permit("GOV") {} function exposed_mint(address to, uint256 amount) public { @@ -15,4 +16,18 @@ contract GovToken is ERC20Votes { function exposed_maxSupply() external view returns (uint256) { return uint256(_maxSupply()); } + + function _update(address from, address to, uint256 amount) internal override(ERC20, ERC20Votes) { + ERC20Votes._update(from, to, amount); + } + + function nonces(address owner) + public + view + virtual + override(ERC20Permit, Nonces) + returns (uint256) + { + return Nonces.nonces(owner); + } } diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index 05d7474..acfd7e6 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -41,13 +41,11 @@ contract GovernorCountingFractionalTest is Test { ); enum VoteType { - Against, - For, - Abstain + Against, + For, + Abstain } - - // We use a min of 1e4 to avoid flooring votes to 0. uint256 constant MIN_VOTE_WEIGHT = 1e4; // This is the vote storage slot size on the Fractional Governor contract. @@ -261,15 +259,9 @@ contract GovernorCountingFractionalTest is Test { againstVotes += uint128(voter.weight.mulWadDown(voter.voteSplit.percentAgainst)); abstainVotes += uint128(voter.weight.mulWadDown(voter.voteSplit.percentAbstain)); } else { - if (voter.support == uint8(VoteType.For)) { - forVotes += voter.weight; - } - if (voter.support == uint8(VoteType.Against)) { - againstVotes += voter.weight; - } - if (voter.support == uint8(VoteType.Abstain)) { - abstainVotes += voter.weight; - } + if (voter.support == uint8(VoteType.For)) forVotes += voter.weight; + if (voter.support == uint8(VoteType.Against)) againstVotes += voter.weight; + if (voter.support == uint8(VoteType.Abstain)) abstainVotes += voter.weight; } } } @@ -425,15 +417,9 @@ contract GovernorCountingFractionalTest is Test { (uint256 _actualAgainstVotes, uint256 _actualForVotes, uint256 _actualAbstainVotes) = governor.proposalVotes(_proposalId); - if (_voter.support == uint8(VoteType.For)) { - assertEq(_voter.weight, _actualForVotes); - } - if (_voter.support == uint8(VoteType.Against)) { - assertEq(_voter.weight, _actualAgainstVotes); - } - if (_voter.support == uint8(VoteType.Abstain)) { - assertEq(_voter.weight, _actualAbstainVotes); - } + if (_voter.support == uint8(VoteType.For)) assertEq(_voter.weight, _actualForVotes); + if (_voter.support == uint8(VoteType.Against)) assertEq(_voter.weight, _actualAgainstVotes); + if (_voter.support == uint8(VoteType.Abstain)) assertEq(_voter.weight, _actualAbstainVotes); // The signature cannot be re-used. vm.expectRevert("GovernorCountingFractional: all weight cast"); From 20435ac2752e310808f74be596dcbd782ded051c Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 15:50:33 -0500 Subject: [PATCH 03/13] Fix tests and remove signature override --- src/GovernorCountingFractional.sol | 93 ----------- test/FractionalGovernor.sol | 14 -- test/FractionalPool.t.sol | 11 +- test/GovernorCountingFractional.t.sol | 222 +------------------------- 4 files changed, 16 insertions(+), 324 deletions(-) diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index 877a267..bd7dd79 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -258,97 +258,4 @@ abstract contract GovernorCountingFractional is Governor { } } - /** - * @notice Cast a vote with a reason and additional encoded parameters using - * the user's cryptographic signature. - * - * Emits a {VoteCast} or {VoteCastWithParams} event depending on the length - * of params. - * - * @dev If casting a fractional vote via `params`, the voter's current nonce - * must be appended to the `params` as the last 16 bytes and included in the - * signature. I.e., the params used when constructing the signature would be: - * - * abi.encodePacked(againstVotes, forVotes, abstainVotes, nonce) - * - * See {fractionalVoteNonce} and {_castVote} for more information. - */ - // TODO signature is differnt. Need to update signature - function castVoteWithReasonAndParamsBySig( - uint256 proposalId, - uint8 support, - string calldata reason, - bytes memory params, - uint8 v, - bytes32 r, - bytes32 s - ) public virtual returns (uint256) { - // Signature-based fractional voting requires `params` be two full words - // in length: - // 16 bytes for againstVotes. - // 16 bytes for forVotes. - // 16 bytes for abstainVotes. - // 16 bytes for the signature nonce. - // Signature-based nominal voting requires `params` be 0 bytes. - require( - params.length == 64 || params.length == 0, - "GovernorCountingFractional: invalid params for signature-based vote" - ); - - address voter = ECDSA.recover( - _hashTypedDataV4( - keccak256( - abi.encode( - EXTENDED_BALLOT_TYPEHASH, - proposalId, - support, - keccak256(bytes(reason)), - keccak256(params) - ) - ) - ), - v, - r, - s - ); - - // If params are zero-length all of the voter's weight will be cast so - // we don't have to worry about checking/incrementing a nonce. - if (params.length == 64) { - // Get the nonce out of the params. It is the last half-word. - uint128 nonce; - assembly { - nonce := and( - // Perform bitwise AND operation on the data in the second word of - // `params` with a mask of 128 zeros followed by 128 ones, i.e. take - // the last 128 bits of `params`. - _MASK_HALF_WORD_RIGHT, - // Load the data from memory at the returned address. - mload( - // Skip the first 64 bytes (0x40): - // 32 bytes encoding the length of the bytes array. - // 32 bytes for the first word in the params - // Return the memory address for the last word in params. - add(params, 0x40) - ) - ) - } - - require( - fractionalVoteNonce[voter] == nonce, - "GovernorCountingFractional: signature has already been used" - ); - - fractionalVoteNonce[voter]++; - - // Trim params in place to keep only the first 48 bytes (which are - // the voting params) and save gas. - assembly { - mstore(params, 0x30) - } - } - - return _castVote(proposalId, voter, support, reason, params); - } - } diff --git a/test/FractionalGovernor.sol b/test/FractionalGovernor.sol index c7f107e..a811b39 100644 --- a/test/FractionalGovernor.sol +++ b/test/FractionalGovernor.sol @@ -9,20 +9,6 @@ import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; contract FractionalGovernor is GovernorVotes, GovernorCountingFractional { constructor(string memory name_, IVotes token_) Governor(name_) GovernorVotes(token_) {} - function castVoteWithReasonAndParamsBySig( - uint256 proposalId, - uint8 support, - string calldata reason, - bytes memory params, - uint8 v, - bytes32 r, - bytes32 s - ) public virtual override(GovernorCountingFractional) returns (uint256) { - return GovernorCountingFractional.castVoteWithReasonAndParamsBySig( - proposalId, support, reason, params, v, r, s - ); - } - function quorum(uint256) public pure override returns (uint256) { return 10 ether; } diff --git a/test/FractionalPool.t.sol b/test/FractionalPool.t.sol index 8ce0afa..2338ef8 100644 --- a/test/FractionalPool.t.sol +++ b/test/FractionalPool.t.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.10; import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; +import {console2} from "forge-std/console2.sol"; +import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; import {FractionalPool, IVotingToken, IFractionalGovernor} from "../src/FractionalPool.sol"; import "./GovToken.sol"; import "./FractionalGovernor.sol"; @@ -47,6 +49,9 @@ contract FractionalPoolTest is Test { function _mintGovAndApprovePool(address _holder, uint256 _amount) public { vm.assume(_holder != address(0)); + console2.logUint(token.totalSupply()); + console2.logUint(token.exposed_maxSupply()); + console2.logUint(_amount); token.exposed_mint(_holder, _amount); vm.prank(_holder); token.approve(address(pool), type(uint256).max); @@ -115,7 +120,7 @@ contract Deployment is FractionalPoolTest { contract Deposit is FractionalPoolTest { function test_UserCanDepositGovTokens(address _holder, uint256 _amount) public { - _amount = bound(_amount, 0, type(uint224).max); + _amount = bound(_amount, 0, type(uint208).max); vm.assume(_holder != address(pool)); uint256 initialBalance = token.balanceOf(_holder); @@ -245,9 +250,11 @@ contract Vote is FractionalPoolTest { // Jump ahead so that we're outside of the proposal's voting period. vm.roll(governor.proposalDeadline(_proposalId) + 1); + IGovernor.ProposalState status = IGovernor.ProposalState(uint32(governor.state(_proposalId))); // We should not be able to castVote at this point. - vm.expectRevert(bytes("Governor: vote not currently active")); + // vm.expectRevert(bytes("Governor: vote not currently active")); + vm.expectRevert(abi.encodeWithSelector(IGovernor.GovernorUnexpectedProposalState.selector, _proposalId, status, bytes32(1 << uint8(IGovernor.ProposalState.Active)))); pool.castVote(_proposalId); } diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index acfd7e6..8406cf0 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -11,6 +11,7 @@ import {FractionalGovernor} from "./FractionalGovernor.sol"; import {ProposalReceiverMock} from "./ProposalReceiverMock.sol"; import {IVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; +import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; contract GovernorCountingFractionalTest is Test { using FixedPointMathLib for uint256; @@ -157,12 +158,12 @@ contract GovernorCountingFractionalTest is Test { function _executeProposal() internal { Proposal memory _rawProposalInfo = _getSimpleProposal(); + // TODO: This had to be re-arranged vm.expectEmit(true, true, true, true); - emit ProposalExecuted(_rawProposalInfo.id); + emit MockFunctionCalled(); - // Ensure that the other contract is invoked. vm.expectEmit(true, true, true, true); - emit MockFunctionCalled(); + emit ProposalExecuted(_rawProposalInfo.id); governor.execute( _rawProposalInfo.targets, @@ -332,7 +333,7 @@ contract GovernorCountingFractionalTest is Test { assertEq(uint8(status), uint8(IGovernor.ProposalState.Defeated)); Proposal memory _rawProposalInfo = _getSimpleProposal(); - vm.expectRevert(bytes("Governor: proposal not successful")); + vm.expectRevert(abi.encodeWithSelector(IGovernor.GovernorUnexpectedProposalState.selector, _proposalId, status, bytes32(1 << uint8(IGovernor.ProposalState.Succeeded)) | bytes32(1 << uint8(IGovernor.ProposalState.Queued)))); governor.execute( _rawProposalInfo.targets, _rawProposalInfo.values, @@ -372,62 +373,6 @@ contract GovernorCountingFractionalTest is Test { _fractionalGovernorHappyPathTest(voters); } - function testFuzz_NominalVotingWithFractionalizedParamsAndSignature( - uint256 _weight, - uint128 _nonce - ) public { - Voter memory _voter; - uint256 _privateKey; - (_voter.addr, _privateKey) = makeAddrAndKey("voter"); - vm.assume(_voter.addr != address(this)); - - // Use a random nonce. Bound by max - 1 because we need to sign one message. - _nonce = uint128(bound(_nonce, 0, type(uint128).max - 1)); - governor.exposed_setFractionalVoteNonce(_voter.addr, _nonce); - uint128 _initNonce = governor.fractionalVoteNonce(_voter.addr); - - _voter.weight = bound(_weight, MIN_VOTE_WEIGHT, MAX_VOTE_WEIGHT); - _voter.support = _randomSupportType(_weight); - - _mintAndDelegateToVoter(_voter); - uint256 _proposalId = _createAndSubmitProposal(); - - bytes32 _voteMessage = keccak256( - abi.encode( - keccak256("ExtendedBallot(uint256 proposalId,uint8 support,string reason,bytes params)"), - _proposalId, - _voter.support, - keccak256(bytes("I have my reasons")), - keccak256(new bytes(0)) - ) - ); - - bytes32 _voteMessageHash = - keccak256(abi.encodePacked("\x19\x01", EIP712_DOMAIN_SEPARATOR, _voteMessage)); - - (uint8 _v, bytes32 _r, bytes32 _s) = vm.sign(_privateKey, _voteMessageHash); - - governor.castVoteWithReasonAndParamsBySig( - _proposalId, _voter.support, "I have my reasons", new bytes(0), _v, _r, _s - ); - - assertEq( - _initNonce, governor.fractionalVoteNonce(_voter.addr), "nonce should not have incremented" - ); - - (uint256 _actualAgainstVotes, uint256 _actualForVotes, uint256 _actualAbstainVotes) = - governor.proposalVotes(_proposalId); - if (_voter.support == uint8(VoteType.For)) assertEq(_voter.weight, _actualForVotes); - if (_voter.support == uint8(VoteType.Against)) assertEq(_voter.weight, _actualAgainstVotes); - if (_voter.support == uint8(VoteType.Abstain)) assertEq(_voter.weight, _actualAbstainVotes); - - // The signature cannot be re-used. - vm.expectRevert("GovernorCountingFractional: all weight cast"); - governor.castVoteWithReasonAndParamsBySig( - _proposalId, _voter.support, "I have my reasons", new bytes(0), _v, _r, _s - ); - } - struct CastVoteWithReasonAndParamsBySigTestVars { uint128 forVotes; uint128 againstVotes; @@ -448,159 +393,6 @@ contract GovernorCountingFractionalTest is Test { uint256 remainingWeight; } - function testFuzz_VotingWithFractionalizedParamsAndSignature( - uint256 _weight, - uint256 _partialVoteWeight, - FractionalVoteSplit memory _voteSplit, - uint128 _nonce - ) public { - CastVoteWithReasonAndParamsBySigTestVars memory _vars; - (_vars.voter.addr, _vars.privateKey) = makeAddrAndKey("voter with replay protection"); - vm.assume(_vars.voter.addr != address(this)); - - // Use a random nonce. Bound by (max - 2) because we need to sign 2 messages. - _nonce = uint128(bound(_nonce, 0, type(uint128).max - 2)); - governor.exposed_setFractionalVoteNonce(_vars.voter.addr, _nonce); - - _vars.voter.weight = bound(_weight, MIN_VOTE_WEIGHT, MAX_VOTE_WEIGHT); - _vars.voter.support = _randomSupportType(_weight); - _vars.voter.voteSplit = _randomVoteSplit(_voteSplit); - - // We want to be able to cast two distinct signature-based votes. - _partialVoteWeight = bound( - _partialVoteWeight, - _vars.voter.weight.mulWadDown(0.1e18), // 10% - _vars.voter.weight.mulWadDown(0.9e18) // 90% - ); - - _vars.forVotes = uint128(_partialVoteWeight.mulWadDown(_voteSplit.percentFor)); - _vars.againstVotes = uint128(_partialVoteWeight.mulWadDown(_voteSplit.percentAgainst)); - _vars.abstainVotes = uint128(_partialVoteWeight.mulWadDown(_voteSplit.percentAbstain)); - _vars.fractionalizedVotes = abi.encodePacked( - _vars.againstVotes, - _vars.forVotes, - _vars.abstainVotes, - governor.fractionalVoteNonce(_vars.voter.addr) - ); - - _mintAndDelegateToVoter(_vars.voter); - _vars.proposalId = _createAndSubmitProposal(); - - _vars.voteMessage = keccak256( - abi.encode( - governor.EXTENDED_BALLOT_TYPEHASH(), - _vars.proposalId, - _vars.voter.support, - keccak256(bytes("I have my reasons")), - keccak256(_vars.fractionalizedVotes) - ) - ); - - _vars.voteMessageHash = - keccak256(abi.encodePacked("\x19\x01", EIP712_DOMAIN_SEPARATOR, _vars.voteMessage)); - - (_vars.v, _vars.r, _vars.s) = vm.sign(_vars.privateKey, _vars.voteMessageHash); - _vars.lastNonce = governor.fractionalVoteNonce(_vars.voter.addr); - - // First vote. - governor.castVoteWithReasonAndParamsBySig( - _vars.proposalId, - _vars.voter.support, - "I have my reasons", - _vars.fractionalizedVotes, - _vars.v, - _vars.r, - _vars.s - ); - - // Nonce should have incremented. - assertEq( - _vars.lastNonce + 1, - governor.fractionalVoteNonce(_vars.voter.addr), - "nonce should have incremented" - ); - _vars.lastNonce = governor.fractionalVoteNonce(_vars.voter.addr); - - (_vars.actualAgainstVotes, _vars.actualForVotes, _vars.actualAbstainVotes) = - governor.proposalVotes(_vars.proposalId); - - assertEq(_vars.forVotes, _vars.actualForVotes); - assertEq(_vars.againstVotes, _vars.actualAgainstVotes); - assertEq(_vars.abstainVotes, _vars.actualAbstainVotes); - - // Try to re-use the signature, which should revert. - vm.expectRevert("GovernorCountingFractional: signature has already been used"); - governor.castVoteWithReasonAndParamsBySig( - _vars.proposalId, - _vars.voter.support, - "I have my reasons", - _vars.fractionalizedVotes, - _vars.v, - _vars.r, - _vars.s - ); - - // Nonce shouldn't have changed since the first successful vote. - assertEq( - _vars.lastNonce, - governor.fractionalVoteNonce(_vars.voter.addr), - "nonce should not have incremented" - ); - - // Sign a new message. - _vars.remainingWeight = _vars.voter.weight - _partialVoteWeight; - _vars.forVotes = uint128(_vars.remainingWeight.mulWadDown(_voteSplit.percentFor)); - _vars.againstVotes = uint128(_vars.remainingWeight.mulWadDown(_voteSplit.percentAgainst)); - _vars.abstainVotes = uint128(_vars.remainingWeight.mulWadDown(_voteSplit.percentAbstain)); - _vars.fractionalizedVotes = abi.encodePacked( - _vars.againstVotes, - _vars.forVotes, - _vars.abstainVotes, - governor.fractionalVoteNonce(_vars.voter.addr) - ); - _vars.voteMessage = keccak256( - abi.encode( - governor.EXTENDED_BALLOT_TYPEHASH(), - _vars.proposalId, - _vars.voter.support, - keccak256(bytes("I have my reasons")), - keccak256(_vars.fractionalizedVotes) - ) - ); - _vars.voteMessageHash = - keccak256(abi.encodePacked("\x19\x01", EIP712_DOMAIN_SEPARATOR, _vars.voteMessage)); - (_vars.v, _vars.r, _vars.s) = vm.sign(_vars.privateKey, _vars.voteMessageHash); - - // Submit second signed vote. It should succeed. - governor.castVoteWithReasonAndParamsBySig( - _vars.proposalId, - _vars.voter.support, - "I have my reasons", - _vars.fractionalizedVotes, - _vars.v, - _vars.r, - _vars.s - ); - - // Nonce should have incremented again. - assertEq( - _vars.lastNonce + 1, - governor.fractionalVoteNonce(_vars.voter.addr), - "nonce should have incremented" - ); - - (_vars.actualAgainstVotes, _vars.actualForVotes, _vars.actualAbstainVotes) = - governor.proposalVotes(_vars.proposalId); - - // Actual weights can differ by up to 1 because of rounding during division. - assertApproxEqAbs(_vars.voter.weight.mulWadDown(_voteSplit.percentFor), _vars.actualForVotes, 1); - assertApproxEqAbs( - _vars.voter.weight.mulWadDown(_voteSplit.percentAgainst), _vars.actualAgainstVotes, 1 - ); - assertApproxEqAbs( - _vars.voter.weight.mulWadDown(_voteSplit.percentAbstain), _vars.actualAbstainVotes, 1 - ); - } function testFuzz_VoteSplitsCanBeMaxedOut(uint256[4] memory _weights, uint8 _maxSplit) public { Voter[4] memory _voters = _setupNominalVoters(_weights); @@ -725,7 +517,7 @@ contract GovernorCountingFractionalTest is Test { bytes memory emptyVotingParams; vm.prank(voter.addr); - vm.expectRevert("SafeCast: value doesn't fit in 128 bits"); + vm.expectRevert(abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight)); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Yay", emptyVotingParams); } @@ -753,7 +545,7 @@ contract GovernorCountingFractionalTest is Test { bytes memory fractionalizedVotes = abi.encodePacked(_againstVotes, _forVotes, _abstainVotes); vm.prank(voter.addr); - vm.expectRevert("SafeCast: value doesn't fit in 128 bits"); + vm.expectRevert(abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight)); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Weeee", fractionalizedVotes); } From 01337bb4d2c0b81406023322e1337b2e85afc108 Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 15:53:26 -0500 Subject: [PATCH 04/13] Format code --- test/FractionalPool.t.sol | 15 +++++++++++---- test/GovernorCountingFractional.t.sol | 21 ++++++++++++++++----- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/test/FractionalPool.t.sol b/test/FractionalPool.t.sol index 2338ef8..753c009 100644 --- a/test/FractionalPool.t.sol +++ b/test/FractionalPool.t.sol @@ -49,9 +49,9 @@ contract FractionalPoolTest is Test { function _mintGovAndApprovePool(address _holder, uint256 _amount) public { vm.assume(_holder != address(0)); - console2.logUint(token.totalSupply()); - console2.logUint(token.exposed_maxSupply()); - console2.logUint(_amount); + console2.logUint(token.totalSupply()); + console2.logUint(token.exposed_maxSupply()); + console2.logUint(_amount); token.exposed_mint(_holder, _amount); vm.prank(_holder); token.approve(address(pool), type(uint256).max); @@ -254,7 +254,14 @@ contract Vote is FractionalPoolTest { // We should not be able to castVote at this point. // vm.expectRevert(bytes("Governor: vote not currently active")); - vm.expectRevert(abi.encodeWithSelector(IGovernor.GovernorUnexpectedProposalState.selector, _proposalId, status, bytes32(1 << uint8(IGovernor.ProposalState.Active)))); + vm.expectRevert( + abi.encodeWithSelector( + IGovernor.GovernorUnexpectedProposalState.selector, + _proposalId, + status, + bytes32(1 << uint8(IGovernor.ProposalState.Active)) + ) + ); pool.castVote(_proposalId); } diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index 8406cf0..201677f 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -158,7 +158,7 @@ contract GovernorCountingFractionalTest is Test { function _executeProposal() internal { Proposal memory _rawProposalInfo = _getSimpleProposal(); - // TODO: This had to be re-arranged + // TODO: This had to be re-arranged vm.expectEmit(true, true, true, true); emit MockFunctionCalled(); @@ -333,7 +333,15 @@ contract GovernorCountingFractionalTest is Test { assertEq(uint8(status), uint8(IGovernor.ProposalState.Defeated)); Proposal memory _rawProposalInfo = _getSimpleProposal(); - vm.expectRevert(abi.encodeWithSelector(IGovernor.GovernorUnexpectedProposalState.selector, _proposalId, status, bytes32(1 << uint8(IGovernor.ProposalState.Succeeded)) | bytes32(1 << uint8(IGovernor.ProposalState.Queued)))); + vm.expectRevert( + abi.encodeWithSelector( + IGovernor.GovernorUnexpectedProposalState.selector, + _proposalId, + status, + bytes32(1 << uint8(IGovernor.ProposalState.Succeeded)) + | bytes32(1 << uint8(IGovernor.ProposalState.Queued)) + ) + ); governor.execute( _rawProposalInfo.targets, _rawProposalInfo.values, @@ -393,7 +401,6 @@ contract GovernorCountingFractionalTest is Test { uint256 remainingWeight; } - function testFuzz_VoteSplitsCanBeMaxedOut(uint256[4] memory _weights, uint8 _maxSplit) public { Voter[4] memory _voters = _setupNominalVoters(_weights); @@ -517,7 +524,9 @@ contract GovernorCountingFractionalTest is Test { bytes memory emptyVotingParams; vm.prank(voter.addr); - vm.expectRevert(abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight)); + vm.expectRevert( + abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight) + ); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Yay", emptyVotingParams); } @@ -545,7 +554,9 @@ contract GovernorCountingFractionalTest is Test { bytes memory fractionalizedVotes = abi.encodePacked(_againstVotes, _forVotes, _abstainVotes); vm.prank(voter.addr); - vm.expectRevert(abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight)); + vm.expectRevert( + abi.encodeWithSelector(SafeCast.SafeCastOverflowedUintDowncast.selector, 128, voter.weight) + ); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Weeee", fractionalizedVotes); } From f7df59d11da45d136c6c3e8003bf3d3c656989a8 Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 16:02:36 -0500 Subject: [PATCH 05/13] Cleanup pr --- test/FractionalPool.t.sol | 5 ----- test/GovernorCountingFractional.t.sol | 1 - 2 files changed, 6 deletions(-) diff --git a/test/FractionalPool.t.sol b/test/FractionalPool.t.sol index 753c009..9c6039a 100644 --- a/test/FractionalPool.t.sol +++ b/test/FractionalPool.t.sol @@ -3,7 +3,6 @@ pragma solidity ^0.8.10; import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; -import {console2} from "forge-std/console2.sol"; import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; import {FractionalPool, IVotingToken, IFractionalGovernor} from "../src/FractionalPool.sol"; import "./GovToken.sol"; @@ -49,9 +48,6 @@ contract FractionalPoolTest is Test { function _mintGovAndApprovePool(address _holder, uint256 _amount) public { vm.assume(_holder != address(0)); - console2.logUint(token.totalSupply()); - console2.logUint(token.exposed_maxSupply()); - console2.logUint(_amount); token.exposed_mint(_holder, _amount); vm.prank(_holder); token.approve(address(pool), type(uint256).max); @@ -253,7 +249,6 @@ contract Vote is FractionalPoolTest { IGovernor.ProposalState status = IGovernor.ProposalState(uint32(governor.state(_proposalId))); // We should not be able to castVote at this point. - // vm.expectRevert(bytes("Governor: vote not currently active")); vm.expectRevert( abi.encodeWithSelector( IGovernor.GovernorUnexpectedProposalState.selector, diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index 201677f..2f4739a 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -158,7 +158,6 @@ contract GovernorCountingFractionalTest is Test { function _executeProposal() internal { Proposal memory _rawProposalInfo = _getSimpleProposal(); - // TODO: This had to be re-arranged vm.expectEmit(true, true, true, true); emit MockFunctionCalled(); From bf77d88d884ee92942ce3b7019f1d810d6a14344 Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 16:14:06 -0500 Subject: [PATCH 06/13] Remove additional nonce code --- src/GovernorCountingFractional.sol | 8 -------- test/GovernorCountingFractional.t.sol | 20 -------------------- 2 files changed, 28 deletions(-) diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index bd7dd79..5aa883f 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -46,14 +46,6 @@ abstract contract GovernorCountingFractional is Governor { */ mapping(uint256 => mapping(address => uint128)) private _proposalVotersWeightCast; - /** - * @dev Mapping from voter address to signature-based vote nonce. The - * voter's nonce increments each time a signature-based vote is cast with - * fractional voting params and must be included in the `params` as the last - * 16 bytes when signing for a fractional vote. - */ - mapping(address => uint128) public fractionalVoteNonce; - /** * @dev See {IGovernor-COUNTING_MODE}. */ diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index 2f4739a..ff66f78 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -380,26 +380,6 @@ contract GovernorCountingFractionalTest is Test { _fractionalGovernorHappyPathTest(voters); } - struct CastVoteWithReasonAndParamsBySigTestVars { - uint128 forVotes; - uint128 againstVotes; - uint128 abstainVotes; - uint128 lastNonce; - bytes fractionalizedVotes; - uint256 privateKey; - uint256 proposalId; - Voter voter; - bytes32 voteMessage; - bytes32 voteMessageHash; - uint8 v; - bytes32 r; - bytes32 s; - uint256 actualAgainstVotes; - uint256 actualForVotes; - uint256 actualAbstainVotes; - uint256 remainingWeight; - } - function testFuzz_VoteSplitsCanBeMaxedOut(uint256[4] memory _weights, uint8 _maxSplit) public { Voter[4] memory _voters = _setupNominalVoters(_weights); From 9306f84d51eed9caffa019f5b1df67867d431bb6 Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 16:16:55 -0500 Subject: [PATCH 07/13] Fix compilation --- test/FractionalGovernor.sol | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/FractionalGovernor.sol b/test/FractionalGovernor.sol index a811b39..3d93e44 100644 --- a/test/FractionalGovernor.sol +++ b/test/FractionalGovernor.sol @@ -25,10 +25,6 @@ contract FractionalGovernor is GovernorVotes, GovernorCountingFractional { return _quorumReached(_proposalId); } - function exposed_setFractionalVoteNonce(address _voter, uint128 _newNonce) public { - fractionalVoteNonce[_voter] = _newNonce; - } - function cancel( address[] memory targets, uint256[] memory values, From e4a7c4687b6956bf93d6c7716a8e9fff427915ae Mon Sep 17 00:00:00 2001 From: keating Date: Fri, 12 Jan 2024 16:28:17 -0500 Subject: [PATCH 08/13] Lower test coverage --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be79645..09d8be1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -106,7 +106,7 @@ jobs: uses: zgosalvez/github-actions-report-lcov@v2 with: coverage-files: ./lcov.info - minimum-coverage: 70 # Set coverage threshold. + minimum-coverage: 67 # Set coverage threshold. slither-analyze: runs-on: ubuntu-latest From c9aecc47ef7bb95d799381d364a05aa9ea693022 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Sat, 27 Apr 2024 16:08:12 -0400 Subject: [PATCH 09/13] Update OpenZeppelin Contracts to v5.0.2 --- lib/openzeppelin-contracts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts index 01ef448..dbb6104 160000 --- a/lib/openzeppelin-contracts +++ b/lib/openzeppelin-contracts @@ -1 +1 @@ -Subproject commit 01ef448981be9d20ca85f2faf6ebdf591ce409f3 +Subproject commit dbb6104ce834628e473d2173bbc9d47f81a9eec3 From 8f0185f651a71c46362311edea4aac98e5d42464 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Sat, 27 Apr 2024 16:37:02 -0400 Subject: [PATCH 10/13] Convert GovernorCountingFractional to use error types --- src/GovernorCountingFractional.sol | 29 +++++++++++----- test/FractionalPool.t.sol | 6 +++- test/GovernorCountingFractional.t.sol | 50 ++++++++++++++++++++++----- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index 5aa883f..e101fcd 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -7,6 +7,7 @@ import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; /** + * @author [ScopeLift](https://scopelift.co) * @notice Extension of {Governor} for 3 option fractional vote counting. When * voting, a delegate may split their vote weight between Against/For/Abstain. * This is most useful when the delegate is itself a contract, implementing its @@ -19,6 +20,10 @@ import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; */ abstract contract GovernorCountingFractional is Governor { + error GovernorCountingFractional__VoteWeightExceeded(); + error GovernorCountingFractional__InvalidVoteData(); + error GovernorCountingFractional_NoVoteWeight(); + /** * @dev Supported vote types. Matches Governor Bravo ordering. */ @@ -132,9 +137,12 @@ abstract contract GovernorCountingFractional is Governor { uint256 totalWeight, bytes memory voteData ) internal virtual override { - require(totalWeight > 0, "GovernorCountingFractional: no weight"); + if (totalWeight == 0) { + revert GovernorCountingFractional_NoVoteWeight(); + } + if (_proposalVotersWeightCast[proposalId][account] >= totalWeight) { - revert("GovernorCountingFractional: all weight cast"); + revert GovernorCountingFractional__VoteWeightExceeded(); } uint128 safeTotalWeight = SafeCast.toUint128(totalWeight); @@ -159,10 +167,9 @@ abstract contract GovernorCountingFractional is Governor { uint128 totalWeight, uint8 support ) internal { - require( - _proposalVotersWeightCast[proposalId][account] == 0, - "GovernorCountingFractional: vote would exceed weight" - ); + if (_proposalVotersWeightCast[proposalId][account] != 0) { + revert GovernorCountingFractional__VoteWeightExceeded(); + } _proposalVotersWeightCast[proposalId][account] = totalWeight; @@ -173,7 +180,7 @@ abstract contract GovernorCountingFractional is Governor { } else if (support == uint8(VoteType.Abstain)) { _proposalVotes[proposalId].abstainVotes += totalWeight; } else { - revert("GovernorCountingFractional: invalid support value, must be included in VoteType enum"); + revert GovernorInvalidVoteType(); } } @@ -204,14 +211,18 @@ abstract contract GovernorCountingFractional is Governor { uint128 totalWeight, bytes memory voteData ) internal { - require(voteData.length == 48, "GovernorCountingFractional: invalid voteData"); + if (voteData.length != 48) { + revert GovernorCountingFractional__InvalidVoteData(); + } (uint128 _againstVotes, uint128 _forVotes, uint128 _abstainVotes) = _decodePackedVotes(voteData); uint128 _existingWeight = _proposalVotersWeightCast[proposalId][account]; uint256 _newWeight = uint256(_againstVotes) + _forVotes + _abstainVotes + _existingWeight; - require(_newWeight <= totalWeight, "GovernorCountingFractional: vote would exceed weight"); + if (_newWeight > totalWeight) { + revert GovernorCountingFractional__VoteWeightExceeded(); + } // It's safe to downcast here because we've just confirmed that // _newWeight <= totalWeight, and totalWeight is a uint128. diff --git a/test/FractionalPool.t.sol b/test/FractionalPool.t.sol index 9c6039a..7710fc3 100644 --- a/test/FractionalPool.t.sol +++ b/test/FractionalPool.t.sol @@ -455,7 +455,11 @@ contract Vote is FractionalPoolTest { pool.castVote(_proposalId); // Try to submit them again. - vm.expectRevert(bytes("GovernorCountingFractional: all weight cast")); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); pool.castVote(_proposalId); } diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index ff66f78..83402e1 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -7,7 +7,7 @@ import {FractionalPool, IVotingToken, IFractionalGovernor} from "../src/Fraction import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol"; import {GovToken} from "./GovToken.sol"; -import {FractionalGovernor} from "./FractionalGovernor.sol"; +import {FractionalGovernor, GovernorCountingFractional} from "./FractionalGovernor.sol"; import {ProposalReceiverMock} from "./ProposalReceiverMock.sol"; import {IVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; import {IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; @@ -410,7 +410,11 @@ contract GovernorCountingFractionalTest is Test { // Attempt to cast nominal votes. vm.prank(_voterAddr); - vm.expectRevert("GovernorCountingFractional: no weight"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional_NoVoteWeight.selector + ) + ); governor.castVoteWithReasonAndParams( _proposalId, uint8(VoteType.For), @@ -420,7 +424,11 @@ contract GovernorCountingFractionalTest is Test { // Attempt to cast fractional votes. vm.prank(_voterAddr); - vm.expectRevert("GovernorCountingFractional: no weight"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional_NoVoteWeight.selector + ) + ); governor.castVoteWithReasonAndParams( _proposalId, uint8(VoteType.For), @@ -484,7 +492,11 @@ contract GovernorCountingFractionalTest is Test { ); vm.prank(voter.addr); - vm.expectRevert("GovernorCountingFractional: vote would exceed weight"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Yay", fractionalizedVotes); } @@ -557,7 +569,11 @@ contract GovernorCountingFractionalTest is Test { uint256 _proposalId = _createAndSubmitProposal(); vm.prank(voter.addr); - vm.expectRevert("GovernorCountingFractional: invalid voteData"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__InvalidVoteData.selector + ) + ); governor.castVoteWithReasonAndParams(_proposalId, voter.support, "Weeee", _invalidVoteData); } @@ -974,7 +990,11 @@ contract GovernorCountingFractionalTest is Test { // Attempt to cast votes again. This should revert. vm.prank(_voter.addr); - vm.expectRevert("GovernorCountingFractional: vote would exceed weight"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); governor.castVoteWithReasonAndParams( _proposalId, _voter.support, @@ -1001,7 +1021,11 @@ contract GovernorCountingFractionalTest is Test { // It should not be possible to vote again. vm.prank(_voter.addr); - vm.expectRevert("GovernorCountingFractional: all weight cast"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); governor.castVoteWithReasonAndParams( _proposalId, _voter.support, "Yay", _emptyDataBecauseWereVotingNominally ); @@ -1048,7 +1072,11 @@ contract GovernorCountingFractionalTest is Test { ); // Now attempt to vote fractionally. It should fail. - vm.expectRevert("GovernorCountingFractional: all weight cast"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); vm.prank(_voter.addr); governor.castVoteWithReasonAndParams( _proposalId, _voter.support, "Fractional vote", _fractionalizedVoteData @@ -1069,7 +1097,11 @@ contract GovernorCountingFractionalTest is Test { ); vm.prank(_voter.addr); - vm.expectRevert("GovernorCountingFractional: vote would exceed weight"); + vm.expectRevert( + abi.encodeWithSelector( + GovernorCountingFractional.GovernorCountingFractional__VoteWeightExceeded.selector + ) + ); governor.castVoteWithReasonAndParams( _proposalId, _voter.support, "Nominal vote", _nominalVoteData ); From 8ad67a73bcd384236fa92a7d8c1a97257a42d142 Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Sat, 27 Apr 2024 17:29:10 -0400 Subject: [PATCH 11/13] Improve natspec --- src/GovernorCountingFractional.sol | 66 +++++++++++++++++------------- 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index e101fcd..099b71a 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {Governor} from "@openzeppelin/contracts/governance/Governor.sol"; +import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; /** @@ -20,12 +20,18 @@ import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; */ abstract contract GovernorCountingFractional is Governor { + /// @notice Thrown when casting a vote would exceed the weight delegated to the voting account. error GovernorCountingFractional__VoteWeightExceeded(); + + /// @notice Thrown when params data submitted with a vote does not match the convention expected for fractional voting. error GovernorCountingFractional__InvalidVoteData(); + + /// @notice Thrown when vote is cast by an account that has no voting weight. error GovernorCountingFractional_NoVoteWeight(); /** - * @dev Supported vote types. Matches Governor Bravo ordering. + * @notice Supported vote types. + * @dev Matches Governor Bravo ordering. */ enum VoteType { Against, @@ -33,6 +39,12 @@ abstract contract GovernorCountingFractional is Governor { Abstain } + /** + * @notice Metadata about how votes were cast for a given proposal. + * @param againstVotes The number of votes cast Against a proposal. + * @param forVotes The number of votes cast For a proposal. + * @param abstainVotes The number of votes that Abstain from voting for a proposal. + */ struct ProposalVote { uint128 againstVotes; uint128 forVotes; @@ -40,43 +52,45 @@ abstract contract GovernorCountingFractional is Governor { } /** - * @dev Mapping from proposal ID to vote tallies for that proposal. + * @notice Mapping from proposal ID to vote tallies for that proposal. */ mapping(uint256 => ProposalVote) private _proposalVotes; /** - * @dev Mapping from proposal ID and address to the weight the address + * @notice Mapping from proposal ID and address to the weight the address * has cast on that proposal, e.g. _proposalVotersWeightCast[42][0xBEEF] * would tell you the number of votes that 0xBEEF has cast on proposal 42. */ mapping(uint256 => mapping(address => uint128)) private _proposalVotersWeightCast; - /** - * @dev See {IGovernor-COUNTING_MODE}. - */ - // solhint-disable-next-line func-name-mixedcase + /// @inheritdoc IGovernor function COUNTING_MODE() public pure virtual override returns (string memory) { return "support=bravo&quorum=for,abstain¶ms=fractional"; } - /** - * @dev See {IGovernor-hasVoted}. - */ + /// @inheritdoc IGovernor function hasVoted(uint256 proposalId, address account) public view virtual override returns (bool) { return _proposalVotersWeightCast[proposalId][account] > 0; } /** - * @dev Get the number of votes cast thus far on proposal `proposalId` by + * @notice Get the number of votes cast thus far on proposal `proposalId` by * account `account`. Useful for integrations that allow delegates to cast * rolling, partial votes. + * @param proposalId Identifier of any past or present proposal. + * @param account The voting account in question. + * @return The total voting weight cast so far for this proposal by this account */ function voteWeightCast(uint256 proposalId, address account) public view returns (uint128) { return _proposalVotersWeightCast[proposalId][account]; } /** - * @dev Accessor to the internal vote counts. + * @notice Accessor to the internal vote counts. + * @param proposalId Identifier of any past or present proposal. + * @return againstVotes The Against votes cast so far for the given proposal. + * @return forVotes The For votes cast so far for given proposal. + * @return abstainVotes The Abstain votes cast so far for the given proposal. */ function proposalVotes(uint256 proposalId) public @@ -92,9 +106,7 @@ abstract contract GovernorCountingFractional is Governor { return (proposalVote.againstVotes, proposalVote.forVotes, proposalVote.abstainVotes); } - /** - * @dev See {Governor-_quorumReached}. - */ + /// @inheritdoc Governor function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) { ProposalVote storage proposalVote = _proposalVotes[proposalId]; @@ -102,7 +114,8 @@ abstract contract GovernorCountingFractional is Governor { } /** - * @dev See {Governor-_voteSucceeded}. In this module, forVotes must be > againstVotes. + * @inheritdoc Governor + * @dev In this module, forVotes must be > againstVotes. */ function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) { ProposalVote storage proposalVote = _proposalVotes[proposalId]; @@ -111,11 +124,8 @@ abstract contract GovernorCountingFractional is Governor { } /** - * @notice See {Governor-_countVote}. - * - * @dev Function that records the delegate's votes. - * - * If the `voteData` bytes parameter is empty, then this module behaves + * @inheritdoc Governor + * @dev If the `voteData` bytes parameter is empty, then this module behaves * identically to GovernorBravo. That is, it assigns the full weight of the * delegate to the `support` parameter, which follows the `VoteType` enum * from Governor Bravo. @@ -155,9 +165,8 @@ abstract contract GovernorCountingFractional is Governor { } /** - * @dev Record votes with full weight cast for `support`. - * - * Because this function votes with the delegate's full weight, it can only + * @notice Record votes with full weight cast for `support`. + * @dev Because this function votes with the delegate's full weight, it can only * be called once per proposal. It will revert if combined with a fractional * vote before or after. */ @@ -185,9 +194,8 @@ abstract contract GovernorCountingFractional is Governor { } /** - * @dev Count votes with fractional weight. - * - * `voteData` is expected to be three packed uint128s, i.e. + * @notice Count votes with fractional weight. + * @dev `voteData` is expected to be three packed uint128s, i.e. * `abi.encodePacked(againstVotes, forVotes, abstainVotes)`. * * This function can be called multiple times for the same account and @@ -241,7 +249,7 @@ abstract contract GovernorCountingFractional is Governor { uint256 constant internal _MASK_HALF_WORD_RIGHT = 0xffffffffffffffffffffffffffffffff; // 128 bits of 0's, 128 bits of 1's /** - * @dev Decodes three packed uint128's. Uses assembly because of a Solidity + * @notice Decodes three packed uint128's. Uses assembly because of a Solidity * language limitation which prevents slicing bytes stored in memory, rather * than calldata. */ From 4a7153201d3a17387f25a72881d632afd04dcb0f Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Sat, 27 Apr 2024 17:34:49 -0400 Subject: [PATCH 12/13] Add SPDX license identifier to FlexVotingClient --- src/FlexVotingClient.sol | 2 +- src/GovernorCountingFractional.sol | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/FlexVotingClient.sol b/src/FlexVotingClient.sol index a260af5..6967f3a 100644 --- a/src/FlexVotingClient.sol +++ b/src/FlexVotingClient.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: UNLICENSED +// SPDX-License-Identifier: MIT pragma solidity >=0.8.10; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index 099b71a..303cca8 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -1,5 +1,4 @@ // SPDX-License-Identifier: MIT - pragma solidity ^0.8.0; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; From 6e4f4170593b8922cce9131a6456e522e1a167bc Mon Sep 17 00:00:00 2001 From: Ben DiFrancesco Date: Sat, 27 Apr 2024 17:47:45 -0400 Subject: [PATCH 13/13] Update the compiler version to the latest, and match pragmas to OpenZeppelin --- foundry.toml | 4 +--- script/Deploy.s.sol | 3 +-- src/FlexVotingClient.sol | 2 +- src/FractionalPool.sol | 2 +- src/GovernorCountingFractional.sol | 2 +- src/interfaces/IFractionalGovernor.sol | 2 +- src/interfaces/IVotingToken.sol | 2 +- test/FractionalGovernor.sol | 2 +- test/FractionalPool.t.sol | 2 +- test/GovToken.sol | 2 +- test/GovernorCountingFractional.t.sol | 2 +- test/ProposalReceiverMock.sol | 2 +- 12 files changed, 12 insertions(+), 15 deletions(-) diff --git a/foundry.toml b/foundry.toml index 7c325cf..3244911 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,10 +1,8 @@ [profile.default] - # We don't specify a solc version because Aave contracts are pinned to 0.8.10, - # but we use more recent solc versions for other contracts, so we let forge - # auto-detect solc versions. optimizer = true optimizer_runs = 10_000_000 remappings = ["@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts"] + solc_version = '0.8.25' verbosity = 3 [profile.ci] diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 7f82173..c68424a 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -1,6 +1,5 @@ // SPDX-License-Identifier: MIT - -pragma solidity ^0.8.10; +pragma solidity ^0.8.20; import "forge-std/Script.sol"; diff --git a/src/FlexVotingClient.sol b/src/FlexVotingClient.sol index 6967f3a..08aa326 100644 --- a/src/FlexVotingClient.sol +++ b/src/FlexVotingClient.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; import {Checkpoints} from "@openzeppelin/contracts/utils/structs/Checkpoints.sol"; diff --git a/src/FractionalPool.sol b/src/FractionalPool.sol index 5dc076a..e380a26 100644 --- a/src/FractionalPool.sol +++ b/src/FractionalPool.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.10; +pragma solidity ^0.8.20; import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol"; import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; diff --git a/src/GovernorCountingFractional.sol b/src/GovernorCountingFractional.sol index 303cca8..93213a6 100644 --- a/src/GovernorCountingFractional.sol +++ b/src/GovernorCountingFractional.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; +pragma solidity ^0.8.20; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol"; diff --git a/src/interfaces/IFractionalGovernor.sol b/src/interfaces/IFractionalGovernor.sol index 6541fe4..4aef6b6 100644 --- a/src/interfaces/IFractionalGovernor.sol +++ b/src/interfaces/IFractionalGovernor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; /// @dev The interface that flexible voting-compatible governors are expected to support. interface IFractionalGovernor { diff --git a/src/interfaces/IVotingToken.sol b/src/interfaces/IVotingToken.sol index a166347..f8e3809 100644 --- a/src/interfaces/IVotingToken.sol +++ b/src/interfaces/IVotingToken.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; /// @dev The interface that flexible voting-compatible voting tokens are expected to support. interface IVotingToken { diff --git a/test/FractionalGovernor.sol b/test/FractionalGovernor.sol index 3d93e44..54d26a0 100644 --- a/test/FractionalGovernor.sol +++ b/test/FractionalGovernor.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; import {GovernorCountingFractional} from "../src/GovernorCountingFractional.sol"; import {GovernorVotes} from "@openzeppelin/contracts/governance/extensions/GovernorVotes.sol"; diff --git a/test/FractionalPool.t.sol b/test/FractionalPool.t.sol index 7710fc3..80723bb 100644 --- a/test/FractionalPool.t.sol +++ b/test/FractionalPool.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.10; +pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; diff --git a/test/GovToken.sol b/test/GovToken.sol index 2fb6cc5..55e0f5e 100644 --- a/test/GovToken.sol +++ b/test/GovToken.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Votes.sol"; import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol"; diff --git a/test/GovernorCountingFractional.t.sol b/test/GovernorCountingFractional.t.sol index 83402e1..ef38939 100644 --- a/test/GovernorCountingFractional.t.sol +++ b/test/GovernorCountingFractional.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; import {Test} from "forge-std/Test.sol"; import {Vm} from "forge-std/Vm.sol"; diff --git a/test/ProposalReceiverMock.sol b/test/ProposalReceiverMock.sol index 8744284..e70f51b 100644 --- a/test/ProposalReceiverMock.sol +++ b/test/ProposalReceiverMock.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity >=0.8.10; +pragma solidity ^0.8.20; contract ProposalReceiverMock { event MockFunctionCalled();