Add ERC1155URIStorage

[takahser]

Fixes #3206

• add a new extension for ERC1155 called ERC1155URIStorage that implements a ERC721-style _setTokenURI and tokenURI behaviors.

PR Checklist

• Tests
• Documentation (inline)
• Changelog entry

[Amxx]

Hello @takahser

You are depending on ERC1155Supply instead of ERC1155, yet it doesn't look like you are using the supply features. Why is that ?

[takahser]

You are depending on ERC1155Supply instead of ERC1155, yet it doesn't look like you are using the supply features. Why is that ?

This is because this extension depends on the 'exists' method that's located on the ERC1155Supply contract.

[Amxx]

I'd personnally remove the exist check, and drop the dependency on ERC1155Supply.

@frangio any opinion on that ?

[frangio]

This has come up before. Perhaps exists should be in ERC1155 itself... (with a default implementation returning true)

For now I'm ok with dropping the exists check in this PR.

[takahser]

@frangio @Amxx Thx for your feedback, I updated the PR accordingly.

[frangio]

I don't think it's right to use super.uri as a base URI and use concatenation. super.uri should be seen like a "global" URI and _setTokenURI should act like an override.

So, someone could set global URI as ipfs://abcd/{id}.json and then set an override ipfs://efgh for a particular id.

I think the base URI mechanism we have in ERC721 makes less sense here because there is already a built in base URI-like mechanism built into the ERC. So just the global URI plus a storage-based override sounds like enough.

[takahser]

I think the base URI mechanism we have in ERC721 makes less sense here because there is already a built in base URI-like mechanism built into the ERC. So just the global URI plus a storage-based override sounds like enough.

@frangio In that case, the following scenario wouldn't be supported:

• ERC1155._uri set to https://token.com/
• token uri set to one
• concatenated uri results in https://token.com/one

Hence, I think @Amxx proposal provides more flexibility.

[frangio]

If we want that behavior, I think we need a separate _baseURI variable. But again, I don't think it's necessary because the ERC has a built in mechanism for this.

You may as well just set uri to https://token.com/{id} and publish the metadata in https://token.com/0...01 instead of https://token.com/one.

[takahser]

If we want that behavior, I think we need a separate _baseURI variable. But again, I don't think it's necessary because the ERC has a built in mechanism for this.

Well, don't we already have a _baseURI variable, although it's actually called _uri:

// Used as the URI for all token types by relying on ID substitution, e.g. https://token-cdn-domain/{id}.json
string private _uri;

You may as well just set uri to https://token.com/{id} and publish the metadata in https://token.com/0...01 instead of https://token.com/one.

While I agree that this should be sufficient in most cases, there might be some edge cases where you'd want to have this extra flexibility, e.g. because you have to support a legacy system. Or maybe you want to have different locations on the same base uri for different categories of NFTs. I'm sure there will be more edge cases.

[Amxx]

ERC1155 includes a "base" variable (_uri) which can be set using the internal _setURI(string) method, and read using super.uri()

IMO, for naming consistency, the setter in ERC155URIStorage should probably be _setURI(uint256, string);

[takahser]

ERC1155 includes a "base" variable (_uri) which can be set using the internal _setURI(string) method, and read using super.uri()

IMO, for naming consistency, the setter in ERC155URIStorage should probably be _setURI(uint256, string);

The problem I see with that approach is that it'd make it impossible to change the base IERC1155._uri.
Please note that the constructor expects a string memory uri_ argument:

    /**
     * @dev See {_setURI}.
     */
    constructor(string memory uri_) {
        _setURI(uri_);
    }

[Amxx]

Why would it make impossible to change the base URI ?

_setURI(string); // this would changes the base URI
_setURI(tokenId, string); // this would changes the "extra" URI for tokenId

[takahser]

@Amxx Apologies, for some reason, I ignored the fact that the two methods have different signatures. Ofc you're right, I updated the PR. 👍

