Add ERC1155.totalSupply that returns overall supply count

[JulissaDantes]

Fixes #3301

This PR adds a tracking storage variable inside the ERC1155Supply contract, to keep track of the amount of all the circulating tokens, without having to check each tokenId's total supply.

PR Checklist

• Tests
• Documentation
• Changelog entry

[Amxx]

There is something terribly wrong here:

• totalAmount starts at 0
• you increase is for every new token
• you add that sum to the _totalSupplyAll
• you then increase it again (without resetting it) with every token being burned
• you decrease that from the _totalSupplyAll

So if someone transfers from 0 to 0 (which would happen with the new _update transition), you would mess up the _totalSupplyAll :/

Also, on a completely different point, this change will restrict a total of 2**256-1 tokens minted accross ALL ids. This is a significant change, which needs to be documented.

[Amxx]

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

        if (to == address(0)) {
            for (uint256 i = 0; i < ids.length; ++i) {
                uint256 id = ids[i];
                uint256 amount = amounts[i];
                uint256 supply = _totalSupply[id];
                require(supply >= amount, "ERC1155: burn amount exceeds totalSupply");
                unchecked {
                    _totalSupply[id] = supply - amount;
                    totalAmount += amount;
                }
            }
            _totalSupplyAll -= totalAmount;
        }

should be

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

        if (to == address(0)) {
            uint256 totalBurnAmount = 0;
            for (uint256 i = 0; i < ids.length; ++i) {
                uint256 id = ids[i];
                uint256 amount = amounts[i];
                uint256 supply = _totalSupply[id];
                require(supply >= amount, "ERC1155: burn amount exceeds totalSupply");
                unchecked {
                    _totalSupply[id] = supply - amount;
                    totalBurnAmount += amount;
                }
            }
            unchecked {
                _totalSupplyAll -= totalBurnAmount;
            }
        }

[JulissaDantes]

Also, on a completely different point, this change will restrict a total of 2**256-1 tokens minted accross ALL ids. This is a significant change, which needs to be documented.

Thanks for pointing this out, now it uses uint update:It is documented.

[Amxx]

Thanks for pointing this out, now it uses uint.

uint is an alias to uint256. Using one or the other is strictly equivalent and results in the same bytecode. Any limitation that you have with uint256 you will also have with uint.

We should be using uint256 all the time, just to avoid possible confusion, and also because function signature & EIP-712 type hash are produced using uint256

[Amxx]

This is a breaking change on 2 fronts:

• this change restrict the ability to mint token, imposing a new limit that was not there before
• upgrading from the previous version to this is ok from a storage layout point of view... but it would lead to inconsistent values (and even mint reverting)

For the mint reverting:

• you already have 100 token
• you upgrade, the totalSupply() is 0
• you burn 1 token ... decrease is unchecked so is overflows
• totalSupply() is now 2**256-1
• you try to mint 1 token ... increase is checked and reverts because of the overflow

[frangio]

Yeah I share the concerns. Let's move this to 5.0. @JulissaDantes Please rebase or rewrite this on top of next-v5.0.

[JulissaDantes]

Yeah I share the concerns. Let's move this to 5.0. @JulissaDantes Please rebase or rewrite this on top of next-v5.0.

Ready.

[frangio]

It isn't obvious to me why this variable won't overflow. We need to add the explanation.

[Amxx]

It comes from two inequalities:

• amounts[i] <= totalSupply(i) (from the require on line 67)
• sum(totalSupply(i)) <= totalSupplyAll (contract invariant)

so totalBurnAmount = sum(amounts[i]) <= sum(totalSupply(i)) <= totalSupplyAll <= 2*256-1

[Amxx]

Technically we could even get rid of the check on line 67 if we were to check balance >= amount before. (the check if done after, because this is the "pre" hook)

[frangio]

@Amxx I think we didn't do that because we were concerned what would happen if ERC1155Supply was added in an upgrade... I wish we had a more consistent way of dealing with that kind of concern.

[changeset-bot]

🦋 Changeset detected

Latest commit: 2ddaded

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[frangio]

Is there a reason not to check totalSupply(firstTokenId) as well?

[JulissaDantes]

Since Is checked multiple times before this test I did not think we needed it, but do you think we should include it for a no-op test?

[frangio]

Yes, totalSupply(uint256) is not checked for no-op transfers from zero to zero. I added the test.