Skip to content

Use Trace208 in Votes to support ERC6372 clocks #4539

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.

Sign up for GitHub

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

Merged
frangio merged 10 commits into from
Aug 30, 2023
Merged

Use Trace208 in Votes to support ERC6372 clocks #4539

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
06b4189
Use trace208 in Checkpoints to support full uint48 keys for ERC6372 c…
Amxx Aug 25, 2023
48fba73
add changeset
Amxx Aug 25, 2023
15ca017
Update wild-peas-remain.md
frangio Aug 25, 2023
74b8d26
accept storage breakage when labeled as breaking
frangio Aug 25, 2023
d5ab954
trigger tests
frangio Aug 25, 2023
b2cd627
Merge branch 'master' into refactor/checkpoints/use-trace208
Amxx Aug 28, 2023
bd9595d
document
Amxx Aug 30, 2023
2d93da5
Update ERC20Votes.sol
Amxx Aug 30, 2023
bc335f5
Update ERC20Votes.sol
Amxx Aug 30, 2023
7927100
lint
frangio Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
document
  • Loading branch information
@Amxx
Amxx committed Aug 30, 2023
commit bd9595d894fe210a64d9f93d93b3f39120902e11
6 changes: 6 additions & 0 deletions contracts/token/ERC20/extensions/ERC20Votes.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ abstract contract ERC20Votes is ERC20, Votes {

/**
* @dev Maximum token supply. Defaults to `type(uint208).max` (2^208^ - 1).
*
* This maximum is enforced in {_update}. It limits the total supply of the token, which is otherwize a uint256, so
* that checkpoints can be stored in Trace208 structure used by {{Votes}}. Increasing this value will not remove
* the underlying limitation, and will the {_update} to fail because of a math overflow in {_transferVotingUnits}.
* An override could be used to further restrict the total supply (to a lower value) if additional logic requires
* it. When resolving override conflicts on this function, the minimum should be returned.
*/
function _maxSupply() internal view virtual returns (uint256) {
Copy link
Contributor

@frangio frangio Aug 25, 2023

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.

Suggested change
function _maxSupply() internal view virtual returns (uint256) {
function _maxSupply() internal view virtual returns (uint208) {

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023 •
edited
Loading

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)

Copy link
Contributor

@frangio frangio Aug 25, 2023

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.

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023 •
edited
Loading

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

Copy link
Contributor

@frangio frangio Aug 25, 2023

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.

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023

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.

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023

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

Copy link
Contributor

@frangio frangio Aug 25, 2023

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.

Copy link
Contributor

@frangio frangio Aug 25, 2023

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.

return type(uint208).max;
Expand Down