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

EIP-2612: Add ERC20 permit() function #2206

Closed
nventuro opened this issue Apr 20, 2020 · 13 comments · Fixed by #2237
Closed

EIP-2612: Add ERC20 permit() function #2206

nventuro opened this issue Apr 20, 2020 · 13 comments · Fixed by #2237

Comments

@nventuro
Copy link
Contributor

nventuro commented Apr 20, 2020

There's been some discussion about an ERC20 extension with the permit function, popularized by MakerDAO's Dai token (see the code here).

This recently became even more relevant in light of hacks revolving ERC777 and reentrancy, since permit could help solve some of the UX issues that drive people to using ERC777 improperly. It looks like a proper ERC might be in the works: we should make sure we're compliant if that ends up happening. In all likelihood the spec will be identical to the Dai code, however.

Link to EIP-2612

@miohtama
Copy link
Contributor

I agree that permit() needs its own ERC.

However, the ERC-777 solves UX issues better than permit(). permit() requires all wallets to have a special flow to sign + send. In ERC-777 we can just send() tokens to the smart contract. ERC-777 t is semantically more correct and also from the user perspective much easier to understand.

I think it is a separate discussion if people are using ERC-777 improperly or not, and maybe we should not go in there on Github.

@miohtama
Copy link
Contributor

For the underlying issue of re-entrance:

  • Have Solidity to prevent re-entrance by defaut (like we changed with payable in the past)

  • Have EVM level change that prevents re-entrance by default

Currently the reason why these hacks happen are "bad defaults". Re-entrance should be opt-in, not opt-out.

@nventuro
Copy link
Contributor Author

Yes, I wasn't suggesting ERC777 should be removed or avoided (on the contrary, I think it opens the door to many interesting possibilities), but it does require a deeper understanding of the EVM and the Ethereum ecosystem, partly due to the 'bad defaults' you've mentioned.

In that sense, permit would let people get a better UX than plain ERC20 offers without having to worry about the dangers of misusing it.

@abcoathup
Copy link
Contributor

EIP: ethereum/EIPs#2613

@mds1
Copy link

mds1 commented Apr 25, 2020

In all likelihood the spec will be identical to the Dai code, however.

I don't think this is necessarily true. There's two potential improvements I've seen discussed, which I'm sure you're aware of, but figured I'd mention anyway:

  • Dai/Chai's permit function has a boolean input allowed. If true, the allowance is set to uint256(-1), and set to 0 otherwise. Uniswap V2's permit function instead has uint256 value as the input, allowing more granular control over the approval amount, similar to a standard approve call.

  • Dai/Chai's permit function takes a uint256 expiry input, which is how long the signature is valid for. The ERC references an xDai staking token with an alternative use, where expiry instead refers to how long the allowance is valid for, which helps prevent allowances from lasting indefinitely

Regardless of the spec decided upon, I still think it's valuable addition

@nventuro
Copy link
Contributor Author

nventuro commented May 8, 2020

Thanks for mentioning those improvements @mds1! The ERC is very detailed - I encourage people interested in this to go read it.

The ERC references an xDai staking token with an alternative use, where expiry instead refers to how long the allowance is valid for, which helps prevent allowances from lasting indefinitely

That implementation has permit with a boolean argument, which greatly simplifies working with expiry: we'd otherwise need to e.g. track how much allowance each permit granted, and then remove that. I'm not sure permit(uint256 value, uint256 expiry) would work.

Both permit(bool, uint256 expiry and permit(uint256 value, uint256 deadline) make sense to me, but in the interest of reducing fragmentation I think it'd be probably better if we stick to a single one (whatever the EIP goes with). Should we move forward with an implementation before the EIP is finalized however, or wait for it to mature a little?

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

@Ro5s
Copy link
Contributor

Ro5s commented Oct 12, 2020

Yes, I wasn't suggesting ERC777 should be removed or avoided (on the contrary, I think it opens the door to many interesting possibilities), but it does require a deeper understanding of the EVM and the Ethereum ecosystem, partly due to the 'bad defaults' you've mentioned.

In that sense, permit would let people get a better UX than plain ERC20 offers without having to worry about the dangers of misusing it.

@nventuro what are your thoughts on combining ERC777 features (like transferAndCall-type deposits) and permit() signature txs? Does having both add value to an ERC-20 token?

@nventuro
Copy link
Contributor Author

I'd say it depends on what you intend to use the token for. For 'regular' ERC20 usage (e.g. exchange, participation in DeFi platforms), the bare interface is enough since there's no feature discovery.

@Ro5s
Copy link
Contributor

Ro5s commented Oct 13, 2020

Thanks for response @nventuro. Here, wondering how an updated WETH contract might use both permit() to establish more gasless transactions, as well erc677 to reduce number of txs, metamask popups: WETH10/WETH10#19

@alfredolopez80
Copy link

Another focus of the problem may be this new method of open zeppelin can help
https://docs.openzeppelin.com/contracts/4.x/api/utils#SignatureChecker

@abcoathup @nventuro

@frangio
Copy link
Contributor

frangio commented May 20, 2021

@alfredolopez80 Please ask in the forum. https://forum.openzeppelin.com

@alfredolopez80
Copy link

@alfredolopez80 Please ask in the forum. https://forum.openzeppelin.com

Sure i will do today

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 a pull request may close this issue.

7 participants