-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Changes from all commits
139363f
3902514
7a267c7
17de1d9
3776289
154605d
4dae02c
a27ba26
58b21c2
a634d58
d511fa6
7be1534
e143fae
737d014
73382df
7c88b4c
493c12d
2ddaded
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 . |
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) { | ||
|
@@ -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'); | ||
}); | ||
}); | ||
|
||
|
@@ -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); | ||
}); | ||
}); | ||
|
||
|
@@ -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), | ||
); | ||
}); | ||
}); | ||
}); | ||
|
@@ -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'); | ||
}); | ||
}); | ||
|
||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason not to check There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.