ERC721 safeTransferFrom description

[MoMannn]

In the ERC721 specification for safeTransferFrom method the @devdescription is written as:

Throws unless msg.sender is the current owner, an authorized
/// operator, or the approved address for this NFT. Throws if _from is
/// not the current owner. Throws if _to is the zero address. Throws if
/// _tokenId is not a valid NFT. When transfer is complete, this function
/// checks if _to is a smart contract (code size > 0). If so, it calls
/// onERC721Received on _to and throws if the return value is not
/// bytes4(keccak256("onERC721Received(address,address,uint256,bytes)")).

The way to check for if _to is a smart contract can result in a false result if the smart contract is in the process of deploying . Since EIP-1052 there is a better way to check if an address is a smart contract (with codehash). I propose the description gets updated to reflect this or change the wording that this is clear.

The 0xcert and OpenZeppelin implementations already addressed this.

References:

• Address utils nibbstack/erc721#229
• https://www.reddit.com/r/ethereum/comments/916xni/how_to_pwn_fomo3d_a_beginners_guide
• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol

[fulldecent]

Here the standard includes the actual implementation detail ("code size > 0") directly in the documentation. (Why did they do that!?) So the change will not be compatible.

There's two ways to do this:

1. New EIP to mention this small change (which is only compatible on EIP-1052 networks)
2. Update 721 but include an errata or notes section

We have not had a problem with changing EIPs in the past if the change is non-normative (i.e. does not change the prescription) (*cough* 165 *cough*) but here it does. I would actually support a new EIP for this. It's brief but it's definitely a new thing.

[fulldecent]

Thanks for the chat with @frangio who helped me realize this change is technically still compliant with the literal wording of the standard. (Programmers are lawyers!) Therefore an amendment or new EIP is not necessary.

Right now we have a comment on the main thread and test cases are requested in the reference implementation. Sent them to you @MoMannn! But also appreciate if OpenZeppelin team could help too.

Perhaps after test cases are merged (with OZ review) then we could consider this issue closed.

[MoMannn]

@fulldecent Will jump on it when I find the time :)

[github-actions]

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[github-actions]

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.