Openzeppelin-contracts: ERC-721 tokens / note about ERC-165

[fulldecent]

Please add a note to the ERC-721 implementation that it is incomplete and should not yet be used.

At present, the OpenZeppelin implementation does not support ERC-165, which is required by the ERC standard.

[DanielRX]

Is this not added? this implies it is a full implementation yet it's missing this crucial bit?

[jamesmorgan]

I have added ERC165 to a project I am working with which uses the latest open zeppelin ERC721, this is what I ended up with:

  bytes4 constant InterfaceSignature_ERC165 =
    bytes4(keccak256('supportsInterface(bytes4)'));

  bytes4 constant InterfaceSignature_ERC721Enumerable =
    bytes4(keccak256('totalSupply()')) ^
    bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^
    bytes4(keccak256('tokenByIndex(uint256)'));

  bytes4 constant InterfaceSignature_ERC721Metadata =
    bytes4(keccak256('name()')) ^
    bytes4(keccak256('symbol()')) ^
    bytes4(keccak256('tokenURI(uint256)'));

  bytes4 constant InterfaceSignature_ERC721 =
    bytes4(keccak256('balanceOf(address)')) ^
    bytes4(keccak256('ownerOf(uint256)')) ^
    bytes4(keccak256('exists(uint256)')) ^
    bytes4(keccak256('approve(address,uint256)')) ^
    bytes4(keccak256('getApproved(uint256)')) ^
    bytes4(keccak256('setApprovalForAll(address,bool)')) ^
    bytes4(keccak256('isApprovedForAll(address,address)')) ^
    bytes4(keccak256('transferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'));

  /**
   * @notice Introspection interface as per ERC-165 (https://github.com/ethereum/EIPs/issues/165).
   * @dev Returns true for any standardized interfaces implemented by this contract.
   * @param _interfaceID bytes4 the interface to check for
   * @return true for any standardized interfaces implemented by this contract.
   */
  function supportsInterface(bytes4 _interfaceID) external view returns (bool)
  {
    return ((_interfaceID == InterfaceSignature_ERC165)
      || (_interfaceID == InterfaceSignature_ERC721)
      || (_interfaceID == InterfaceSignature_ERC721Enumerable)
      || (_interfaceID == InterfaceSignature_ERC721Metadata));
  }

Not entirely sure if this should cover it?

[DanielRX]

@jamesmorgan would it not make more sense to have the derivation in comments and the final value as a constant static string?

So

  bytes4 constant InterfaceSignature_ERC721 = 0xcff9d6b4;
  /*
    bytes4(keccak256('balanceOf(address)')) ^
    bytes4(keccak256('ownerOf(uint256)')) ^
    bytes4(keccak256('exists(uint256)')) ^
    bytes4(keccak256('approve(address,uint256)')) ^
    bytes4(keccak256('getApproved(uint256)')) ^
    bytes4(keccak256('setApprovalForAll(address,bool)')) ^
    bytes4(keccak256('isApprovedForAll(address,address)')) ^
    bytes4(keccak256('transferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'));
  */

It saves a little on the gas for deployment as you don't need to bake in those lines.

[jamesmorgan]

@DanielRX yes this makes sense, I was more demonstrating my first pass at a solution. Not entirely sure of the cost saving but it's probably worth doing.

This is what it looks like using statics:

  bytes4 constant InterfaceSignature_ERC165 = 0x01ffc9a7;
    /*
    bytes4(keccak256('supportsInterface(bytes4)'));
    */

  bytes4 constant InterfaceSignature_ERC721Enumerable = 0x780e9d63;
    /*
    bytes4(keccak256('totalSupply()')) ^
    bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^
    bytes4(keccak256('tokenByIndex(uint256)'));
    */

  bytes4 constant InterfaceSignature_ERC721Metadata = 0x5b5e139f;
    /*
    bytes4(keccak256('name()')) ^
    bytes4(keccak256('symbol()')) ^
    bytes4(keccak256('tokenURI(uint256)'));
    */

  bytes4 constant InterfaceSignature_ERC721 = 0x80ac58cd;
    /*
    bytes4(keccak256('balanceOf(address)')) ^
    bytes4(keccak256('ownerOf(uint256)')) ^
    bytes4(keccak256('approve(address,uint256)')) ^
    bytes4(keccak256('getApproved(uint256)')) ^
    bytes4(keccak256('setApprovalForAll(address,bool)')) ^
    bytes4(keccak256('isApprovedForAll(address,address)')) ^
    bytes4(keccak256('transferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^
    bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)'));
    */

  bytes4 public constant InterfaceSignature_ERC721Optional =- 0x4f558e79;
    /*
    bytes4(keccak256('exists(uint256)'));
    */

[fulldecent]

@jamesmorgan The official signature for ERC721 is 0x80ac58cd as documented in http://eips.ethereum.org/EIPS/eip-721.

Please report TRUE for supportsInterface on 0x80ac58cd. This is the interface ID that currently-deployed wallets and applications are using.

---

If you will advertise availability of the exists(uint256) function (which is a non-standard), then you are welcome to do that by having your other interface signature return TRUE in addition to 0x80ac58cd returning TRUE.

[jamesmorgan]

@fulldecent updated comment above to reflect this 👍

[emkman]

@fulldecent what is the purpose of InterfaceSignature_ERC721Optional =- 0x4f558e79;? Was exists(uint256) in a draft and then removed from the standard? I can't find it documented anywhere.

[fulldecent]

@emkman exists(uint256) was never in the draft or standard. OZ decided to include this function as a proprietary extension to the standard.

FYI, proprietary extensions are good. If something provides utility to you, then use it!

[frangio]

"Proprietary" is not the right word in this context. It's just an extension to the standard.