ERC1155Supply: Add totalSupply() function that returns overall supply count

[philipSKYBIT]

🧐 Motivation
ERC1155Supply only counts by token ID. In some cases we may want a general overall count of all tokens that have been minted under the contract. e.g. etherscan and polygonscan tries to call totalSupply() when showing information about an NFT, and shows an error on the page if it failed.

📝 Details
Having only a count for each token ID in ERC1155Supply means to get an overall total count, options are:

• loop through all minted token IDs and call totalSupply(id)
• or not use ERC1155Supply and within the ERC1155 contract:
  -- use Counters. But Counters cannot increment by more than 1, which we'd want after a batch mint. Here is an issue about this: Allow Counter increment by more than just 1 #3296
  -- or just use uint256 instead of both ERC1155Supply and Counters

I still want to use ERC1155Supply as its purpose is supply tracking. I propose to add overloaded function totalSupply() that returns the overall count.

Some code that could be added are:

uint256 private _totalSupplyAll;

    function totalSupply() public view virtual returns (uint256) {
        return _totalSupplyAll;
    }

In _beforeTokenTransfer:

        if (from == address(0)) {
            for (uint256 i = 0; i < ids.length; ++i) {
                _totalSupply[ids[i]] += amounts[i];
                _totalSupplyAll += amounts[i];
            }
        }

        if (to == address(0)) {
            for (uint256 i = 0; i < ids.length; ++i) {
                _totalSupply[ids[i]] -= amounts[i];
                _totalSupplyAll -= amounts[i];
            }
        }

[Amxx]

Hello @philipSKYBIT

If someone wanted to track the overall token supply, the best way would indeed be to have a uint256 _totalSupplyAll variable, that gets incremented in the _beforeTokenTransfer hook.

The reason why we don't have it is:

• When we received a request for totalSupply support in ERC1155, what was requested was a totalSupply per ID
• We have not, so far, seen any usecase for a totalSupplyAll.
• A developer can easily add that himself if he/she needs it
• Bundling it with ERC1155Supply would make this module even more expensive to use. Why pay for both if you possibly only need one?

[frangio]

To be fair, this variable would only get read/written on mints and burns, so I wouldn't entirely rule out adding totalSupply() in ERC1155Supply.

Before agreeing, I would like to understand the motivation a little more. What page on Etherscan tries to access this function?

[philipSKYBIT]

What page on Etherscan tries to access this function?

On the Etherscan token page e.g.:
https://etherscan.io/token/0xddb149ae8e6635df01a530da1e46921bd78dc385
https://etherscan.io/token/0x341a1c534248966c4b6afad165b98daed4b964ef

It's the same on Polygonscan.

We want to be able to show total supply minted so far on a minting dapp page.

[frangio]

@philipSKYBIT Can you confirm on a testnet Etherscan that if there is a totalSupply() method in an ERC1155 token Etherscan will show its value properly?

[philipSKYBIT]

https://goerli.etherscan.io/token/0xfA7644E6204B2eeA4C46762Bf82a9570a2054eE8

I minted 2 tokens, and etherscan correctly shows 2 as total supply.

I had created and imported ERC1155SupplyWithAll.sol which is my modified version of ERC1155Supply.sol:

// SPDX-License-Identifier: MIT
// Modified version of OpenZeppelin Contracts v4.4.1 (token/ERC1155/extensions/ERC1155Supply.sol) - added totalSupply overall

pragma solidity ^0.8.0;

// import "../ERC1155.sol";
import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";

/**
 * @dev Extension of ERC1155 that adds tracking of total supply per id.
 *
 * Useful for scenarios where Fungible and Non-fungible tokens have to be
 * clearly identified. Note: While a totalSupply of 1 might mean the
 * corresponding is an NFT, there is no guarantees that no other token with the
 * same id are not going to be minted.
 */
