diff --git a/.changeset/spotty-hotels-type.md b/.changeset/spotty-hotels-type.md new file mode 100644 index 000000000..866d8fc02 --- /dev/null +++ b/.changeset/spotty-hotels-type.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC721Consecutive`: Add a `_firstConsecutiveId` internal function that can be overridden to change the id of the first token minted through `_mintConsecutive`. diff --git a/contracts/mocks/token/ERC721ConsecutiveMock.sol b/contracts/mocks/token/ERC721ConsecutiveMock.sol index 427f44a19..65859f956 100644 --- a/contracts/mocks/token/ERC721ConsecutiveMock.sol +++ b/contracts/mocks/token/ERC721ConsecutiveMock.sol @@ -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]); } @@ -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); } diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 4312d9849..abdba5b90 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -20,8 +20,8 @@ import "../../../utils/structs/BitMaps.sol"; * regained after construction. During construction, only batch minting is allowed. * * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for tokens minted in - * batch. When using this extension, you should consider the {_beforeConsecutiveTokenTransfer} and - * {_afterConsecutiveTokenTransfer} hooks in addition to {_beforeTokenTransfer} and {_afterTokenTransfer}. + * batch. The hooks will be only called once per batch, so you should take `batchSize` parameter into consideration + * when relying on hooks. * * IMPORTANT: When overriding {_afterTokenTransfer}, be careful about call ordering. {ownerOf} may return invalid * values during the {_afterTokenTransfer} execution if the super call is not called first. To be safe, execute the @@ -56,7 +56,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { address owner = super._ownerOf(tokenId); // If token is owned by the core, or beyond consecutive range, return base value - if (owner != address(0) || tokenId > type(uint96).max) { + if (owner != address(0) || tokenId > type(uint96).max || tokenId < _firstConsecutiveId()) { return owner; } @@ -82,7 +82,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 next = _nextConsecutiveId(); // minting a batch of size 0 is a no-op if (batchSize > 0) { @@ -91,29 +91,29 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { require(batchSize <= _maxBatchSize(), "ERC721Consecutive: batch too large"); // hook before - _beforeTokenTransfer(address(0), to, first, batchSize); + _beforeTokenTransfer(address(0), to, next, batchSize); // push an ownership checkpoint & emit event - uint96 last = first + batchSize - 1; + uint96 last = next + batchSize - 1; _sequentialOwnership.push(last, uint160(to)); // The invariant required by this function is preserved because the new sequentialOwnership checkpoint // is attributing ownership of `batchSize` new tokens to account `to`. __unsafe_increaseBalance(to, batchSize); - emit ConsecutiveTransfer(first, last, address(0), to); + emit ConsecutiveTransfer(next, last, address(0), to); // hook after - _afterTokenTransfer(address(0), to, first, batchSize); + _afterTokenTransfer(address(0), to, next, batchSize); } - return first; + return next; } /** * @dev See {ERC721-_mint}. Override version that restricts normal minting to after construction. * - * Warning: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. + * WARNING: Using {ERC721Consecutive} prevents using {_mint} during construction in favor of {_mintConsecutive}. * After construction, {_mintConsecutive} is no longer available and {_mint} becomes available. */ function _mint(address to, uint256 tokenId) internal virtual override { @@ -132,17 +132,30 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { ) internal virtual override { if ( to == address(0) && // if we burn - firstTokenId < _totalConsecutiveSupply() && // and the tokenId was minted in a batch - !_sequentialBurn.get(firstTokenId) // and the token was never marked as burnt - ) { + firstTokenId >= _firstConsecutiveId() && + firstTokenId < _nextConsecutiveId() && + !_sequentialBurn.get(firstTokenId) + ) // and the token was never marked as burnt + { require(batchSize == 1, "ERC721Consecutive: batch burn not supported"); _sequentialBurn.set(firstTokenId); } super._afterTokenTransfer(from, to, firstTokenId, batchSize); } - function _totalConsecutiveSupply() private view returns (uint96) { + /** + * @dev Used to offset the first token id in {_nextConsecutiveId} + */ + function _firstConsecutiveId() internal view virtual returns (uint96) { + return 0; + } + + /** + * @dev Returns the next tokenId to mint using {_mintConsecutive}. It will return {_firstConsecutiveId} + * if no consecutive tokenId has been minted before. + */ + function _nextConsecutiveId() private view returns (uint96) { (bool exists, uint96 latestId, ) = _sequentialOwnership.latestCheckpoint(); - return exists ? latestId + 1 : 0; + return exists ? latestId + 1 : _firstConsecutiveId(); } } diff --git a/test/token/ERC721/extensions/ERC721Consecutive.t.sol b/test/token/ERC721/extensions/ERC721Consecutive.t.sol index 3cc868929..5bcbb024d 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.t.sol +++ b/test/token/ERC721/extensions/ERC721Consecutive.t.sol @@ -14,9 +14,11 @@ function toSingleton(address account) pure returns (address[] memory) { } contract ERC721ConsecutiveTarget is StdUtils, ERC721Consecutive { + uint96 private immutable _offset; uint256 public totalMinted = 0; - constructor(address[] memory receivers, uint256[] memory batches) ERC721("", "") { + constructor(address[] memory receivers, uint256[] memory batches, uint256 startingId) ERC721("", "") { + _offset = uint96(startingId); for (uint256 i = 0; i < batches.length; i++) { address receiver = receivers[i % receivers.length]; uint96 batchSize = uint96(bound(batches[i], 0, _maxBatchSize())); @@ -28,43 +30,71 @@ contract ERC721ConsecutiveTarget is StdUtils, ERC721Consecutive { function burn(uint256 tokenId) public { _burn(tokenId); } + + function _firstConsecutiveId() internal view virtual override returns (uint96) { + return _offset; + } } contract ERC721ConsecutiveTest is Test { - function test_balance(address receiver, uint256[] calldata batches) public { + function test_balance(address receiver, uint256[] calldata batches, uint96 startingId) public { vm.assume(receiver != address(0)); - ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches); + uint256 startingTokenId = bound(startingId, 0, 5000); + + ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches, startingTokenId); assertEq(token.balanceOf(receiver), token.totalMinted()); } - function test_ownership(address receiver, uint256[] calldata batches, uint256[2] calldata unboundedTokenId) public { + function test_ownership( + address receiver, + uint256[] calldata batches, + uint256[2] calldata unboundedTokenId, + uint96 startingId + ) public { vm.assume(receiver != address(0)); - ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches); + uint256 startingTokenId = bound(startingId, 0, 5000); + + ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches, startingTokenId); if (token.totalMinted() > 0) { - uint256 validTokenId = bound(unboundedTokenId[0], 0, token.totalMinted() - 1); + uint256 validTokenId = bound( + unboundedTokenId[0], + startingTokenId, + startingTokenId + token.totalMinted() - 1 + ); assertEq(token.ownerOf(validTokenId), receiver); } - uint256 invalidTokenId = bound(unboundedTokenId[1], token.totalMinted(), type(uint256).max); + uint256 invalidTokenId = bound( + unboundedTokenId[1], + startingTokenId + token.totalMinted(), + startingTokenId + token.totalMinted() + 1 + ); vm.expectRevert(); token.ownerOf(invalidTokenId); } - function test_burn(address receiver, uint256[] calldata batches, uint256 unboundedTokenId) public { + function test_burn( + address receiver, + uint256[] calldata batches, + uint256 unboundedTokenId, + uint96 startingId + ) public { vm.assume(receiver != address(0)); - ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches); + uint256 startingTokenId = bound(startingId, 0, 5000); + + ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches, startingTokenId); // only test if we minted at least one token uint256 supply = token.totalMinted(); vm.assume(supply > 0); // burn a token in [0; supply[ - uint256 tokenId = bound(unboundedTokenId, 0, supply - 1); + uint256 tokenId = bound(unboundedTokenId, startingTokenId, startingTokenId + supply - 1); token.burn(tokenId); // balance should have decreased @@ -78,25 +108,28 @@ contract ERC721ConsecutiveTest is Test { function test_transfer( address[2] calldata accounts, uint256[2] calldata unboundedBatches, - uint256[2] calldata unboundedTokenId + uint256[2] calldata unboundedTokenId, + uint96 startingId ) public { vm.assume(accounts[0] != address(0)); vm.assume(accounts[1] != address(0)); vm.assume(accounts[0] != accounts[1]); + uint256 startingTokenId = bound(startingId, 1, 5000); + address[] memory receivers = new address[](2); receivers[0] = accounts[0]; receivers[1] = accounts[1]; // We assume _maxBatchSize is 5000 (the default). This test will break otherwise. uint256[] memory batches = new uint256[](2); - batches[0] = bound(unboundedBatches[0], 1, 5000); - batches[1] = bound(unboundedBatches[1], 1, 5000); + batches[0] = bound(unboundedBatches[0], startingTokenId, 5000); + batches[1] = bound(unboundedBatches[1], startingTokenId, 5000); - ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(receivers, batches); + ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(receivers, batches, startingTokenId); - uint256 tokenId0 = bound(unboundedTokenId[0], 0, batches[0] - 1); - uint256 tokenId1 = bound(unboundedTokenId[1], 0, batches[1] - 1) + batches[0]; + uint256 tokenId0 = bound(unboundedTokenId[0], startingTokenId, batches[0]); + uint256 tokenId1 = bound(unboundedTokenId[1], startingTokenId, batches[1]) + batches[0]; assertEq(token.ownerOf(tokenId0), accounts[0]); assertEq(token.ownerOf(tokenId1), accounts[1]); @@ -119,4 +152,29 @@ contract ERC721ConsecutiveTest is Test { assertEq(token.balanceOf(accounts[0]), batches[0]); assertEq(token.balanceOf(accounts[1]), batches[1]); } + + function test_start_consecutive_id( + address receiver, + uint256[2] calldata unboundedBatches, + uint256[2] calldata unboundedTokenId, + uint96 startingId + ) public { + vm.assume(receiver != address(0)); + + uint256 startingTokenId = bound(startingId, 1, 5000); + + // We assume _maxBatchSize is 5000 (the default). This test will break otherwise. + uint256[] memory batches = new uint256[](2); + batches[0] = bound(unboundedBatches[0], startingTokenId, 5000); + batches[1] = bound(unboundedBatches[1], startingTokenId, 5000); + + ERC721ConsecutiveTarget token = new ERC721ConsecutiveTarget(toSingleton(receiver), batches, startingTokenId); + + uint256 tokenId0 = bound(unboundedTokenId[0], startingTokenId, batches[0]); + uint256 tokenId1 = bound(unboundedTokenId[1], startingTokenId, batches[1]); + + assertEq(token.ownerOf(tokenId0), receiver); + assertEq(token.ownerOf(tokenId1), receiver); + assertEq(token.balanceOf(receiver), batches[0] + batches[1]); + } } diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index a92aef480..4e2df4690 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -20,159 +20,169 @@ contract('ERC721Consecutive', function (accounts) { ]; const delegates = [user1, user3]; - describe('with valid batches', function () { - beforeEach(async function () { - this.token = await ERC721ConsecutiveMock.new( - name, - symbol, - delegates, - batches.map(({ receiver }) => receiver), - batches.map(({ amount }) => amount), - ); - }); - - describe('minting during construction', function () { - it('events are emitted at construction', async function () { - let first = 0; - - for (const batch of batches) { - if (batch.amount > 0) { - await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { - fromTokenId: web3.utils.toBN(first), - toTokenId: web3.utils.toBN(first + batch.amount - 1), - fromAddress: constants.ZERO_ADDRESS, - toAddress: batch.receiver, - }); - } else { - // expectEvent.notEmitted.inConstruction only looks at event name, and doesn't check the parameters - } - first += batch.amount; - } - }); - - it('ownership is set', async function () { - const owners = batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)); - - for (const tokenId in owners) { - expect(await this.token.ownerOf(tokenId)).to.be.equal(owners[tokenId]); - } - }); - - it('balance & voting power are set', async function () { - for (const account of accounts) { - const balance = batches - .filter(({ receiver }) => receiver === account) - .map(({ amount }) => amount) - .reduce((a, b) => a + b, 0); - - expect(await this.token.balanceOf(account)).to.be.bignumber.equal(web3.utils.toBN(balance)); - - // If not delegated at construction, check before + do delegation - if (!delegates.includes(account)) { - expect(await this.token.getVotes(account)).to.be.bignumber.equal(web3.utils.toBN(0)); - - await this.token.delegate(account, { from: account }); - } - - // At this point all accounts should have delegated - expect(await this.token.getVotes(account)).to.be.bignumber.equal(web3.utils.toBN(balance)); - } - }); - }); - - describe('minting after construction', function () { - it('consecutive minting is not possible after construction', async function () { - await expectRevert( - this.token.$_mintConsecutive(user1, 10), - 'ERC721Consecutive: batch minting restricted to constructor', + for (const offset of [0, 1, 42]) { + describe(`with offset ${offset}`, function () { + beforeEach(async function () { + this.token = await ERC721ConsecutiveMock.new( + name, + symbol, + offset, + delegates, + batches.map(({ receiver }) => receiver), + batches.map(({ amount }) => amount), ); }); - it('simple minting is possible after construction', async function () { - const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0); + describe('minting during construction', function () { + it('events are emitted at construction', async function () { + let first = offset; - expect(await this.token.$_exists(tokenId)).to.be.equal(false); + for (const batch of batches) { + if (batch.amount > 0) { + await expectEvent.inConstruction(this.token, 'ConsecutiveTransfer', { + fromTokenId: web3.utils.toBN(first), + toTokenId: web3.utils.toBN(first + batch.amount - 1), + fromAddress: constants.ZERO_ADDRESS, + toAddress: batch.receiver, + }); + } else { + // expectEvent.notEmitted.inConstruction only looks at event name, and doesn't check the parameters + } + first += batch.amount; + } + }); - expectEvent(await this.token.$_mint(user1, tokenId), 'Transfer', { - from: constants.ZERO_ADDRESS, - to: user1, - tokenId: tokenId.toString(), + it('ownership is set', async function () { + const owners = [ + ...Array(offset).fill(constants.ZERO_ADDRESS), + ...batches.flatMap(({ receiver, amount }) => Array(amount).fill(receiver)), + ]; + + for (const tokenId in owners) { + if (owners[tokenId] != constants.ZERO_ADDRESS) { + expect(await this.token.ownerOf(tokenId)).to.be.equal(owners[tokenId]); + } + } + }); + + it('balance & voting power are set', async function () { + for (const account of accounts) { + const balance = batches + .filter(({ receiver }) => receiver === account) + .map(({ amount }) => amount) + .reduce((a, b) => a + b, 0); + + expect(await this.token.balanceOf(account)).to.be.bignumber.equal(web3.utils.toBN(balance)); + + // If not delegated at construction, check before + do delegation + if (!delegates.includes(account)) { + expect(await this.token.getVotes(account)).to.be.bignumber.equal(web3.utils.toBN(0)); + + await this.token.delegate(account, { from: account }); + } + + // At this point all accounts should have delegated + expect(await this.token.getVotes(account)).to.be.bignumber.equal(web3.utils.toBN(balance)); + } }); }); - it('cannot mint a token that has been batched minted', async function () { - const tokenId = batches.reduce((acc, { amount }) => acc + amount, 0) - 1; + describe('minting after construction', function () { + it('consecutive minting is not possible after construction', async function () { + await expectRevert( + this.token.$_mintConsecutive(user1, 10), + 'ERC721Consecutive: batch minting restricted to constructor', + ); + }); - expect(await this.token.$_exists(tokenId)).to.be.equal(true); + it('simple minting is possible after construction', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, offset); - await expectRevert(this.token.$_mint(user1, tokenId), 'ERC721: token already minted'); + expect(await this.token.$_exists(tokenId)).to.be.equal(false); + + expectEvent(await this.token.$_mint(user1, tokenId), 'Transfer', { + from: constants.ZERO_ADDRESS, + to: user1, + tokenId: tokenId.toString(), + }); + }); + + it('cannot mint a token that has been batched minted', async function () { + const tokenId = batches.reduce((acc, { amount }) => acc + amount, offset) - 1; + + expect(await this.token.$_exists(tokenId)).to.be.equal(true); + + await expectRevert(this.token.$_mint(user1, tokenId), 'ERC721: token already minted'); + }); + }); + + 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, tokenId, { from: user1 }); + + 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(tokenId, { from: user1 }), 'Transfer', { + from: user1, + to: constants.ZERO_ADDRESS, + tokenId, + }); + + await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID'); + + expectEvent(await this.token.$_mint(user2, tokenId), 'Transfer', { + from: constants.ZERO_ADDRESS, + to: user2, + tokenId, + }); + + expect(await this.token.ownerOf(tokenId)).to.be.equal(user2); + }); + + it('tokens can be burned and re-minted #2', async function () { + 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'); + + // mint + await this.token.$_mint(user1, tokenId); + + expect(await this.token.$_exists(tokenId)).to.be.equal(true); + expect(await this.token.ownerOf(tokenId), user1); + + // burn + expectEvent(await this.token.$_burn(tokenId, { from: user1 }), 'Transfer', { + from: user1, + to: constants.ZERO_ADDRESS, + tokenId, + }); + + expect(await this.token.$_exists(tokenId)).to.be.equal(false); + await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID'); + + // re-mint + expectEvent(await this.token.$_mint(user2, tokenId), 'Transfer', { + from: constants.ZERO_ADDRESS, + to: user2, + tokenId, + }); + + expect(await this.token.$_exists(tokenId)).to.be.equal(true); + expect(await this.token.ownerOf(tokenId), user2); + }); }); }); - - describe('ERC721 behavior', function () { - it('core takes over ownership on transfer', async function () { - await this.token.transferFrom(user1, receiver, 1, { from: user1 }); - - expect(await this.token.ownerOf(1)).to.be.equal(receiver); - }); - - it('tokens can be burned and re-minted #1', async function () { - expectEvent(await this.token.$_burn(1, { from: user1 }), 'Transfer', { - from: user1, - to: constants.ZERO_ADDRESS, - tokenId: '1', - }); - - await expectRevert(this.token.ownerOf(1), 'ERC721: invalid token ID'); - - expectEvent(await this.token.$_mint(user2, 1), 'Transfer', { - from: constants.ZERO_ADDRESS, - to: user2, - tokenId: '1', - }); - - expect(await this.token.ownerOf(1)).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)); - - expect(await this.token.$_exists(tokenId)).to.be.equal(false); - await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID'); - - // mint - await this.token.$_mint(user1, tokenId); - - expect(await this.token.$_exists(tokenId)).to.be.equal(true); - expect(await this.token.ownerOf(tokenId), user1); - - // burn - expectEvent(await this.token.$_burn(tokenId, { from: user1 }), 'Transfer', { - from: user1, - to: constants.ZERO_ADDRESS, - tokenId, - }); - - expect(await this.token.$_exists(tokenId)).to.be.equal(false); - await expectRevert(this.token.ownerOf(tokenId), 'ERC721: invalid token ID'); - - // re-mint - expectEvent(await this.token.$_mint(user2, tokenId), 'Transfer', { - from: constants.ZERO_ADDRESS, - to: user2, - tokenId, - }); - - expect(await this.token.$_exists(tokenId)).to.be.equal(true); - expect(await this.token.ownerOf(tokenId), user2); - }); - }); - }); + } 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, 0, [], [user1], ['5001']), 'ERC721Consecutive: batch too large', ); });