Several issues with ERC721 standard

[wighawag]

The current implementation do not respect ERC721 standard

event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId); This event should always be emitted, regardless of how the actual transfer of balance happen.

In the current implementation it is only emitted when using the ERC721 transfers methods (the one who do not specify quantity)

It should be emitted also when doing ERC721x transfers, including Batch transfers

Other potential issues

In the current implementation

• balanceOf(owner) return the number of token, including FT Will that not be confusing for ERC721 compatible wallets, tools... when then get hang of the FT id and request the owner ?

• event TransferToken is emiited even for NFT when quantity is 1 Should it not be only emitted for FT ? Should it also be emitted for FT when doing batchTransfer? or do we consider TransferToken and BatchTransfer together

Regarding naming, I suggest calling it : TransferWithQuantity or at least something less generic than TransferToken

There also few function mentioned on github as part of the interface but not implemented

• tokenName(tokenId) Is that one necessary at all since there is tokenURI(tokenId) ?

• individualSupply(tokenId) should this throw upon calling it on a NFT or simply return 1 ?

Lastly this one is implemented but return 0

• tokenOfOwnerByIndex Would that not confuse wallet and tools supporting ERC721 Enumerable extension ?

[gakonst]

Thank you for all the issues you have found in the implementation :)

event Transfer(address indexed _from, address indexed _to, uint256 indexed _tokenId); This event should always be emitted, regardless of how the actual transfer of balance happen. In the current implementation it is only emitted when using the ERC721 transfers methods (the one who do not specify quantity) It should be emitted also when doing ERC721x transfers, including Batch transfers

I'm not sure I agree here, ERC721 Transfer events should be only regarding NFTs in the contract

event TransferToken is emiited even for NFT when quantity is 1. Should it not be only emitted for FT ?

Fixed in a previous commit, _transferFrom with quantity is now only callable on FTs.

Agree with the TransferWithQuantity, changed. When doing a batch transfer we just a emit a batchTransfer event for all arguments together, firing a Transfer/TransferWithQuantity wouldn't be feasible, as it's 2k extra gas per emission.

Regarding tokenName and individualSupply, the README was outdated, updated now.

tokenOfOwnerByIndex Would that not confuse wallet and tools supporting ERC721 Enumerable extension

Agree, however adding the enumerable extension removes any gains introduced by batch transfers. IMHO the enumerable extension is not necessary and with proper event indexing the data should be retrievable.

[wighawag]

ERC721 Transfer events should be only regarding NFTs in the contract

Sorry if it was not clear but that is exactly my thoughts. I was referring to ERC721 NFT only. As it stands, the Transfer event is not triggered when such NFT is transfered to another owner if the method used is different than the ERC721 transfer function (the one that does not specify quantity) More precisely:

• when calling function transfer(address to, uint256 tokenId, uint256 quantity) and related functions with a tokenId referring to an NFT the ERC721 Transfer event is not emitted

• Similarly when calling function safeBatchTransferFrom(address _from, address _to, uint256[] tokenIds, uint256[] _amounts, bytes _data) public; with one or more tokenIds referring to NFTs the ERC721 Transfer event is not emitted for any of them.

This means the contract is not ERC721 compliant.

When doing a batch transfer we just a emit a batchTransfer event for all arguments together, firing a Transfer/TransferWithQuantity wouldn't be feasible, as it's 2k extra gas per emission.

I understand (though you just need Transfer events to be compliant and not the TransferWithQuantity) but if the Transfer event is not sent for NFT transfer, the Contract will not be implementing ERC721, by definition.

What this means, is that if an application supports ERC721 only (no enumeration extension), there would be no way for that application to keep track of ownership (since Transfer event is not emitted for all case) of ERC721x's NFTs

Agree, however adding the enumerable extension removes any gains introduced by batch transfers. IMHO the enumerable extension is not necessary ...

Would it not better to remove the function entirely and not try to be compliant with the enumerable extension then? Do you plan to add ERC165 supportInterface function (you would need to be ERC721 compliant) ? If so you do not need to return true for 0x780e9d63 (ERC721Enumerable)

... and with proper event indexing the data should be retrievable.

As mentioned above, if the ERC721 Transfer event is not emitted for all NFT transfer (regardless of the method invoked), an application supporting ERC721 only would not be able to use events to track the data

[gakonst]

Emitting a Transfer event now during batch transfers if the token is a NFT in https://github.com/loomnetwork/erc721x/commit/265c3368425cd3d75f0bf5618123e9f7ee249b38. Will consider removing the Enumerable extension