Openzeppelin-contracts: [Help Wanted][ERC721] Question about deprecated function `tokensOf`

[canhlinh]

Hi Team,

Sorry for bothering you guys
I saw that the function below is already deprecated. Could you please let me know the reason why this function was deprecated ? High gas fee or any security issue.
Should i use this function in production ?

Thank in advance.

 /**
 * @dev Gets the list of tokens owned by a given address
 * @param _owner address to query the tokens of
 * @return uint256[] representing the list of tokens owned by the passed address
 */
function tokensOf(address _owner) public view returns (uint256[]) {
   return ownedTokens[_owner];
}

Deprecated by this commit https://github.com/OpenZeppelin/openzeppelin-solidity/commit/e96164feea96bde033105e7dafd6c63164e7a021#diff-bc9f12d1d1e8ef6bdba9bc4d73ae8329

Now this function is declared as interface for compatibility with previously deployed contracts
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC721/DeprecatedERC721.sol

[hadv]

More details here:

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md

[canhlinh]

@hadv Maybe you are misunderstand what i said ?

[hadv]

nope, I mean you can research yourself why they remove this function on the EIP document ;)

[canhlinh]

@hadv Sorry, I didn't find any document that related to this one.

[canhlinh]

@hadv Actually, I've searched it from the history commit of this file https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md

But i did't see any information.

[hadv]

Okay, so it's not the standard method of ERC-721 token

[AndreiD]

ok, and this guy canhlinh, and so do I, ask why it was deprecated. I'm also interested in knowing this. Hadv if you don't want to provide a constructive answer why not go spend your time on something else

[canhlinh]

@AndreiD I think what he said is correctly.

[AndreiD]

so why that function was removed?

[canhlinh]

@AndreiD Because it's not the standard method of ERC-721 token. The team just build functions from the standard, then remove the non-standard method

[frangio]

@AndreiD @canhlinh I looked for discussion online about this but unfortunately couldn't find any. I can tell you my understanding of why tokensOf was removed. Working on arrays of arbitrary length on the blockchain can result in a security hole. A loop on a big array can cost more than the block gas limit, and this can turn a contract unusable, cause loss of funds, etc. Having a tokensOf function that returns an arbitrary length array was begging for people to make this mistake, so it was removed. Unfortunately, the tokenOfOwnerByIndex it was replaced with has its own problems.

Perhaps @fulldecent can share some of his thoughts on the issue?

[fulldecent]

To original poster @canhlinh. You can read more history on the process of ERC-721 standardization at https://github.com/ethereum/EIPs/pull/841. The original 721 was forked (to PR 841) -- then after a long process everybody (including the original 721 author) agreed on the standard and the number 721 was used for clarity.

Hey @frangio, yes you are 100% correct. It is not best practice to return arbitrary length arrays from functions. There is an exception to this rule (if the code will NEVER be called from another contract) but the exception cannot be enforced in code so it would surely lead to problems.

I am not aware of any complaints against tokenOfOwnerByIndex. It is an opt-in extension which costs more gas to use and it is scaleable. Scaleable means I have tested it with contracts having an "XKCD 865" number of tokens.

[frangio]

@fulldecent The problem I see with tokenOfOwnerByIndex is the multiple calls needed to retrieve all of the tokens of an owner, mainly how this is not atomic, given that the array could change arbitrarily in the middle of the process. How do you deal with this?

It's not clear to me exactly what procedure clients should follow to display the full list. Either using the events only, or tokenOfOwnerByIndex combined with something to guarantee atomicity, or something else that I'm not aware of. Is there a standard recommendation?

[fulldecent]

Good question. If you are calling from on-chain, of course you can atomically get the list by iterating.

But if you are off-chain then you are correct -- the requests could run against different blocks and you do not get atomic data. The solution is to run your queries all against the same block number. In web3js you can choose which block to run your query against. Just get the latest block number and then run your iterative query against that block. Guaranteed atomic!

[frangio]

Cool! I wonder if querying against a specific block number works on Infura, but it's a great start nonetheless.

@AndreiD @canhlinh I hope this has answered your questions. Thanks @fulldecent for helping out!

[hadv]

Good to know this! Thanks @frangio & @fulldecent

[canhlinh]

🚀 Thank @frangio and @fulldecent