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

Governor: Use smaller integers for timing parameters #3208

Closed
cygnusv opened this issue Feb 21, 2022 · 5 comments · Fixed by #4268
Closed

Governor: Use smaller integers for timing parameters #3208

cygnusv opened this issue Feb 21, 2022 · 5 comments · Fixed by #4268
Labels
area: governance breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@cygnusv
Copy link

cygnusv commented Feb 21, 2022

Some voting timing parameters are expressed as uint256, like votingDelay and votingPeriod inGovernorSettings, while the newly introduce GovernorPreventLateQuorum has a votingExtension parameter with type uint64. These should be consistent.

@Amxx
Copy link
Collaborator

Amxx commented Feb 21, 2022

Hello @cygnusv, and thank you for raising that issue.

I'll check what we can do, but please note that we want to move the votingDelay and votingPeriod to uint64

@cygnusv
Copy link
Author

cygnusv commented Feb 21, 2022

Right, either way works 👍

AtlasPilotPuppy added a commit to AtlasPilotPuppy/openzeppelin-contracts that referenced this issue Feb 27, 2022
Governor: Inconsistent types for voting timing parameters OpenZeppelin#3208
Changed time parameters to uint64 for consistency
@AtlasPilotPuppy
Copy link

fixed in #3224

@frangio
Copy link
Contributor

frangio commented Mar 1, 2022

Sorry folks the getters that return uint256 were inherited from GovernorBravo and we defined them that way for compatibility with that contract. That hasn't changed so I think we should keep them this way.

@frangio frangio changed the title Governor: Inconsistent types for voting timing parameters Governor: Use smaller integers for timing parameters Sep 16, 2022
@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Sep 16, 2022
@frangio
Copy link
Contributor

frangio commented Sep 16, 2022

Following up on my previous comment. While the external getters should continue to return uint256, we can change the storage variables to use uint64 in order to more efficiently pack storage.

This is a breaking change that would be scheduled for 5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: governance breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
4 participants