eip1363 implementation

[vittominacori]

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

• [x] Tests
• [x] Documentation
• [ ] Changelog entry

[frangio]

The implementation in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3017 already reverts if the receiver is not a contract. It's implicit in this line:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/a3ef6b445ab5bb8c5d3bf2e6073708ce503bea0a/contracts/token/ERC20/extensions/ERC1363.sol#L105

Are there other differences in this PR?

Please see my comment in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3017#issuecomment-1174534533 for some context as well.

[vittominacori]

@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.

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]

@frangio I also added presets, utils and documentation draft

[vittominacori]

@frangio are there any changes requested?

[frangio]

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]

@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]

@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]

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

[Amxx]

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]

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]

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

[vittominacori]

I'm closing in favor of #4631.