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

Make ERC20Votes independent from ERC20Permit #3160

Closed
k06a opened this issue Feb 2, 2022 · 5 comments
Closed

Make ERC20Votes independent from ERC20Permit #3160

k06a opened this issue Feb 2, 2022 · 5 comments
Labels
breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@k06a
Copy link
Contributor

k06a commented Feb 2, 2022

🧐 Motivation

We need a more clear and independent structure of ERC20 extensions.

📝 Details

Now ERC20Votes inherits ERC20Permit to use EIP712 inheritance and moreover to abuse its permit nonces. I believe we can remove the ERC20Permit constructor to keep it abstract and make ERC20Votes have its own nonces for delegation and direct inheritance from EIP712.

Moreover, I see you have Votes implementation since 4.5, so hopefully, you are going to use this implementation in ERC20Votes.

@Amxx
Copy link
Collaborator

Amxx commented Feb 2, 2022

Moreover, I see you have Votes implementation since 4.5, so hopefully, you are going to use this implementation in ERC20Votes.

We would love to, but that would change the storage layout, which is a big breaking change for the upgradeable version of the contract :/

@frangio
Copy link
Contributor

frangio commented Feb 2, 2022

The storage layout is incompatible because of the nonces mapping in Votes, which in ERC20Votes is reused from ERC20Permit as pointed out here. Other than that, some variables were wrapped in structs which should be soon supported by the Upgrades Plugins as a valid upgrade.

The nonces are an issue but interestingly it's part of @k06a's proposa to change that as well. However:

make ERC20Votes have its own nonces

How would this work? The Votes interface is not standardized but the EIP712 signatures for voting by sig should use the value returned by the nonces() function so I don't see how we could change this without affecting clients that rely on this.

remove the ERC20Permit constructor to keep it abstract

This is a breaking change but we agree, we will have to do it in the next major release.

@k06a Is there something in particular that motivates these suggestions?

@k06a
Copy link
Contributor Author

k06a commented Feb 5, 2022

@frangio was just browsing library code and wondered when saw code duplications and unexpected dependencies.

@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Feb 7, 2022
@frangio frangio added this to the 5.0 milestone Aug 26, 2022
@frangio frangio changed the title Fix ERC20Votes dependencies Make ERC20Votes independent of ERC20Permit Sep 16, 2022
@frangio
Copy link
Contributor

frangio commented Sep 16, 2022

Changed the title to make it clearer but as the description says this also implies changing ERC20Permit so it doesn't invoke the constructor of EIP712.

@frangio
Copy link
Contributor

frangio commented Dec 2, 2022

Fixed by #3816 for future 5.0 release.

@frangio frangio closed this as completed Dec 2, 2022
@k06a k06a changed the title Make ERC20Votes independent of ERC20Permit Make ERC20Votes independent from ERC20Permit Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

No branches or pull requests

3 participants