-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
ERC for permit
: 712-signed token approvals
#2612
Conversation
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is a good point
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
EIPS/eip-2612.md
Outdated
|
||
## 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>
Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>
EIPS/eip-2612.md
Outdated
|
||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to agree
|
||
## 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include example implementations in the ## Implementations
section (inline), rather than linking to external sites/code, whenever possible.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider expanding on this a bit and explaining how it achieves this.
- 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, remove external links in favor of summaries, inline code, etc.
|
||
## Test Cases | ||
|
||
Some basic tests can be found here https://github.com/Uniswap/uniswap-v2-core/blob/master/test/UniswapV2ERC20.spec.ts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these test cases be formatted in a language neutral way and inlined here rather than an external link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
../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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend inlining this, and consider stripping out all of the code unrelated to this specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
```solidity | ||
keccak256(hex"1901" | ||
++ keccak256( | ||
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
++ 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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>
* 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>
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. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 boolallowed
, setting approval to 0 oruint(-1)
. - the
deadline
argument is instead calledexpiry
. This is not just a syntactic change, as it effects the contents of the signed message. holder
is used instead ofowner
, similar todeadline
<>expiry
this will affect the contents of the signed message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An ERC for the
permit
token extension, as popularized by the Dai ERC20. Will update eip number anddiscussions-to
once I find out which PR this ends up being