openzeppelin-contracts

openzeppelin-contracts copied to clipboard

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

• [X] Tests
• [X] Documentation
• [ ] Changelog entry

        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;
            }
        }

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.

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

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

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.

🦋 Changeset detected

Latest commit: 2ddadedd206cbaa0b3116be4e5527f4da9fae311

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

I added a missing comment in one of the unchecked blocks. Please refer to the point about unchecked in the guidelines.

I'm not sure I understand why we have such a CHANGELOG.md diff. Is that an error ?

The last merge of master and nextv5.0 remove all the breaking changes entries in the CHANGELOG from nextv5.0. That's why they show here, unless it was intentional and I should remove the entries too.

Sorry for being pedantic here, but the PR does not include any user-facing documentation wrt the overflow (except in the CHANGELOG). Do you plan to add something here since it is used in the OZ docs (not sure about the underlying UI mechanics on the tags tho):

/**
 * @dev Total amount of tokens.
 * @custom:security Implementing this function will restrict the total tokens minted across all `ids` to 2**256-1.
 */
function totalSupply() public view virtual returns (uint256) {
    return _totalSupplyAll;
}

@pcaversaccio Thanks for raising that. The note should be at the contract level and it should be a "NOTE:" section.

Added note in https://github.com/OpenZeppelin/openzeppelin-contracts/commit/2d05db171a99e93132abef98bda15534129f7805.