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

eip1363 implementation #3525

Conversation

vittominacori
Copy link
Contributor

ERC1363 is final and after we have had a discussion about it, the EIP cannot be updated (as you can view here) so it remains the availability for the token to interact also with EOA.

This PR uses the original behavior so transferAndCall, transferFromAndCall and approveAndCall revert if called on EOA.

I didn't suggested changes in #3017 because it is a completely different behavior so community and reviewers should be able to choose what implementation to use (the original intention or the one like 4524 safeTransfer).

I think that we should ask the community.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@frangio
Copy link
Contributor

frangio commented Jul 5, 2022

The implementation in #3017 already reverts if the receiver is not a contract. It's implicit in this line:

try IERC1363Receiver(to).onTransferReceived(_msgSender(), from, value, data) returns (bytes4 retval) {

Are there other differences in this PR?

Please see my comment in #3017 (comment) for some context as well.

@vittominacori
Copy link
Contributor Author

vittominacori commented Jul 5, 2022

@frangio

Basically I've created a structure in name and in folder hierarchy that I think follows the OZ guidelines for other contracts.
Also params name has been inherited from ERC20 to evidence the compatibility.

image

Here I use msg.sender instead of passing the owner because approveAndCall is called by owner directly and I think it is redundant to add another param.

I added detailed revert messages for each case.

I added my test cases as I've worked on them during years and they should have a 100% coverage.

@vittominacori
Copy link
Contributor Author

vittominacori commented Jul 5, 2022

@frangio I also added presets, utils and documentation draft

@vittominacori
Copy link
Contributor Author

@frangio are there any changes requested?

@frangio
Copy link
Contributor

frangio commented Jul 8, 2022

Given that there are two competing PRs for this same EIP, I need to take some time to review both and see what are the meaningful differences and how to proceed. I'm definitely interested in the test suite you're contributing, but I don't know that we will favor this Solidity implementation over the one that we had already been working on.

@frangio
Copy link
Contributor

frangio commented Jul 8, 2022

@vittominacori I'm proposing that we move forward with #3017.

Once that is merged (which looks straightforward enough), I would like you to contribute some of the things from this PR: tests (ideally without redundancy with the tests we already have in #3017), docs, and utils/ERC1363Holder. Is this ok?

Note that presets are deprecated so we shouldn't add a new one.

@vittominacori
Copy link
Contributor Author

@frangio so code structure will be the one in #3017?

I mean interface is written into interfaces folder instead of import like the others OZ interfaces. Mocks are all written in a unique file instead of having separation. Also erc1363 will be in erc20 extension folder?

It seems different from other codebase contracts.

Anyway if you prefer that structure I will try to add tests and utils later.

@frangio frangio mentioned this pull request Jul 14, 2022
3 tasks
@frangio
Copy link
Contributor

frangio commented Jul 14, 2022

Yes, we see ERC1363 as an ERC20 extension. I will raise the point about the location of the interface.

@Amxx
Copy link
Collaborator

Amxx commented Jul 15, 2022

IMO all standard (ERC) interfaces should be in contract/interfaces.

In particular, this is the case of ERC3156 & ERC4626. "older" interfaces are still in their respective folders, and linked into contract/interfaces.

I believe that in 5.0 we should move all the standard interfaces that are not yet there (ERC20, ERC721, ...) into the interfaces folder

@vittominacori
Copy link
Contributor Author

Ok you have a full overview on where OZ guideline are going.
They were my opinion as I've always used OZ code structure and it seems a bit different from others.
Let me know if I can help with something.

@vittominacori
Copy link
Contributor Author

vittominacori commented Nov 10, 2022

Hi @frangio @Amxx once v4.8.0 has been released could we consider discussing about merging this or #3017 again.

@vittominacori vittominacori mentioned this pull request Jan 24, 2023
3 tasks
@vittominacori vittominacori mentioned this pull request Sep 27, 2023
3 tasks
@vittominacori
Copy link
Contributor Author

I'm closing in favor of #4631.

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 this pull request may close these issues.

3 participants