abstract contract ERC1155SupplyWithAll is ERC1155 {
    mapping(uint256 => uint256) private _totalSupply;
    uint256 private _totalSupplyAll;

    /**
     * @dev Total amount of tokens in with a given id.
     */
    function totalSupply(uint256 id) public view virtual returns (uint256) {
        return _totalSupply[id];
    }

    function totalSupply() public view virtual returns (uint256) {
        return _totalSupplyAll;
    }

    /**
     * @dev Indicates whether any token exist with a given id, or not.
     */
    function exists(uint256 id) public view virtual returns (bool) {
        return totalSupply(id) > 0;
    }

    /**
     * @dev See {ERC1155-_beforeTokenTransfer}.
     */
    function _beforeTokenTransfer(
        address operator,
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) internal virtual override {
        super._beforeTokenTransfer(operator, from, to, ids, amounts, data);

        if (from == address(0)) {
            for (uint256 i = 0; i < ids.length; ++i) {
                _totalSupply[ids[i]] += amounts[i];
                _totalSupplyAll += amounts[i];
            }
        }

        if (to == address(0)) {
            for (uint256 i = 0; i < ids.length; ++i) {
                _totalSupply[ids[i]] -= amounts[i];
                _totalSupplyAll -= amounts[i];
            }
        }
    }
}

[apecollector]

Would really be good if this was considered, I think the current supply makes the contract seem broken or non-standard conforming when viewing on etherscan:

Also I can confirm that with a totalSupply() method it shows the total tokens:

[frangio]

Feel free to submit a pull request with this change to ERC1155Supply.

Note that the code submitted by @philipSKYBIT above is suboptimal because the global supply is updated once for each token in the batch transfer, we should optimize this by accumulating in one variable and writing to storage just once.

[pcaversaccio]

Since I just recently implemented the ERC1155 token in Vyper I was also considering this option but I would like to share a big caveat here (not sure whether this was already discussed; at least not in this issue) that I arrived at when thinking about it. The variable _totalSupplyAll is of type uint256, meaning that the overall supply across all ids cannot exceed type(uint256).max (in order to make that theoretically work we would need a uint512 type; probably practically something like uint328 would be enough :)). I feel like this is important to consider before a decision is made to implement that feature.

[Amxx]

Yes @pcaversaccio, this is something we have identified. See this comment on the PR.

Do you feel like this constraint would be cause real issues, or will it remain mostly theoretical. Said differently, do you know any ERC1155 contract where the supply as large enough for this value to overflow ?

[pcaversaccio]

awesome - thx for the link. So am personally not aware of any ERC1155 contract which would be close enough to exceed the total supply of 2**256-1. Maybe it's time to run some Dune analytics :-). What I however feel is that new contract features should be future-proof. Since ERC1155 has efficient minting it could be reasonable that maybe for certain, yet unknown use cases, the threshold will be breached. Or in other words, I would consider this caveat as a rather low-risk issue in the current environment but since we live in an immutable world, decisions made now can have long-lasting impacts and we're not aware of the market dynamics in 1 month, 1 year or so on (NFTs were not even part of the Ethereum White Paper iirc).

[frangio]

In any case this will never be included by default so users can make an informed decision if they decide to use this functionality with what it entails.

I had also proposed to make the total supply number saturate at the max and cause the getter to revert from then on. This would be acceptable if the function is only used off chain but can't be sure about that.

[pcaversaccio]

so users can make an informed decision

I wish this was true - how many times will people simply go to OpenZeppelin Wizard, and enable "Supply Tracking" because they read "Keeps track of total supply of tokens." which sounds like a good feature to have and that's it...

Reverting with a nice message or custom error would be indeed informative but who knows how people will interact in the future.

[frangio]

We're with you, making a library without footguns is definitely part of our design philosophy, but at some point we need to treat users like adults too. 😅 In this particular case, the reasoning for me is that this is unlikely to be a problem. If it was likely, say it capped the supply to a low number, then I would not be okay with it.

[pcaversaccio]

we need to treat users like adults too.

Fair enough 😎 - but just for the sake of history, I hereby make a bet that at some time in the future there will be a case where an overflow will happen and it will pose a problem (think of totalSupply is used as part of a weird fee calculation somewhere :-D; they will be surprised if at some point the calculation will revert or wrap, depending on the implementation). But overall I agree with you that at some point you guys need to accept certain tradeoffs. I think actually that the most important part is already done - there is an issue documenting this behavior as well as proper documentation will explain this design pattern. I actually like that you don't want to include it in 4.9.0 since it's kind of a breaking change.

[frangio]

Fixed by #3962 in next-v5.0.