[Amxx]

If we want that behavior, I think we need a separate _baseURI variable. But again, I don't think it's necessary because the ERC has a built in mechanism for this.

You may as well just set uri to https://token.com/{id} and publish the metadata in https://token.com/0...01 instead of https://token.com/one.

We could possibly write this override mechanism as

function uri(uint256 tokenId) public view virtual override returns (string memory) {
    string memory _tokenURI = _tokenURIs[tokenId];
    
    return bytes(_tokenURI).length > 0
        ? _tokenURI
        : super.uri(tokenId);
}

IMO that would fit most usecases I see, and would make it simpler to have a default uri. @takahser what do you think?

[takahser]

@Amxx tbh I'd prefer the current implementation since it supports combining the ERC1155. uri with the ERC1155URIStorage._tokenURIs[tokenId]. That's not the case with your suggestion (fee free to correct me if I'm wrong). Is there a particular reason you don't want to support this use case?

[Amxx]

@Amxx tbh I'd prefer the current implementation since it supports combining the ERC1155. uri with the ERC1155URIStorage._tokenURIs[tokenId]. That's not the case with your suggestion (fee free to correct me if I'm wrong). Is there a particular reason you don't want to support this use case?

Well, technically, if you wanted to concatenate both you could do that.

Contract X is ERC1155URIStorage {
    
    /// THIS IS JUST AN EXAMPLE, PLEASE DON'T DO THAT
    function uri(uint256 tokenId) public view virtual override returns (string memory) {
        string memory base = ERC1155.uri(tokenId);
        string memory extra = ERC1155URIStorage.uri(tokenId);
        
        return keccak256(base) == keccak256(extra) 
            ? base
            : string.concat(base, extra);
    }
}

@frangio might say this is terrible, and I would agree, but its technically possible.

This point is that, if the "override" is concatenated to the base, that has a strong restriction on the base, and it basically kills the possibility of having a usefull base, that would have a real meaning even when it is not overridden with token specific info.

[Amxx]

Not that ERC721 is designed differently, because the super call (if _tokenURI is not set) will append the tokenId string.

In ERC1155, this is achieved through the use of {id} in the base

[takahser]

Fell, technically, if you wanted to concatenate both you could do that.
This point is that, if the "override" is concatenated to the base, that has a strong restriction on the base, and it basically kills the possibility of having a usefull base, that would have a real meaning even when it is not overridden with token specific info.

Sorry, you lost me there. Would you mind elaborating on that using an example?

[frangio]

super.uri() should not be taken as a base URI. It should be taken as a default global URI, and it may include {id} in it to result in a different URI per token. _setTokenURI should override that default global URI entirely, it should not concatenate.

If we want concatenation, we need a separate _baseURI variable and _setBaseURI setter.

@takahser Feel free to implement either of these options but please do not use super.uri for concatenation. It should still be the fallback if a custom token URI is not set, though.

[takahser]

@frangio that actually sounds logical to me. I'm going to implement the 2nd suggestion you made (separate base URI). 👍

[takahser]

@Amxx @frangio PR updated 👍

[Amxx]

The super call on line 42 should be done on line 50, in the return. We don't want to have to pay for it if it turns out it's not needed.

@frangio do you like this baseUri mechanism, considering it is not used by the core erc1155 ?
I'm not really confortable...

[takahser]

@Amxx @frangio PR updated, conflict resolved 👍
I'm not sure why Test / slither is failing, it used to work before fixing the conflict. The conflict was just in the CHANGELOG.md so I doubt that this is the reason (correct me if I'm wrong).

[takahser]

@Amxx @frangio did you have time to look into this yet?

[takahser]

@Amxx @frangio

• PR updated
• new conflicts resolved
• all checks passed

Any feedback would be appreciated:)

[takahser]

@Amxx @frangio any updates on this one?

[takahser]

Thx guys!

[Amxx]

Thx guys!

Thanks for your work and your patience