-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: master
Are you sure you want to change the base?
Upgrade to OZ v5 #66
Conversation
* | ||
* See {fractionalVoteNonce} and {_castVote} for more information. | ||
*/ | ||
function castVoteWithReasonAndParamsBySig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This 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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed because there is no Trace256
. Should we write one for OZ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traces are a list of checkpoint. Each checkpoint contains:
- a key
- a value
such that sizeof(key) + sizeof(value) = 256
Trace224
isuint224
for value, anduint32
for keyTrace208
isuint208
for value, anduint48
for key
Trace256
would not have any room for the key ... unless you start have checkpoints that are 2 slots.
} | ||
|
||
/// @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -115,7 +116,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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -106,7 +106,7 @@ jobs: | |||
uses: zgosalvez/github-actions-report-lcov@v2 | |||
with: | |||
coverage-files: ./lcov.info | |||
minimum-coverage: 70 # Set coverage threshold. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add more tests
IGovernor.GovernorUnexpectedProposalState.selector, | ||
_proposalId, | ||
status, | ||
bytes32(1 << uint8(IGovernor.ProposalState.Succeeded)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a 208?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to allign with ERC-5805, then yes
Coverage after merging feature/support-oz-5 into master will be
Coverage Report
|
Description
castVoteWithReasonAndParamsBySig
has a nonce now so we can remove our method override.GovernorCompatibilityBravo
contract was removed so we need to add our own VoteType struct