-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
ERC1363 #3017
ERC1363 #3017
Conversation
AFAIK, we cannot go further with coverage... |
There are many EIPs for |
IMO, this "transfer and call" mechanism is really interesting, and some projects could really benefit from it. In particular, it can be used to avoid the approval + transferFrom workflow, which costs 2 sstores and 2 Approval events. AFAIK, there are not that many other options.
An I missing other standardization efforts? My understanding is that this feature can be used both
The second point would be great in the long run, but I'd say my main fear is that projects would need the first point and will either not use it, for lack of knowledge of this pattern (making their workflow overly expensive/complex) or use a non-standard version which will work for them in the short run, but make point 2 much harder to achieve. IMO, we need to push toward more standardization, and one way to do that is by providing a safe & effective implementation. Note: I've opened this discussion in sept 2020, and the discussion was not that rich :/ |
@lukehutch Your comments sound like you're angry about things. I recommend reviewing your approach to the conversation. This is a place for constructive discussion. |
@frangio edited, sorry. |
I honestly don't see anything in the EIP that strongly indicates What can be read as ambiguous is that the intention is to "execute code" on the receiver, and the wording that I think the EIP is the ultimate source of truth. @vittominacori's implementation isn't linked in the EIP so arguably it's not even a "reference" implementation, and it doesn't claim to be so in the repository (says it's "an implementation"). All that said, I'm slightly leaning towards requiring the receiver to be a contract, because I think the general expectation for a "call" is that it will actually execute code. The fact that |
@frangio my own implementation is not linked because of the reviewer said to remove external link to repo and test cases (also if I've seen many EIP referencing their own implementation). However maybe I missed explicit reference to EOA address but it was clear for me to refer only contracts by saying that "contracts MUST implement..." and that Will try to update the EIP as they say "A Final EIP exists in a state of finality and should only be updated to correct errata and add non-normative clarifications." in order to clarify this missing declaration. |
@frangio Exactly, the EIP does not mention EOAs at all, or even hint at what should be done for EOA recipients or spenders. And the statement that you found is what I was referring to. It is simply ambiguous about the EOA case (by omission), as I pointed out several times in the issue about deprecating ERC777. Because it is ambiguous, implementations should not all decide differently about what should be done in this case. I think @Amxx did the right thing by asking @vittominacori what his intention was in this case, and he made that intention very clear in the comment that I quoted (and the comment he posted seconds before I posted this one): he believes ERC20 APIs should be use to send to EOAs, since ERC1363 is a proper superset of ERC20 -- therefore, ERC1363 should revert if trying to send to EOAs. And I don't think the reference implementation is irrelevant, since it proves what @vittominacori's intent really was in the spec (even though the implementation is not linked from the spec -- it used to be, but had to be removed at the request of a reviewer). Note that ERC1363's reference implementation refusing to send to EOAs is different from ERC777 and ERC4524, both of which allow sending to EOAs through their ERC20-superset APIs, by actual definition in the spec. I think there is room for a sender notification API that refuses to send to EOAs.
Correct if "call" is passed a non-empty ABI encoding of a function call. I don't think it matters that |
In solidity, doing
Will be true if target is an EOA. Just like sending à transaction with any data field to an EOA will not revert. |
Correct, to prevent sending to EOA you have to do
(Although that also prevents sending to contracts whose constructors have not finished executing) |
I meant external function calls like |
Not true, if the target implements a fallback function. This is actually a huge potential security problem that affects hundreds of millions of dollars locked up in deployed contracts. https://media.dedaub.com/phantom-functions-and-the-billion-dollar-no-op-c56f062ae49f This can even affect ERC777, because there is no return value for the sender and recipient notification interface. ERC1363 and ERC4524 solve this by requiring their notification hooks to return their own function selector. |
How can a "not a contract" have a fallback function? When doing
|
|
Yes, so in order to be similar to any other OZ interface in #3525 I moved them into token folder and then linked in Also I created different mock files instead of having all into a single file like here https://github.com/OpenZeppelin/openzeppelin-contracts/blob/08dd234f57f0372260422ac85281a21d70adac50/contracts/mocks/ERC1363Mock.sol |
Again, I think 1363 should be just like 3156 or 4626, with the ERC interface in contracts/interfaces. I don't see why 1363 would be handled differently than 4626. The mock fonder is also messy. IMO it's worth putting all the 1363 related mocks in a single file. |
Hi guys, is there any work to be done to merge this PR? |
Reading the whole discussion here - regarding the adoption, it is like chicken-egg problem. People will start using that once the 1363 is available in the OZ wizard. I can see there are only foundry-tests blocking the merge now? |
We only do foundry tests when we want fuzzing. IMO fuzzing is not needed here. AFAIK, the blocker is a formal decision on what to do when targetting EOA. It seems that different people have different interpretations of the ERC document, and that the original idea of the ERC author might not match the ERC document. In that case I'd personnally follow the ERC document, but that is not resolved. |
Ah, in that case - since there is an ambiguity in understanding EOA detail in the ERC and as I understood OZ shall not be opinionated I propose there were both implementations. OZ can leave it up to the using developer to decide on a behavior towards EOAs. |
That is the best way to make sure people are confused, don't know which one to use (possibly don't understand the difference) ... In the end you'll just have no standard behavior, and no reliability on what to expect from a contract. Either we come to an agreement on what to provide or we don't provide anything (so far its been the later) |
I don't want to open the discussion again but above we agreed to revert to EOA receiver.
Yes, it doesn't say nothing about EOA addresses but the original purpose was to give a way to use transfer with a receiver that can make a callback. Why should someone want to call a method on a receiver and expecting it is done but it doesn't as it is an EOA? The Here my implementation https://github.com/vittominacori/erc1363-payable-token |
This comment was marked as duplicate.
This comment was marked as duplicate.
Yes I'm ok that the EIP could be confusing but for me (as author) the following concept is clear. If I call the If I use |
This comment was marked as duplicate.
This comment was marked as duplicate.
I'm seeing "optimistic" use cases were someone wants to send and give a change for the receiver to react. That is eactly the
This usecase can be achieved using your approach by doing if (target.code.length == 0) {
token.transfer(target, amount);
} else {
token.transferAndCall(target, amount);
} The thing is that with your approach On the other hand if we imagine that require(target.code.length > 0, "some error");
token.transferAndCall(target, amount); with basically no overhead. IMO not reverting on EOA is more efficient (because it remove check that people are free to do, or not do, before calling). It also matches We "often" see devs looking for a I believe a version of 1363 that doesn't revert on EOA (which IMO is what the document says, despite the author's intentions) is the best shot we have to addressing a long standing issue in the community ... when a version of 1363 that does revert on EOA would not be working for some (IMO important) usecases and would lead to this long standing issue not being fixed. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
What is the status of this ERC? Anything I can do to contribute to finalizing it? |
Once v5 will be ready, I will be happy to close this PR and open a fresh new one with the new guidelines. |
Replaced by #4631 |
Fixes #3736
ERC1363 is marked final
I think we should implement it. It would be an important move toward adoption of these mechanisms that can both improve the UX (only one tx needed instead of 2) and save a lot of gas during contract interaction (using approval added 2 sstore and 2 events: one when approval is set and one when transferFrom is called.)
PR Checklist