Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to OZ v5 #66

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion lib/openzeppelin-contracts
18 changes: 10 additions & 8 deletions src/FlexVotingClient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a 208?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to allign with ERC-5805, then yes


/// @notice The voting options corresponding to those used in the Governor.
enum VoteType {
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed because there is no Trace256. Should we write one for OZ?

Copy link

@Amxx Amxx May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traces are a list of checkpoint. Each checkpoint contains:

  • a key
  • a value

such that sizeof(key) + sizeof(value) = 256

  • Trace224 is uint224 for value, and uint32 for key
  • Trace208 is uint208 for value, and uint48 for key

Trace256 would not have any room for the key ... unless you start have checkpoints that are 2 slots.

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) {
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

/// @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);
}
}
108 changes: 12 additions & 96 deletions src/GovernorCountingFractional.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -250,96 +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.
*/
function castVoteWithReasonAndParamsBySig(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added because the v4 version did not handle replay attacks. I think the v5 uses a nonce now so we don't need to override it.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/920225a1c7e6e749ba23b10ed076b04f454290e8/contracts/governance/Governor.sol#L577

uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
) public virtual override 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);
}

}
20 changes: 4 additions & 16 deletions test/FractionalGovernor.sol
Original file line number Diff line number Diff line change
@@ -1,26 +1,14 @@
// 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_) {}

function castVoteWithReasonAndParamsBySig(
uint256 proposalId,
uint8 support,
string calldata reason,
bytes memory params,
uint8 v,
bytes32 r,
bytes32 s
) public virtual override(GovernorCountingFractional, Governor) returns (uint256) {
return GovernorCountingFractional.castVoteWithReasonAndParamsBySig(
proposalId, support, reason, params, v, r, s
);
}

function quorum(uint256) public pure override returns (uint256) {
return 10 ether;
}
Expand Down
18 changes: 16 additions & 2 deletions test/FractionalPool.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Votes.sol uses Checkpoints.Trace208 for the checkpoints. This is a consequence of the adoption of uint48 for all clocks/timestamps, following ERC-5805

vm.assume(_holder != address(pool));
uint256 initialBalance = token.balanceOf(_holder);

Expand Down Expand Up @@ -245,9 +250,18 @@ 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);
}

Expand Down
19 changes: 17 additions & 2 deletions test/GovToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
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";
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 {
Expand All @@ -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);
}
}