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

Add ERC20 Permit (EIP-2612) #2237

Merged
merged 38 commits into from
Dec 11, 2020
Merged

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented May 15, 2020

Fixes #2206

This adds support for the ERC20 Permit extension, tentatively called ERC2612 and based on the discussion in ethereum/EIPs#2612. The EIP is still in draft status, we'll need to monitor it for any changes, and will likely not want to publish this code until it is more mature / finalized so it will go in the drafts directory.

This implementation accounts for ChainID changing (i.e. a hard fork) while at the same time being as gas efficient as possible.

The code and documentation is ready, but the tests are not,

To do

  • Add tests
  • Move to drafts directory
  • Rebase once Add EIP 712 helpers #2418 is merged
  • Review contract name: ERC20Permit vs ERC2612Permit or ERC2612

@nventuro nventuro requested a review from frangio May 15, 2020 23:58
@frangio
Copy link
Contributor

frangio commented May 16, 2020

Previous discussion about EIP712 in #1810.

@nventuro
Copy link
Contributor Author

The Uniswap tests might be a good starting place to write the tests for this. We can also look into using eth_signTypedData.

abi.encode(
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
keccak256(bytes(name())),
keccak256(bytes("1")), // Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we should use pure internal method version() or permitVersion() for this.

Copy link
Contributor Author

@nventuro nventuro Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting an internal setter for users to set their own version field? I'm actually not sure what all the use cases for version are, if you have insights on this that'd be extremely helpful.

That said we do need to provide a way for either the version or domain separator to be queried, as noted here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe version makes sense when behaviour changes without address change (upgradablity?). So I mean version method could be overrided by user, or maybe should be even abstract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It'd be great to have some examples to show in the docs.

Regarding implementation, we could have a setter and a getter, where the setter also calls _updateDomainSeparator and not increase gas costs of permit that way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k06a In this case though the semantics of the signature are defined by the EIP, so I don't see a reason for version to change.

@stale

This comment has been minimized.

@stale stale bot added the stale label Jul 16, 2020
@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Jul 17, 2020
@stale stale bot removed the stale label Jul 17, 2020
@frangio
Copy link
Contributor

frangio commented Jul 17, 2020

While we still have to work on the tests for this feature, it's kind of on hold because the ERC is in Draft status.

@dmihal
Copy link

dmihal commented Aug 13, 2020

Stumbled upon this while working on my eth-permit library

Might be useful for your unit tests

@frangio
Copy link
Contributor

frangio commented Aug 25, 2020

@dmihal Looks like it will be useful, thank you!

@frangio frangio changed the title Add ERC20 Permit EIP-2612: Add ERC20 Permit Sep 16, 2020
@frangio frangio changed the title EIP-2612: Add ERC20 Permit Add ERC20 Permit (EIP-2612) Sep 16, 2020
@gorgos
Copy link
Contributor

gorgos commented Oct 17, 2020

With a few modifications, I successfully used the eth-permit library to run some basic tests.

@dmihal
Copy link

dmihal commented Oct 19, 2020

@gorgos the new version of eth-permit should work correctly.

And feel free to add PRs to https://github.com/dmihal/eth-permit 🙂

@frangio
Copy link
Contributor

frangio commented Oct 19, 2020

Is anyone familiar with the finalization status of this EIP? If there are any plans to move it to Final?

@frangio frangio marked this pull request as ready for review December 4, 2020 16:26
@frangio
Copy link
Contributor

frangio commented Dec 4, 2020

@spalladino @mverzilli The code is ready for review. I still need to review the documentation a little bit. If you have any opinions on that front let me know.

spalladino
spalladino previously approved these changes Dec 4, 2020
Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

contracts/drafts/IERC2612Permit.sol Outdated Show resolved Hide resolved
contracts/drafts/IERC2612Permit.sol Outdated Show resolved Hide resolved
contracts/token/ERC20/README.adoc Outdated Show resolved Hide resolved
test/drafts/ERC20Permit.test.js Show resolved Hide resolved
test/drafts/ERC20Permit.test.js Outdated Show resolved Hide resolved
Co-authored-by: Santiago Palladino <spalladino@gmail.com>
mverzilli
mverzilli previously approved these changes Dec 11, 2020
@frangio frangio merged commit ecc6671 into OpenZeppelin:master Dec 11, 2020
@nventuro nventuro deleted the erc20-permit branch January 15, 2021 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EIP-2612: Add ERC20 permit() function
9 participants