-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Use Trace208 in Votes to support ERC6372 clocks #4539
Use Trace208 in Votes to support ERC6372 clocks #4539
Conversation
🦋 Changeset detectedLatest commit: 7927100 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
*/ | ||
function _maxSupply() internal view virtual returns (uint224) { | ||
return type(uint224).max; | ||
function _maxSupply() 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.
Why change this to uint256
? Using the smaller type prevents returning a number that is too large for the data structure to support it.
function _maxSupply() internal view virtual returns (uint256) { | |
function _maxSupply() internal view virtual returns (uint208) { |
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.
I'm honestly not sure what it implies/change to use uint208 vs uint256 here (note that it's internal)
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.
uint208
makes it a type error to override with a function that returns type(uint256).max
, for example, or any literal that is too large to fit uint208.
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.
What if multiple modules implement that function with different values (votes limit to 208, but other modules would limit to 224, 192 or 128 for some other reason) how would we resolve that ?
If the return types are different it's a mess. If they are both uint256 there may be some hope
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 is not a concrete problem we have currently so I would go with the option that is safer.
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 that issue happens during the 5.x cycle, would we be able to change it without a major change ?
I don't think overriding this with a bigger function is something anyone would realistically do.
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.
Also the only place we use it, we do an implicit upcast to 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.
If something like this happens in a minor release I think we would just add an internal function with a name other than _maxSupply
so they wouldn't clash, and ERC20Votes
could implement that internal function in terms of the existing one.
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 feel strongly about this I can accept it. The risk if >uint208 is returned is simply that some transfers might revert but it's a recoverable situation.
In 74b8d26 I added openzeppelin-contracts/.github/workflows/changeset.yml Lines 7 to 11 in b2e7bab
Also, we may want to use a separate label specific to storage breakage, so we don't accidentally miss storage changes when we're doing a less serious breaking change in a minor release. @Amxx what do you think? |
A tag is nice, but I think we should also consider the changeset. This could be done in two ways:
I don't have a strong opinion for the tag name. I'm ok with "breaking change", but if you want to make it more storage specific, then "storage change" would be ok I guess. The question is, do we want all "breaking change" tags to require a major changeset, or do we accept such tags in patch releases? |
Is your idea that the layout check would be disabled if there is any major changeset in the PR branch, or if the current PR is adding a new major changeset? I went with the label because I thought you meant the second option and it seemed a lot more complicated to implement. |
if the current PR is adding a new major changeset. I think is label is good for know. |
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.
I would prefer to make _maxSupply
return uint208
but not a blocker to merge. We should probably document the consequence of returning a number that is larger than type(uint208).max
though.
I added some documentation. |
Checkpoints.Checkpoint224 storage latest = _quorumNumeratorHistory._checkpoints[length - 1]; | ||
uint32 latestKey = latest._key; | ||
uint224 latestValue = latest._value; | ||
Checkpoints.Checkpoint208 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; |
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.
Why memory?
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.
The merge with master
wasn't done correctly. Thanks for raising.
Co-authored-by: Francisco <fg@frang.io>
ERC-6372 clocks are 48 bits but our Votes contracts use 32 bit keys. This changes the Votes library to match clocks and use 48 bit keys, to avoid fallible conversions.
PR Checklist
npx changeset add
)