Skip to content

Add ERC1155.totalSupply that returns overall supply count #3962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jan 25, 2023
Merged

Add ERC1155.totalSupply that returns overall supply count #3962

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
139363f
Fix wrong PR `CHANGELOG` link (#3957)
pcaversaccio Jan 13, 2023
3902514
update contract
JulissaDantes Jan 16, 2023
7a267c7
add tests for the new function
JulissaDantes Jan 16, 2023
17de1d9
add changelog entry
JulissaDantes Jan 16, 2023
3776289
update tests format
JulissaDantes Jan 16, 2023
154605d
Add test case
JulissaDantes Jan 17, 2023
4dae02c
update changelog
JulissaDantes Jan 17, 2023
a27ba26
update changelog
JulissaDantes Jan 17, 2023
58b21c2
move to breaking changes in the changelog
JulissaDantes Jan 18, 2023
a634d58
add reason to unchecked code
JulissaDantes Jan 18, 2023
d511fa6
clean up whitespace
frangio Jan 20, 2023
7be1534
Update test/token/ERC1155/extensions/ERC1155Supply.test.js
JulissaDantes Jan 23, 2023
e143fae
add expect of totalSupply(uint256)
frangio Jan 24, 2023
737d014
improve unchecked overflow comments
frangio Jan 24, 2023
73382df
remove .only
frangio Jan 24, 2023
7c88b4c
Merge branch 'next-v5.0' into erc1155-totalSupply
JulissaDantes Jan 25, 2023
493c12d
Update changelog
JulissaDantes Jan 25, 2023
2ddaded
Add changeset
JulissaDantes Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,9 @@ function supportsInterface(bytes4 interfaceId) public view virtual override retu
* `Math`: optimize `log256` rounding check. ([#3745](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3745))
* `Strings`: add `equal` method. ([#3774](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3774))
* `Strings`: add `toString` method for signed integers. ([#3773](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3773))
* `MerkleProof`: optimize by using unchecked arithmetic. ([#3745](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3745))
* `MerkleProof`: optimize by using unchecked arithmetic. ([#3869](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3869))
* `EnumerableMap`: add a `keys()` function that returns an array containing all the keys. ([#3920](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3920))
* `ERC1155Supply`: add a `totalSupply()` function that returns the total amount of token circulating, this change will restrict the total tokens minted across all ids to 2**256-1 . ([#3962](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3962))

### Deprecations

Expand Down
19 changes: 18 additions & 1 deletion contracts/token/ERC1155/extensions/ERC1155Supply.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import "../ERC1155.sol";
*/
abstract contract ERC1155Supply is ERC1155 {
mapping(uint256 => uint256) private _totalSupply;
uint256 private _totalSupplyAll;

/**
* @dev Total amount of tokens in with a given id.
Expand All @@ -23,6 +24,13 @@ abstract contract ERC1155Supply is ERC1155 {
return _totalSupply[id];
}

/**
* @dev Total amount of tokens.
*/
function totalSupply() public view virtual returns (uint256) {
return _totalSupplyAll;
}

/**
* @dev Indicates whether any token exist with a given id, or not.
*/
Expand All @@ -41,21 +49,30 @@ abstract contract ERC1155Supply is ERC1155 {
bytes memory data
) internal virtual override {
if (from == address(0)) {
uint256 totalMintAmount = 0;
for (uint256 i = 0; i < ids.length; ++i) {
_totalSupply[ids[i]] += amounts[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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Amxx Amxx Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@frangio frangio Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
unchecked {
_totalSupplyAll -= totalBurnAmount;
}
}
super._update(from, to, ids, amounts, data);
}
Expand Down
36 changes: 28 additions & 8 deletions test/token/ERC1155/extensions/ERC1155Supply.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
const { BN } = require('@openzeppelin/test-helpers');
const { BN, constants } = require('@openzeppelin/test-helpers');

const { expect } = require('chai');

const { ZERO_ADDRESS } = constants;

const ERC1155Supply = artifacts.require('$ERC1155Supply');

contract('ERC1155Supply', function (accounts) {
Expand All @@ -25,7 +27,8 @@ contract('ERC1155Supply', function (accounts) {
});

it('totalSupply', async function () {
expect(await this.token.totalSupply(firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply(uint256)'](firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal('0');
});
});

Expand All @@ -40,7 +43,8 @@ contract('ERC1155Supply', function (accounts) {
});

it('totalSupply', async function () {
expect(await this.token.totalSupply(firstTokenId)).to.be.bignumber.equal(firstTokenAmount);
expect(await this.token.methods['totalSupply(uint256)'](firstTokenId)).to.be.bignumber.equal(firstTokenAmount);
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal(firstTokenAmount);
});
});

Expand All @@ -60,8 +64,13 @@ contract('ERC1155Supply', function (accounts) {
});

it('totalSupply', async function () {
expect(await this.token.totalSupply(firstTokenId)).to.be.bignumber.equal(firstTokenAmount);
expect(await this.token.totalSupply(secondTokenId)).to.be.bignumber.equal(secondTokenAmount);
expect(await this.token.methods['totalSupply(uint256)'](firstTokenId)).to.be.bignumber.equal(firstTokenAmount);
expect(await this.token.methods['totalSupply(uint256)'](secondTokenId)).to.be.bignumber.equal(
secondTokenAmount,
);
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal(
firstTokenAmount.add(secondTokenAmount),
);
});
});
});
Expand All @@ -78,7 +87,8 @@ contract('ERC1155Supply', function (accounts) {
});

it('totalSupply', async function () {
expect(await this.token.totalSupply(firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply(uint256)'](firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal('0');
});
});

Expand All @@ -99,9 +109,19 @@ contract('ERC1155Supply', function (accounts) {
});

it('totalSupply', async function () {
expect(await this.token.totalSupply(firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.totalSupply(secondTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply(uint256)'](firstTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply(uint256)'](secondTokenId)).to.be.bignumber.equal('0');
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal('0');
});
});
});

context('total supply of token', function () {
it('totalSupply unaffected by no-op', async function () {
this.token.safeTransferFrom(ZERO_ADDRESS, ZERO_ADDRESS, firstTokenId, firstTokenAmount, '0x', {
from: ZERO_ADDRESS,
});
expect(await this.token.methods['totalSupply()']()).to.be.bignumber.equal('0');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
});
});