Improve extensibility of ERC1155

[frangio]

@KaiRo-at suggested that our current contract is not extensible enough due to using external functions.

They also suggested to add an internal function that implements transfers to have a central extensibility point. I think this is fine but I would like to see the internal function use msg.sender rather than having a sender argument like we did in ERC20.

[KaiRo-at]

Seems like the Augur implementation does have that internal function structure already. I think its naming of functions is not completely congruent with previous OpenZeppeling token implementations, and it implements some non-standard (but useful) extensions like totalSupply tracking, so it also could not be taken 1:1 but it can inspire for what to do.

[KaiRo-at]

This may be mostly bogus now as Solidity 0.6 and OpenZeppelin 3.0 seems to be geared towards WET rather than DRY (due to explicit overrides on Solidity's side and reducing library complexity on the OpenZeppelin side) and any changes that are not strictly extensions to the default OpenZeppelin implementation need copying of the whole contract code for other tokens as well from how I understand it.

[nventuro]

One of our top goals is definitively avoiding code repetition and users copy-pasting the library sources, this issue remains relevant.

We've already removed external from all other functions in #2162 and introduced hooks as an extensibility point with this very goal in mind (see the code for ERC777 for example), so it'd simply be a matter of implementing these on ERC1155: design work should be ready.

[KaiRo-at]

Hmm, if enough virtuals are spread throughout this and people can override those functions (I forgot for a moment that functions marked virtual do not actually require an override), then it may still work and be helpful, yes.

[KaiRo-at]

I've been pointed to https://twitter.com/xanderatallah/status/1232124941425881089 today and that makes me wonder if there is a generic way to do at least an internal implementation of _exists() like we have in ERC721, but that's not really easy the way ERC1155 is built in general.
It can be done in some specific implementations, though.

My best idea right now is something like this (and do the same as for uri() in some other places):

diff --git a/contracts/token/ERC1155/ERC1155.sol b/contracts/token/ERC1155/ERC1155.sol
index aa0d4aae..352d93ec 100644
--- a/contracts/token/ERC1155/ERC1155.sol
+++ b/contracts/token/ERC1155/ERC1155.sol
@@ -193,6 +193,17 @@ contract ERC1155 is ERC165, IERC1155
         _doSafeBatchTransferAcceptanceCheck(msg.sender, from, to, ids, values, data);
     }
 
+    /**
+     * @dev Returns whether the specified token exists. This is intentionally marked
+     * `virtual` as specific implementations may want to override to actually have
+     * existence be respected while the generic implementation always returns true.
+     * @param id uint256 ID of the token to query the existence of
+     * @return bool whether the token exists
+     */
+    function _exists(uint256 id) internal view virtual returns (bool) {
+        return true;
+    }
+
     /**
      * @dev Internal function to mint an amount of a token with the given ID
      * @param to The address that will own the minted token
diff --git a/contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol b/contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol
index ac7ff130..da2d3f8c 100644
--- a/contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol
+++ b/contracts/token/ERC1155/ERC1155MetadataURICatchAll.sol
@@ -31,7 +31,8 @@ contract ERC1155MetadataURICatchAll is ERC165, ERC1155, IERC1155MetadataURI {
      * as an {id} parameter in the string is expected)
      * @return URI string
     */
-    function uri(uint256 /*id*/) external view override returns (string memory) {
+    function uri(uint256 id) external view override returns (string memory) {
+        require(_exists(id), "ERC1155MetadataURI: URI query for nonexistent token");
         return _uri;
     }

[frangio]

(@KaiRo-at I edited your comment just to add diff highlighting.)

I like the idea that specific projects may be able to define a notion of "exists", but for now I'm inclining towards not encoding that in our base contract because it's something rare and that would be simple to add on top.

[KaiRo-at]

@frangio the problem is that quite a few functions that use a require(_exists()) pattern are probably view functions that are not declared virtual and therefore cannot be overridden easily, like the example of uri() I'm giving there. (...and thanks for the diff highlighting!)

[frangio]

Aha! That's super interesting. Do you know of any projects that are using or considering to include this notion of exists?

[KaiRo-at]

You mean, other than the one I'm working on right now? ;-)
Unfortunately, I can't make it public atm, but I'll be using all the ERC1155 patches I posted here and this exists() stuff in addition, in a project that is to be released "soon".
Also, having that notion of _exists() would match the OpenZeppelin ERC721 implementation.

[KaiRo-at]

I thought about this whole thing some more and I'm taking the suggestion to its own PR at #2185 - I figured it's probably best if we have an actually working _exists() there and figured a simple way we can do this (and not need the view virtual stuff), see there.

[Cybourgeoisie]

Hey all - I'd like to propose setting virtual to uri and isApprovedForAll in the ERC1155.sol file. OpenSea currently relies on an override to the default isApprovedForAll to permit use of a proxy address, and OpenSea relies on uri returning the id value concatenated to the URI.

[abcoathup]

Hi @Cybourgeoisie,
There is also a general discussion on making meta data view functions virtual:
#2154

[frangio]

This issue has been fixed now that functions are both public and virtual.