Expose tokensOfOwner method on ERC721Enumerable

Support
1

Hello,

I was wondering why the ERC721Enumerable smart contract only exposes a tokenOfOwnerByIndex which returns only one of the owned token and doesn’t expose a tokensOfOwner which would return _ownedTokens[owner]. I guess there is a good reason for that but can you shed some light?

I was maybe thinking exposing a tokensOfOwner method in our smart contract.

Thank you
Pierre

2 Likes
2

Hi @piedup

The ERC721 standard - optional enumeration extension, only has tokenOfOwnerByIndex (as well as totalSupply and tokenByIndex) eip-721

Which is why I assume the OpenZeppelin implementation only implements these functions. IERC721Enumerable.sol

I don’t know if there is a good reason for you not to expose a tokensOfOwner in your contract.
Hopefully someone in the community can advise.

Update: Thanks @nventuro
Returning an array doesn’t scale, see PR comment

This had been raised previously https://github.com/OpenZeppelin/openzeppelin-solidity/issues/1512 and there is now an internal getter in ERC721Enumerable.sol which you could then create a public getter. Though you should make yourself aware of the scaling issue.

    /**
     * @dev Gets the list of token IDs of the requested owner.
     * @param owner address owning the tokens
     * @return uint256[] List of token IDs owned by the requested address
     */
    function _tokensOfOwner(address owner) internal view returns (uint256[] storage) {
        return _ownedTokens[owner];
    }

The use is shown in ERC721FullMock.sol

    function tokensOfOwner(address owner) public view returns (uint256[] memory) {
        return _tokensOfOwner(owner);
    }
3

The reason behind this is that returning an array is a bad idea, since that will eventually stop working once the array works. Read the discussion here for more information regarding the issue and alternatives.

1 Like
4

@nventuro Based on the PR comment that returning an array doesn’t scale, should ERC721Enumerable.sol have the internal getter _tokensOfOwner at all or have some comments on it’s use?

5

Hm, perhaps we should add a comment briefly explaining this. The function is ‘safe’ to use from Solidity (as safe as any array is), the core issue described in that PR is sending arrays over the RPC interface.

2 Likes
6

Thanks for the insight. I guess then we could just implement some kind of pagination in our ERC721 contract.

2 Likes
7

@piedup pagination is a great idea! It has been discussed multiple times, and returning a slice of the array by passing two indexes (start and end) is IMO the most flexible way to handle this, while still retaining performance.

1 Like
8

I was thinking of doing something like this. Does it make sense?

// Pagination of owner tokens
function tokens(address _owner, uint8 _page, uint8 _rows) public view returns(uint256[] memory) {
    require(_page > 0, "_page should be greater than 0");
    require(_rows > 0, "_rows should be greater than 0");

    uint256 _tokenCount = balanceOf(_owner);
    uint256 _offset = (_page - 1) * _rows;
    uint256 _range = _offset > _tokenCount ? 0 : min(_tokenCount - _offset, _rows);

    uint256[] memory _tokens = new uint256[](_range);
    for (uint256 i = 0; i < _range; i++) {
        _tokens[i] = tokenOfOwnerByIndex(_owner, _offset + i);
    }
    return _tokens;
}

function min(uint256 a, uint256 b) private pure returns (uint256) {
    return a > b ? b : a;
}
1 Like
9

Hi @piedup

It would be great to get @nventuro’s thoughts (especially on my feedback :smile:)

I don’t know if it would be better to use the internal function _tokensOfOwner to get all the owners tokens and filter on that, rather than calling the public function tokenOfOwnerByIndex for each token which has a require check?

I was wondering if returning an empty array might be better than reverting for zero page or row parameters?

Perhaps using index rather than i?

I am getting used to the OpenZeppelin code style e.g. Parameters must not be prefixed with an underscore. It is worth having a look at if your team don’t yet have a code style guide (plus linting of Solidity if you aren’t already).

OpenZeppelin includes a Math library with a min function that you could use.

10

Hi @abcoathup. Thanks for all the suggestions. You’re right I should use the internal function instead of the public one.

Is it better to avoid reverting when you can? I guess in that situation it’s ok to return an empty array as you suggest.

I’ll look at the code style guidelines.

Thanks!

1 Like
11

I like that implementation! Not sure why you are using 1-bssed indexing though (making page zero invalid): it’d be easier and more consistent to allow page zero and remove the - 1.

@abcoathup comment regarding avoiding having the same statement being executed multiple times makes a lot of sense. There are multiple ways to achieve this: filtering out results over a storage pointer (what _tokensOfOwner returns) could make sense.

I’m thinking it may be worthwhile to go for a more generic approach and add this as a function to the Arrays library, perhaps something like viewPaginated. So you’d be able to do something like:

function tokensOfOwner(address owner, uint256 page, uin256 pageSize) public view returns (uint256[] memory) {
  return _tokensOfOwner(owner).viewPaginated(page, pageSize);
}

The issue with this this is, once again, generics: we’d have to implement viewPaginated for uint256, address, etc., since there’s no language support for such a thing. This has been holding us back for too long IMO, we could get started on some custom templating that auto-generates these as required. cc @frangio

1 Like
12

Yes, definitely. Opened https://github.com/OpenZeppelin/openzeppelin-solidity/issues/1806.

Pagination is something I've wanted to see in smart contracts for a long time!! It makes a lot of sense if you think of smart contracts as queries into a database.

It would be awesome to have a generic pagination mechanism, but unfortunately not possible with today's Solidity.

1 Like