Add support for ERC721 token URI concatenation of base token URI and tokenID

General
1

Current ERC721 minting with token URI requires specifying the URI per token.

To reduce gas cost, the OpenSea example uses concatenation of a base token URI and the tokenID. (https://github.com/ProjectOpenSea/opensea-creatures/blob/master/contracts/TradeableERC721Token.sol)

I used this concatenation on BlockHorses. https://github.com/blockhorses/BlockHorses/blob/master/contracts/BlockHorses.sol
https://github.com/blockhorses/BlockHorses/blob/master/contracts/Strings.sol

  function tokenURI(uint256 tokenId) external view returns (string memory) {
    require(_exists(tokenId));
    return Strings.Concatenate(
      baseTokenURI(),
      Strings.UintToString(tokenId)
    );
  }
2 Likes
2

This is a fantastic idea! Thanks @abcoathup!

Potential API for this: an ERC721 extension ERC721DefaultURI with a base URI parameter, which overrides ERC721Metadata's tokenURI() with the concatenation. What I don’t like about this is that ERC721Metadata also has name and symbol and this gets into the unintuitive nastiness of multiple inheritance. We can avoid that by making the default URI extension only provide the URI, and not name and symbol, but then if people do combine it with the metadata extension the behavior would depend on the order of inheritance and that is nasty. I think this can be fixed by adding a common ancestor in the hierarchy, which sounds reasonable to me.

1 Like
3

Moving this from #support to #openzeppelin :slight_smile:

1 Like
4

Ohhh, nice. This would also mean finally developing some sort of string management library. @abcoathup what do you think about opening an initial PR with this String.concat library, so we can get started?

@frangio Hmm I see what you mean, a common ancestor with the URI field may be the way to go, but it introduces a lot of artificial complexity. We should experiment a bit with this.

As a side note, our revert reasons share a prefix: I wonder if there are gas gains to be had by generating the revert reason string on the fly, as opposed to having repeated copies of the prefix stored in the blockchain. My intuition is that the Strings code would offset any gains, but I’ve been wrong before.

1 Like
5

Will have a go on the weekend

6

@nventuro I have made an initial PR with the Strings library.

2 Likes
7

@abcoathup given @frangio's comment in the PR (quoting here)

Ohh that is very cool! I personally would consider removing concatenate altogether then. abi.encodePacked is superior in that it can receive a variable number of arguments.

I think you can start working on a PR for this using abi.encodePacked :slight_smile:

1 Like
8

Very excited that my first PR has been merged into OpenZeppelin!!
It is only small, but great to be contributing.

Definitely learnt more about testing and naming as part of this experience.

2 Likes
9

There is now a PR for this:

10

The current implementation of ERC721Metadata.sol in OpenZeppelin Contracts 3.0 Beta doesn’t support token URI concatenation of base token URI and tokenID. (https://github.com/OpenZeppelin/openzeppelin-contracts/issues/1745)

In a mint function, a token URI would need to be set.

Instead of returning an empty string, we could return the baseURI concatenated with the tokenId.

        return string(abi.encodePacked(_baseURI, Strings.fromUint256(tokenId)));

Alternatively, tokenURI could be made virtual to allow it to be overriden.