ERC for `permit`: 712-signed token approvals #2612

MrChico · 2020-04-24T09:46:34Z

An ERC for the permit token extension, as popularized by the Dai ERC20. Will update eip number and discussions-to once I find out which PR this ends up being

EIPS/eip-2612.md

gakonst · 2020-04-24T10:52:27Z

EIPS/eip-2612.md

+
+The `nonces` mapping is given for replay protection.
+
+A common use case of `permit` has a relayer submit a `Permit` on behalf of the `owner`. In this scenario, the relaying party is essentially given a free option to submit or withhold the `Permit`. If this is a cause of concern, the `owner` can limit the time a `Permit` is valid for by setting `deadline` to a value in the near future. The `deadline` argument can be set to `uint(-1)` to create `Permit`s that effectively never expire.


👍 this is a good point

gakonst · 2020-04-24T10:54:21Z

EIPS/eip-2612.md

+
+## Backwards Compatibility
+There are currently two slightly differing implementations of this ERC, and we are forced to make a choice here for specificity.
+This implies that the given ERC deviates from the implementation given in the Dai and Chai ERC20 contracts. There, the `permit` method takes `nonce` as an additional argument, and the `uint256 value` argument is exchanged for `bool approval`, admitting binary approvals only. There is also a slight difference in argument names. The specification presented here is in line with the implementation in [Uniswap-v2](https://github.com/uniswap/uniswap-v2-core). This mismatch is a little unfortunate, but not very different from the variations found in ERC20 contracts. 


Is this a big deal? Could potentially have a 1-way conversion contract for migrating Chai to a version which is compatible with this ERC. Also not sure how widespread usage of the DAI permit at the moment is

EIPS/eip-2612.md

MrChico · 2020-04-24T11:54:28Z

Thank you @axic and @gakonst for the review. Discussions about the ERC itself are now directed to #2613, leaving this PR for discussing EIP submission technicalities

Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>

EIPS/eip-2612.md

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

EIPS/eip-2612.md

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

EIPS/eip-2612.md

MicahZoltu · 2020-07-27T11:55:10Z

EIPS/eip-2612.md

+
+Note that nowhere in this definition we refer to `msg.sender`. The caller of the `permit` function can be any address.
+
+For smoother debugging, and to be able to validate `permit` signed messages in a STATIC environment, we also require the `DOMAIN_SEPARATOR` to be retrievable through `STATICCALL`. In solidity, this is easily achieved by declaring the `DOMAIN_SEPARATOR` to be `public`.


This doesn't feel like something that needs to be standardized. While I agree it is a good idea, not all good ideas need to be standardized. If it is standardized, it should be standardized by its API surface, not implementation (like suggestions above).

I'm inclined to agree

MicahZoltu · 2020-07-27T11:56:13Z

EIPS/eip-2612.md

+
+## Rationale
+The `permit` function is sufficient for enabling any operation involving ERC-20 tokens to be paid for using the token itself, rather than using ETH. 
+An example of a contract which enables gasless token transactions can be found [here](https://github.com/dapphub/ds-dach).


Include example implementations in the ## Implementations section (inline), rather than linking to external sites/code, whenever possible.

MicahZoltu · 2020-07-27T11:56:43Z

EIPS/eip-2612.md

+The `permit` function is sufficient for enabling any operation involving ERC-20 tokens to be paid for using the token itself, rather than using ETH. 
+An example of a contract which enables gasless token transactions can be found [here](https://github.com/dapphub/ds-dach).
+
+It avoids any calls to unknown code.


Consider expanding on this a bit and explaining how it achieves this.

MicahZoltu · 2020-07-27T11:58:13Z

EIPS/eip-2612.md

+- instead of taking a `value` argument, it takes a bool `allowed`, setting approval to 0 or `uint(-1)`.
+- the `deadline` argument is instead called `expiry`. This is not just a syntactic change, as it effects the contents of the signed message.
+
+There is also an implementation in the token [`Stake`](https://etherscan.io/address/0x0Ae055097C6d159879521C384F1D2123D1f195e6#code) with the same ABI as `dai` but with different semantics: it lets users issue "expiring approvals", that only allow `transferFrom` to occur while `expiry >= block.timestamp`.


As above, remove external links in favor of summaries, inline code, etc.

MicahZoltu · 2020-07-27T11:58:57Z

EIPS/eip-2612.md

+
+## Test Cases
+
+Some basic tests can be found here https://github.com/Uniswap/uniswap-v2-core/blob/master/test/UniswapV2ERC20.spec.ts.


Can these test cases be formatted in a language neutral way and inlined here rather than an external link?

It's kind of hard to inline ERC tests imho, and the general reader will be expecting a functional test base on the ERC - even if we move towards removing external links, I think a link to a working test base for ERCs should be encouraged.

../assets/eip-####/files can be used if your test cases are complex. Note that the purpose of the test cases isn't meant to serve the same purpose as a test suite in an application. It is meant to exemplify expected input and output combinations, which should be able to be written as a table or simple JSON test cases. People should not be writing a full test suite implementation in this section.

MicahZoltu · 2020-07-27T11:59:31Z

EIPS/eip-2612.md

+Some basic tests can be found here https://github.com/Uniswap/uniswap-v2-core/blob/master/test/UniswapV2ERC20.spec.ts.
+
+## Implementation
+[UniswapV2ERC20.sol](https://github.com/Uniswap/uniswap-v2-core/blob/master/contracts/UniswapV2ERC20.sol)


Recommend inlining this, and consider stripping out all of the code unrelated to this specification.

MicahZoltu

The structure looks good enough to merge as a draft to me, but you need to satisfy the bot first.

I left a bunch of feedback, and the specification will need some cleanup before it can move to Last Call/Final (like moving stuff into correct sections, fixing EIP links, etc.) but that can be done after merge to draft.

EIPS/eip-2612.md

wighawag · 2020-07-28T09:43:53Z

EIPS/eip-2612.md

+```solidity
+keccak256(hex"1901"
+          ++ keccak256(
+                keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")


A note should be added here that the EIP712Domain does not need to be exactly as specified here.
An implementation might want to add or remove field. The current EIP-712 spec stipulate that "eip712Domain is a struct named EIP712Domain with one or more of the below fields"

wighawag · 2020-07-28T09:45:52Z

EIPS/eip-2612.md

+              ++ bytes(deadline))
+))
+```
+where `++` denotes bytestring concatenation, `tokenAddress` is the address of the token contract, `chainid` is the chain id of the network it is deployed to and `erc20name` is the name of the token as defined by `ERC-20`. `version` is a `string` defined at contract deployment which remains constant throughout the lifetime of the contract, but is otherwise unconstrained.


Similar to my comment above, a note should be added that none of the chainId, version or even name are required.

if the EIP intend to have name always be the erc20 name then this should be formulated more clearly.

MicahZoltu · 2020-08-27T17:05:18Z

Sorry for not merging once the bot passed. I usually check back when I see commits, but for some reason I didn't on this one.

I strongly recommend heeding the feedback left in this PR as it is going to come up again when it comes time for Last Call and the requirements for merging to Last Call (and then final) are much more strict.

* first draft of permit eip * finish the sentence and make footnote prettier * proper name and some typos * Update EIPS/eip-2612.md Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com> * Update EIPS/eip-2612.md Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com> * address review comments * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Update EIPS/eip-2612.md Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu> * Add erc-20 race condition comment * mention stake implementation, address(0) security consideration, pseudocode syntax for signed message * Update EIPS/eip-2612.md * Specify DOMAIN_SEPARATOR must be public * Update EIPS/eip-2612.md Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com> * Update EIPS/eip-2612.md * Update EIPS/eip-2612.md Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> Co-authored-by: Alex Beregszaszi <alex@rtfs.hu> Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

a6-dou · 2023-02-07T09:58:44Z

EIPS/eip-2612.md

+Its implementation differs slightly from the presentation here in that:
+- instead of taking a `value` argument, it takes a bool `allowed`, setting approval to 0 or `uint(-1)`.
+- the `deadline` argument is instead called `expiry`. This is not just a syntactic change, as it effects the contents of the signed message.
+


value and deadline are not the only differences, dai.sol uses holder instead of owner also.
in total it's 3 points:

instead of taking a value argument, it takes a bool allowed, setting approval to 0 or uint(-1).

the deadline argument is instead called expiry. This is not just a syntactic change, as it effects the contents of the signed message.

holder is used instead of owner, similar todeadline <> expiry this will affect the contents of the signed message

@MrChico

https://etherscan.io/token/0x6B175474E89094C44Da98b954EedeAC495271d0F#code#L107

gakonst reviewed Apr 24, 2020

View reviewed changes

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

MrChico mentioned this pull request Apr 24, 2020

EIP-2612: 712-signed token approvals #2613

Closed

MrChico and others added 6 commits April 24, 2020 13:57

first draft of permit eip

1cfbd0e

finish the sentence and make footnote prettier

5331af0

proper name and some typos

ec64da3

Update EIPS/eip-2612.md

19368ed

Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>

Update EIPS/eip-2612.md

aa4ad92

Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>

address review comments

a577bd4

MrChico force-pushed the master branch from 8b9771f to a577bd4 Compare April 24, 2020 11:58

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MrChico and others added 3 commits April 24, 2020 15:12

Update EIPS/eip-2612.md

5c82a1a

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

Update EIPS/eip-2612.md

97ff239

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

Update EIPS/eip-2612.md

c8f438d

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

axic reviewed Apr 24, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

MrChico and others added 3 commits April 24, 2020 15:30

Update EIPS/eip-2612.md

008a031

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

Update EIPS/eip-2612.md

64a75af

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

Update EIPS/eip-2612.md

7150cc5

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Show resolved Hide resolved

MicahZoltu reviewed Jul 27, 2020

View reviewed changes

MicahZoltu approved these changes Jul 27, 2020

View reviewed changes

MrChico commented Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

Update EIPS/eip-2612.md

d96ea24

MrChico commented Jul 27, 2020

View reviewed changes

EIPS/eip-2612.md Outdated Show resolved Hide resolved

Update EIPS/eip-2612.md

17d5c88

wighawag reviewed Jul 28, 2020

View reviewed changes

Ivshti mentioned this pull request Aug 15, 2020

possible ADXToken improvements AmbireTech/adex-protocol-eth#109

Closed

MicahZoltu merged commit 4f00e4f into ethereum:master Aug 27, 2020

lucasvo mentioned this pull request Sep 29, 2020

Support for EIP-2612: EIP712-signed token approvals centrifuge/tinlake-erc20#3

Merged

code423n4 mentioned this pull request Mar 21, 2022

ERC20.sol permit() does not comply with ERC-2612 standard code-423n4/2022-03-maple-findings#26

Closed

a6-dou reviewed Feb 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERC for `permit`: 712-signed token approvals #2612

ERC for `permit`: 712-signed token approvals #2612

MrChico commented Apr 24, 2020

gakonst Apr 24, 2020

This comment was marked as duplicate.

gakonst Apr 24, 2020

MrChico commented Apr 24, 2020

MicahZoltu Jul 27, 2020

MrChico Jul 27, 2020

MicahZoltu Jul 27, 2020

MicahZoltu Jul 27, 2020

MicahZoltu Jul 27, 2020

MicahZoltu Jul 27, 2020

wschwab Nov 14, 2021

MicahZoltu Nov 15, 2021

MicahZoltu Jul 27, 2020

MicahZoltu left a comment

wighawag Jul 28, 2020

wighawag Jul 28, 2020 •

edited

Loading

MicahZoltu commented Aug 27, 2020

a6-dou Feb 7, 2023

a6-dou Feb 7, 2023


		The `nonces` mapping is given for replay protection.

		A common use case of `permit` has a relayer submit a `Permit` on behalf of the `owner`. In this scenario, the relaying party is essentially given a free option to submit or withhold the `Permit`. If this is a cause of concern, the `owner` can limit the time a `Permit` is valid for by setting `deadline` to a value in the near future. The `deadline` argument can be set to `uint(-1)` to create `Permit`s that effectively never expire.


		Note that nowhere in this definition we refer to `msg.sender`. The caller of the `permit` function can be any address.

		For smoother debugging, and to be able to validate `permit` signed messages in a STATIC environment, we also require the `DOMAIN_SEPARATOR` to be retrievable through `STATICCALL`. In solidity, this is easily achieved by declaring the `DOMAIN_SEPARATOR` to be `public`.


		## Test Cases

		Some basic tests can be found here https://github.com/Uniswap/uniswap-v2-core/blob/master/test/UniswapV2ERC20.spec.ts.

ERC for permit: 712-signed token approvals #2612

ERC for permit: 712-signed token approvals #2612

Conversation

MrChico commented Apr 24, 2020

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

MrChico commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicahZoltu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wighawag Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

MicahZoltu commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ERC for `permit`: 712-signed token approvals #2612

ERC for `permit`: 712-signed token approvals #2612

wighawag Jul 28, 2020 •

edited

Loading