Skip to content

Ability to set starting token id for ERC721Consecutive #4097

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
ernestognw merged 20 commits into from
May 26, 2023
Merged

Ability to set starting token id for ERC721Consecutive #4097

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
b693f7f
first consecutive id
kfishnchips Mar 10, 2023
bf8c076
linting and changeset
kfishnchips Mar 10, 2023
31a565b
using bound instead of assume and small test refactor
kfishnchips Mar 10, 2023
b651b93
modify transfer test to include starting token id
kfishnchips Mar 10, 2023
854b461
checking boundaries
kfishnchips Mar 10, 2023
1c7570d
reduce lookup and rename function for clarity
kfishnchips Mar 10, 2023
ab40061
rename _totalConsecutiveSupply → _nextConsecutiveId
Amxx Mar 10, 2023
1835727
Merge remote-tracking branch 'kfishnchips/master'
Amxx Mar 10, 2023
1fdcbff
Update .changeset/spotty-hotels-type.md
kfishnchips Mar 10, 2023
44cc563
fix minor bug
Amxx Mar 10, 2023
3f8b0f9
Merge branch 'master' of github.com:kfishnchips/openzeppelin-contracts
Amxx Mar 10, 2023
573fa0d
test multiple offsets
Amxx Mar 10, 2023
5d9b21b
make firstConsecutiveId immutable in fuzz tests
Amxx Mar 10, 2023
0fcb048
fix lint
Amxx Mar 10, 2023
dd1d6bf
Merge branch 'master' into master
kfishnchips Mar 10, 2023
361aa8d
Update docs
ernestognw Mar 13, 2023
114a3f6
Merge branch 'master' into master
kfishnchips Mar 13, 2023
6b4e4ee
Merge branch 'master' into master
kfishnchips Mar 27, 2023
78ed610
Merge branch 'master' into master
kfishnchips Apr 6, 2023
7959df3
Merge branch 'master' into master
kfishnchips May 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
Prev Previous commit
Next Next commit
rename _totalConsecutiveSupply → _nextConsecutiveId
  • Loading branch information
@Amxx
Amxx committed Mar 10, 2023
commit ab40061324fdbca5c086a37d7b005a1700c67cab
9 changes: 9 additions & 0 deletions contracts/mocks/token/ERC721ConsecutiveMock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,18 @@ import "../../token/ERC721/extensions/draft-ERC721Votes.sol";
* @title ERC721ConsecutiveMock
*/
contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes {
uint96 private immutable _offset;

constructor(
string memory name,
string memory symbol,
uint96 offset,
address[] memory delegates,
address[] memory receivers,
uint96[] memory amounts
) ERC721(name, symbol) EIP712(name, "1") {
_offset = offset;

for (uint256 i = 0; i < delegates.length; ++i) {
_delegate(delegates[i], delegates[i]);
}
Expand All @@ -27,6 +32,10 @@ contract ERC721ConsecutiveMock is ERC721Consecutive, ERC721Pausable, ERC721Votes
}
}

function _firstConsecutiveId() internal view virtual override returns (uint96) {
return _offset;
}

