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

🧐 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.

image

πŸ“ 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: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/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];
            }
        }

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Reactions: 1
  • Comments: 15 (10 by maintainers)

Most upvoted comments

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 πŸ˜„; 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.

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.

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 ?