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

Refactor Nonces from contract to library to mitigate nonces collision between Votes and ERC20Permit #3848

Open
wants to merge 1 commit into
base: next-v5.0
Choose a base branch
from

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Dec 2, 2022

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@k06a k06a changed the base branch from master to next-v5.0 December 2, 2022 11:26
@k06a
Copy link
Contributor Author

k06a commented Dec 2, 2022

I see more issues:

  • Nonces of Permit and Votes were different sequences in 4.8.0, but in next-v5.0 branch you currently have one sequence of nonces. Was this an intentional change or not?
  • Both old and future versions of your library have no nonces cancellation mechanism, but why?

We have started new EIP to specify how exactly nonces should serve different signature-based operations: ethereum/EIPs#6077

@frangio
Copy link
Contributor

frangio commented Dec 2, 2022

Nonces of Permit and Votes were different sequences in 4.8.0, but in next-v5.0 branch you currently have one sequence of nonces. Was this an intentional change or not?

There is no such difference between 4.8.0 and the current 5.0 branch. ERC20Votes in 4.8.0 implicitly includes ERC20Permit and reuses the same nonce mechanism. Where are you seeing otherwise?

Both old and future versions of your library have no nonces cancellation mechanism, but why?

In the case of Permit we've just implemented the EIP and it doesn't have nonce cancellation (although it's simple to create a permit to cancel a nonce), and in the case of Votes we've implemented a small variant of the COMP interface and didn't add nonce cancellation. We can consider it though.

Regarding "nonce collision". Can you explain the scenario where you see it being a problem that permits and delegation use the same nonce sequence? In my experience a signature is produced and used almost immediately. Are signatures ever long lived?

@k06a
Copy link
Contributor Author

k06a commented Dec 2, 2022

Regarding "nonce collision". Can you explain the scenario where you see it being a problem that permits and delegation use the same nonce sequence? In my experience a signature is produced and used almost immediately. Are signatures ever long lived?

@frangio these different typehashes describes different offchain signature-based activities. Why they should rely on the same sequence of nonces? What if we would end up to token with 5-10 typehashes, will they all be in the same sequence?

For example with Permit and Votes: if you would delegate by signature, it will be auto-cancelled if you will perform trade with permit on DEX/Aggregator. Delegatee for example could perform late claim of delegations only from those who had top balances for example.

@frangio
Copy link
Contributor

frangio commented Dec 5, 2022

Yeah. While I agree with this PR conceptually, it's a breaking change and I don't think there's a way to make it backwards compatible.

@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Dec 5, 2022
@k06a
Copy link
Contributor Author

k06a commented Dec 5, 2022

@frangio we can add missing methods to keep compatibility with prev versions...

@frangio
Copy link
Contributor

frangio commented Dec 6, 2022

How do you make that work? They would be reduced back to a single sequence because applications will take the result of nonces() and use it with permit and delegation signatures.

@k06a
Copy link
Contributor Author

k06a commented Dec 6, 2022

@frangio I don't understand how exactly it is working in 4.8.0. I see both contract introduce nonces(), but if you will try to inherit from both - it will collide. How do you expect to override this method?

@k06a
Copy link
Contributor Author

k06a commented Dec 6, 2022

I see now:

  • In 4.8.0 Permit and Votes has different nonces but had nonces() method collision
  • In next-v5.0 Permit and Votes has inherited from the same Nonces to have single sequence

So it was already a breaking change, of unifying this sequences. I think since 5.0 is not final we can reconsider this. And we can return nonces() methods to both contracts to have backward compatibility and inter-incompatibility again.

@k06a
Copy link
Contributor Author

k06a commented Dec 6, 2022

I feel we can keep nonces() for Permit to keep compatibility with EIP-2612, but Votes could have different method, since it is not formalised as much as EIP-2612.

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

Successfully merging this pull request may close these issues.

None yet

2 participants