function _ownerOf(uint256 tokenId) internal view virtual override(ERC721, ERC721Consecutive) returns (address) {
return super._ownerOf(tokenId);
}
Expand Down
6 changes: 3 additions & 3 deletions contracts/token/ERC721/extensions/ERC721Consecutive.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
* Emits a {IERC2309-ConsecutiveTransfer} event.
*/
function _mintConsecutive(address to, uint96 batchSize) internal virtual returns (uint96) {
uint96 first = _totalConsecutiveSupply();
uint96 first = _nextConsecutiveId();

// minting a batch of size 0 is a no-op
if (batchSize > 0) {
Expand Down Expand Up @@ -137,7 +137,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
if (
to == address(0) && // if we burn
firstTokenId >= _firstConsecutiveId() &&
firstTokenId - _firstConsecutiveId() < _totalConsecutiveSupply() && // and the tokenId is in the batch range
firstTokenId < _nextConsecutiveId() &&
!_sequentialBurn.get(firstTokenId)
) // and the token was never marked as burnt
{
Expand All @@ -154,7 +154,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 {
return 0;
}

function _totalConsecutiveSupply() private view returns (uint96) {
function _nextConsecutiveId() private view returns (uint96) {
(bool exists, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint();
return exists ? latestId + 1 : _firstConsecutiveId();
Copy link
Member

@ernestognw ernestognw Mar 10, 2023

Choose a reason for hiding this comment

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

I don't think 0 to _firstConsecutiveId() should be accounted for the _totalConsecutiveSupply, it affects line 135, which will be excluding the after hook for real burned tokenIds

For example:

  1. Someone replaces _firstConsecutiveId() with a 5
  2. They consecutively mint tokenIds from 5 to 9 (5 mints)
  3. After that, someone else mints the token 2 via normal minting
  4. Then, they burn it, and the token will make it to the _sequentialBurn on the ownerOf(...) method, which is not what we want

I think this is what we should aim for:

Suggested change
return exists ? latestId + 1 : _firstConsecutiveId();
return exists ? latestId + 1 - _firstConsecutiveId() : 0;

Copy link
Contributor Author

@kfishnchips kfishnchips Mar 10, 2023

Choose a reason for hiding this comment

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

Would it make more sense for someone to consecutively mint token 10 instead of 2? I was thinking that there could be no minting below the first consecutive id.

Copy link
Contributor Author

@kfishnchips kfishnchips Mar 10, 2023

Choose a reason for hiding this comment

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

Would requiring that the tokenId be greater than the last batch minted one in the _mint function be a viable option? It might be too inefficient...

    function _mint(address to, uint256 tokenId) internal virtual override {
        require(Address.isContract(address(this)), "ERC721Consecutive: can't mint during construction");
        (, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint();
        require(tokenId > latestId, "ERC721Consecutive: token id must be greater than latest id");
        super._mint(to, tokenId);
    }

Copy link
Collaborator

@Amxx Amxx Mar 10, 2023

Choose a reason for hiding this comment

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

That is something we don't want, because it adds 2 (cold) sload to the mint operation, which we want to keep as cheap as possible.

Also, if we do that, it would be impossible to _mint a token that was first minted as part of a batch then burnt.

Copy link
Collaborator

@Amxx Amxx Mar 10, 2023

Choose a reason for hiding this comment

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

note that mint already check if the tokenId has an owner, and revert if that is the case.

Copy link
Contributor Author

@kfishnchips kfishnchips Mar 11, 2023

Choose a reason for hiding this comment

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

I see, I'm still wrapping my head around the internals of this but I think I got it.

}
Expand Down
37 changes: 23 additions & 14 deletions test/token/ERC721/extensions/ERC721Consecutive.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ contract('ERC721Consecutive', function (accounts) {

const name = 'Non Fungible Token';
const symbol = 'NFT';
const offset = 42;
const batches = [
{ receiver: user1, amount: 0 },
{ receiver: user1, amount: 1 },
Expand All @@ -25,6 +26,7 @@ contract('ERC721Consecutive', function (accounts) {
this.token = await ERC721ConsecutiveMock.new(
name,
symbol,
offset,
delegates,
batches.map(({ receiver }) => receiver),
batches.map(({ amount }) => amount),
Expand All @@ -33,7 +35,7 @@ contract('ERC721Consecutive', function (accounts) {

describe('minting during construction', function () {
it('events are emitted at construction', async function () {
let first = 0;
let first = offset;

for (const batch of batches) {
if (batch.amount > 0) {
Expand All @@ -51,10 +53,15 @@ contract('ERC721Consecutive', function (accounts) {
});

it('ownership is set', async function () {
const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver));
const owners = [
...Array(offset).fill(constants.ZERO_ADDRESS),
...batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)),
];

for (const tokenId in owners) {
expect(await this.token.ownerOf(tokenId)).to.be.equal(owners[tokenId]);
if (owners[tokenId] != constants.ZERO_ADDRESS) {
expect(await this.token.ownerOf(tokenId)).to.be.equal(owners[tokenId]);
}
}
});

Expand Down Expand Up @@ -101,7 +108,7 @@ contract('ERC721Consecutive', function (accounts) {
});

it('cannot mint a token that has been batched minted', async function () {
const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) - 1;
const tokenId = batches.reduce((acc, { amount }) => acc + amount, offset) - 1;

expect(await this.token.$_exists(tokenId)).to.be.equal(true);

Expand All @@ -110,32 +117,34 @@ contract('ERC721Consecutive', function (accounts) {
});

describe('ERC721 behavior', function () {
const tokenId = web3.utils.toBN(offset + 1);

it('core takes over ownership on transfer', async function () {
await this.token.transferFrom(user1, receiver, 1, { from: user1 });
await this.token.transferFrom(user1, receiver, tokenId, { from: user1 });

expect(await this.token.ownerOf(1)).to.be.equal(receiver);
expect(await this.token.ownerOf(tokenId)).to.be.equal(receiver);
});

it('tokens can be burned and re-minted #1', async function () {
expectEvent(await this.token.$_burn(1, { from: user1 }), 'Transfer', {
expectEvent(await this.token.$_burn(tokenId, { from: user1 }), 'Transfer', {
from: user1,
to: constants.ZERO_ADDRESS,
tokenId: '1',
tokenId,
});

await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID');
await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID');

expectEvent(await this.token.$_mint(user2, 1), 'Transfer', {
expectEvent(await this.token.$_mint(user2, tokenId), 'Transfer', {
from: constants.ZERO_ADDRESS,
to: user2,
tokenId: '1',
tokenId,
});

expect(await this.token.ownerOf(1)).to.be.equal(user2);
expect(await this.token.ownerOf(tokenId)).to.be.equal(user2);
});

it('tokens can be burned and re-minted #2', async function () {
const tokenId = batches.reduce((acc, { amount }) => acc.addn(amount), web3.utils.toBN(0));
const tokenId = web3.utils.toBN(batches.reduce((acc, { amount }) => acc + amount, offset));

expect(await this.token.$_exists(tokenId)).to.be.equal(false);
await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID');
Expand Down Expand Up @@ -172,7 +181,7 @@ contract('ERC721Consecutive', function (accounts) {
describe('invalid use', function () {
it('cannot mint a batch larger than 5000', async function () {
await expectRevert(
ERC721ConsecutiveMock.new(name, symbol, [], [user1], ['5001']),
ERC721ConsecutiveMock.new(name, symbol, offset, [], [user1], ['5001']),
'ERC721Consecutive: batch too large',
);
});
Expand Down