-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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: 712-signed token approvals #2613
Comments
IMO the 'security considerations' section should mention the front running attack that can occur when an allowance is reduced, either by If you have a strong desire to do some bike shedding, you could even suggest a specific mitigation for it. 😝 |
I was looking forward to talking about this. I'd like to talk about the need for a "deadline" What aspects of Tx Relayer do we need to take into account "deadline"? The Tx Relayer may have a response that causes tx while setting tx fee low. or no response. And when a user signs for an interface, it requires more information. or more knowledge. Is usability not compromised? and more complexity for Tx Relayer. So I think the deadline is unnecessary. 🤔 |
I like |
Thanks @maurelian, added a comment about the approval front running attack. |
Actually @maurelian I was just made aware of the xDai staking token implementation of |
In terms of "Free Options," I think But in terms of EIP-712, deadline is not convincing. We can't show you the "Free Options" article every time ask for a signature. If we cannot effectively explain Furthermore, If all of these signatures necessary a "deadline" option, if all of EIP-712 signatures have the same security level, I think it's better to update the EIP-712 specification. 🙂 What is the best option? What do you think? |
I'm perfectly happy with
There are already interfaces which deal with deadline, uniswap being one of them: or, for I can agree with your point that it may a difficult point for users to understand, and one that may require some EIP-712 tweaking to improve. Another ux improving solution could also be to introduce I do think that these options fall outside the scope of this ERC though, especially since this in some sense more of a descriptive erc than a prescriptive erc; I am highlighting what has been implemented and the motivations behind it. We can certainly make tweaks in new versions, but I am mainly interested in documenting what already exists here. |
I've seen on twitter that apparently there are at least 3 different semantics contracts follow and still call it "permits". @MrChico you proposed to discuss all in the EIP/ERC. I think an EIP should be unambiguous and promote a single way of doing things. Is it possible to do that while perhaps mentioning "semi-compatible" contracts? If not, would it make sense having 3 different EIPs? |
@axic I'm planning on mentioning the Stake-flavored implementation under "backwards compatibility", making that, the dai and chai implementations all belong to this " semi-compatible" class, but keep the specification as is for specificity |
Nice, sounds good! |
@MrChico Good! I think we've written quite a meaningful conversation in publicly. It will be the data we or the next people will refer to when working in the future! |
Why not take the opportunity to fix this? Otherwise, we might end up with proliferations of safePermit too. Is the assumption that the allowances will be spent immediately, so it's not actually a problem? |
To be honest I don't find the approval race condition to be much of an issue in practice – it's only ever a problem when allowing addresses that can "spontaneously" spend your allowance. In most use cases, you give your allowance to a smart contract in such a way that only you can later initialize the I'm all for people implementing |
It wasn't clear to me from a cursory read of the EIP that the following getter is also part of the required interface, I'd make it more explicit: function nonces(address owner) external view returns (uint256); I'd also consider changing its name to something else, since |
Do you have a suggestion? |
Keep in mind that a change in this name would make this etc incompatible with Uniswap v2 (and even more incompatible with dai and chai) |
permitNonces |
Its okay for nonces to be generic, in some cases it can be an advantage. The erc doesn't specify other methods can't use |
But most of the time, this doesn't seem like a particularly important concern |
I think it would be useful to have a function that let smart contract know if the permit call will succeed if executed. This could be like :
|
This is interesting. But if they were called with staticcall they wouldn't be able to execute the permit later anyway? In the cases I can imagine, it seems like you'd want to submit the permit whenever if it is well formed so it seems a try-catch pattern would be sufficient for this use case. Or is there some scenario you can think of in which you would NOT want to submit a well-formed permit? |
Hmmm, I guess one could imagine a sort of |
Yes, that's basically it. |
@wighawag couldn't you just as well check the validity of the permit in the "paymaster" in that case? As per the spec, you need to validate:
all of which can be verified in a static environment. |
Although the |
I think this EIP can do without the |
@MrChico @nventuro That's exactly what I do as a workaround, see here : https://github.com/wighawag/gsn-playground/blob/5f40cc949031129374a48e43c8a0b71b83e52bf1/contracts/src/DAIPaymaster.sol#L101-L119 but this does not scale if my paymaster wanted to support all token that support the permit standard (unless A DOMAIN_SEPARATOR view was also part of the standard), hence why in my opinion a function like By the way in regard to EIP-712 domain. I am not sure this should be part of the standard. EIP-2612 Contract implementer should be free to set the EIP-712 domain they see fit. |
Amazing - EIP-5267 will solve one of the biggest pain points around using EIP-712: "how in the world do I generate the domain separator?". Thank you for all the great work! As an aside, I'd love to spec out a new reference impl of WETH9 for new EVM chains and L2s such that they can support |
Proposing to move EIP-2612 to Last Call with deadline September 6th 2022. #5506 |
permit
: 712-signed token approvals
This EIP is now in Last Call until October 8th. |
What about EIP-2098 (Compact Signatures Representation)? |
@k06a What is your proposal? |
Let’s store |
In my opinion we should consider EIP-2612 frozen and start working on an improved EIP that extends this one, as suggested by @phil-ociraptor above. Note that using EIP-2098 is at odds with EIP-1271 which is another thing that has been proposed as a needed improvement to permits. |
why? In Eip-4973 we use both 1271 any 2098 and I didn't notice anything at odds |
In EIP-1271 the signature has no specified semantics, it should be seen as a blob, so it's not correct to apply the EIP-2098 encoding to it since it's specific to ECDSA. Unless perhaps it's only applied as an optimization for the EOA case? I need to think about that more. |
Cross linking to EIP-4973 on Magicians here for reference: https://ethereum-magicians.org/t/eip-4973-account-bound-tokens/8825/146?u=timdaub |
EIP-2612 need not be considered frozen. You can always change the spec until it's final (as was done with EIP-712). |
That is true in theory, but in practice making normative changes to an EIP that's been widely deployed can be very problematic so I think it should be avoided. |
Fair enough. It should be pushed to final regardless, though. |
I think we should go full no-mercy here. If ppl implemented an EIP in non-final status that's their fault and I don't think we should give into "oh but they have implemented it already." We're EIP editors and our legitimacy comes from executing EIP-1 to the best of our capabilities. I know this will make some enemies but I believe it sends the right signal to implementers to make breaking changes to EIP-2612. Or alternatively move EIP-2612 into withdrawn unless someone steps up and moves it into final. And then create a new EIP with the breaking changes. |
This is exactly what was proposed and what is happening. I opened the PR to move to Final yesterday: #5782 |
any reason (beside the CI error of missing ethereum-magicians.org thread) this is not Final now? |
This has reached Final. |
Hi! i might be late, but i want to propose to add: The rationale behind is that there are right know 2 typical ways of creating the DOMAIN_TYPEHASH:
With the current standard view functions, a front-end cannot know which type of |
@invocamanman The domain hashes are not enough. Please see EIP-5267 for a proposal to tackle this problem. |
It's true, i was proposing a fix for a current common situation, but this solution has more general purpose and seems better, thanks for sharing! |
@lightclient if you meant to unlock, fixed. |
This EIP is now final. Discussions about final EIPs should be on https://ethereum-magicians.org/
This is a place for discussing #2612, which proposes an additional method
permit
for making approvals by way of signed messages rather than direct transactionsThe text was updated successfully, but these errors were encountered: