feat: add baseTokenURI to ERC721Metadata

[nachomazzara]

Fixes #1745

• Add a private stringbaseURI variable.
• Functions to set and expose baseURI.
• Concatenate each tokenURI the baseURI
• tokenURI() from external to public.
• New _tokenURI() function to expose the tokenURI isolated without the base.

Notes: I used _baseURI as private and _baseTokenURI as internal to not change the existing ERC721Metadata interface.

[nventuro]

Neat, thanks @nachomazzara! I wonder if we should add this to the base ERC721Metadata contract, or create a new one. Having it all in the same place seems like the way to go. @frangio?

Also, could you please add a changelog entry under 2.5? Thanks!

[frangio]

I think we should add this feature as a new contract. The proposed name in #1745 was ERC721DefaultURI, but there is room to propose alternatives.

Edit: I was talking about a different feature, see below.

[frangio]

Actually, the approach in this PR is quite different from the one proposed in #1745. It doesn't use tokenId to form the full URI, but a string that has to be assigned to each token separately. It seems that the only benefit of this over the current ERC721Metadata is that it is cheaper to store on chain (edit: also that all the token URIs can be mass-updated in one transaction). This honestly seems quite reasonable and I would be happy to add it as a backwards-compatible change to ERC721Metadata.

But I still think we should implement ERC721DefaultURI as proposed in #1745. The idea was to define a base URI which is then concatenated with each tokenId, so the tokenURI becomes completely automatic. The server or storage endpoint at the base URI can determine off-chain the assets needed from the tokenId alone.

Have you considered this approach and discarded it? If so, please share more details about that!

[abcoathup]

A contract like the following could be added to the PR and cover the functionality of #1745.

ERC721DefaultURI.sol

pragma solidity ^0.5.0;

import "./ERC721.sol";
import "./ERC721Metadata.sol";
import "./ERC721Enumerable.sol";
import "../../access/roles/MinterRole.sol";
import "../../drafts/Strings.sol";

/**
 * @title ERC721DefaultURI
 * @dev ERC721 minting logic with a default URI.
 */
contract ERC721DefaultURI is ERC721, ERC721Metadata, ERC721Enumerable, MinterRole {

    /**
     * @dev Constructor function
     */
    constructor (string memory name, string memory symbol, string memory uri) public ERC721Metadata(name, symbol) {
        _setBaseTokenURI(uri);
    }

    /**
     * @dev Function to mint tokens
     * @param to The address that will receive the minted tokens.
     * @return A boolean that indicates if the operation was successful.
     */
    function mint(address to) public onlyMinter returns (bool) {
        uint256 tokenId = totalSupply().add(1);
        _mint(to, tokenId);
        return true;
    }

    /**
     * @dev Returns an URI for a given token ID.
     * Throws if the token ID does not exist. May return an empty string.
     * @param tokenId uint256 ID of the token to query
     */
    function tokenURI(uint256 tokenId) public view returns (string memory) {
        require(_exists(tokenId), "ERC721DefaultURI: URI query for nonexistent token");
        return string(abi.encodePacked(_baseTokenURI(), Strings.fromUint256(tokenId)));
    }
}

[nachomazzara]

Thanks for the review!!

But I still think we should implement ERC721DefaultURI as proposed in #1745. The idea was to define a base URI which is then concatenated with each tokenId, so the tokenURI becomes completely automatic. The server or storage endpoint at the base URI can determine off-chain the assets needed from the tokenId alone.
Have you considered this approach and discarded it? If so, please share more details about that!

Yes, I did. My idea is to add flexibility, re-usability and reduce gas costs while keeping the ERC721Metadata standard.

Maybe I used #1745 wrong misunderstanding the main goal of it.
My proposal is to abstract it one layer letting everyone re-implement tokenURI and use the approach described here by @abcoathup:

 /**
     * @dev Returns an URI for a given token ID.
     * Throws if the token ID does not exist. May return an empty string.
     * @param tokenId uint256 ID of the token to query
     */
    function tokenURI(uint256 tokenId) public view returns (string memory) {
        require(_exists(tokenId), "ERC721DefaultURI: URI query for nonexistent token");
        return string(abi.encodePacked(_baseTokenURI(), Strings.fromUint256(tokenId)));
     }

As everything requested for ERC721DefaultURI can be done with this new ERC721Metadata implementation, I would only create it as an example (maybe a blog post too) of how you can save more gas by using the tokenId as string at the tokenURI getter.

In our actual case at Decentraland using only the tokenId as string does not solve the issue. We are using an API-model approach where the tokenURI for each token is something like:
https://api.com/path1/path/:dynamicField1/:dynamicField2 Where https://api.com/path1/path/ is the baseURI and :dynamicField1/:dynamicField2 is the tokenURI.

[frangio]

I see! I slightly disagree with your point regarding the relationship between this and #1745, but we can continue the discussion in the issue.

Let's continue this PR as it was originally.

[nventuro]

@frangio @nachomazzara I ended up removing some of the getters that were causing naming issues, since they are probably not required anyway.

I also changed the behavior of tokenURI: it now only concatenates the base URI if the token's URI is not empty. In other words, _setTokenURI must be called on all token IDs, and uninitialized tokens will return the empty string, even if the base URI was set.

[nachomazzara]

@frangio @nachomazzara I ended up removing some of the getters that were causing naming issues, since they are probably not required anyway.

I also changed the behavior of tokenURI: it now only concatenates the base URI if the token's URI is not empty. In other words, _setTokenURI must be called on all token IDs, and uninitialized tokens will return the empty string, even if the base URI was set.

Thanks, @nventuro.

The changes are ok, but I think that we still need a way to get the _baseURI from children's contracts. In the implementation is private.

I will keep it as private but I would add:

/**	
     * @dev function to get the base token URI.	
     * @return string representing the base token URI	
     */	
    function baseTokenURI() public view returns (string memory) {	
        return _baseURI;	
    }

[nventuro]

After thinking about this some more, we may want to consider adding a custom event that logs the setting of a base URI.

The changes are ok, but I think that we still need a way to get the _baseURI from children's contracts. In the implementation is private.

Are there any particular use cases you have in mind for this? We often try to keep the API surface as small as possible, and perhaps there are better ways to achieve what you want to do.

[nachomazzara]

After thinking about this some more, we may want to consider adding a custom event that logs the setting of a base URI.

I thought about it and I would leave it for children's contract implementation keeping the same behavior when setting the tokenURI.

Are there any particular use cases you have in mind for this? We often try to keep the API surface as small as possible, and perhaps there are better ways to achieve what you want to do.

Sure, maybe you want to override the tokenURI method without using this.tokenURI(tokenId) having something like:

function tokenURI(uint256 tokenId) public view returns (string memory) {
     return string(abi.encodePacked(baseTokenURI(), Strings.fromUint256(tokenId)));
}

This will save gas by not using the _tokenURIs mapping.

IMO _baseURI should have the same exposure as name and symbol. It can be external or we can leave it as public to allow this.baseTokenURI.

[nventuro]

Hm, those are good arguments, thanks! I'll add it back.

[nventuro]

I've updated the PR, adding back that getter and improving the docs a bit. However, I've also rolled back the change on the visibility of tokenURI: I think we should open a separate PR to discuss whether those should be accessible to the contract or not, to help keep things tidy and trackable.

I think this is now ready to be merged. @frangio?

[frangio]

Great work guys!