Skip to content
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.

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

Add ERC1155.totalSupply that returns overall supply count #3962

Merged
merged 18 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/chilled-spiders-attack.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`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 .
22 changes: 21 additions & 1 deletion contracts/token/ERC1155/extensions/ERC1155Supply.sol
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,33 @@ 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 {
// Overflow not possible: amounts[i] <= totalSupply(i)
_totalSupply[id] = supply - amount;
// Overflow not possible: sum(amounts[i]) <= sum(totalSupply(i)) <= totalSupplyAll
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

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

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 {
// Overflow not possible: totalBurnAmount = sum(amounts[i]) <= sum(totalSupply(i)) <= totalSupplyAll
_totalSupplyAll -= totalBurnAmount;
}
}
super._update(from, to, ids, amounts, data);
}
Expand Down
37 changes: 29 additions & 8 deletions test/token/ERC1155/extensions/ERC1155Supply.test.js
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,20 @@ 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('other', function () {
it('supply 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(uint256)'](firstTokenId)).to.be.bignumber.equal('0');
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.

});
});
});