diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index a8e5aa4ab..0facc3dbd 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -488,11 +488,9 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { ) internal virtual {} /** - * @dev Hook that is called before consecutive token transfers. + * @dev Hook that is called before "consecutive token transfers" as defined in ERC2309 and implemented in + * {ERC721Consecutive}. * Calling conditions are similar to {_beforeTokenTransfer}. - * - * The default implementation include balances updates that extensions such as {ERC721Consecutive} cannot perform - * directly. */ function _beforeConsecutiveTokenTransfer( address from, @@ -509,7 +507,8 @@ contract ERC721 is Context, ERC165, IERC721, IERC721Metadata { } /** - * @dev Hook that is called after consecutive token transfers. + * @dev Hook that is called after "consecutive token transfers" as defined in ERC2309 and implemented in + * {ERC721Consecutive}. * Calling conditions are similar to {_afterTokenTransfer}. */ function _afterConsecutiveTokenTransfer( diff --git a/contracts/token/ERC721/extensions/ERC721Consecutive.sol b/contracts/token/ERC721/extensions/ERC721Consecutive.sol index 15ecdd986..42b447fd3 100644 --- a/contracts/token/ERC721/extensions/ERC721Consecutive.sol +++ b/contracts/token/ERC721/extensions/ERC721Consecutive.sol @@ -5,17 +5,17 @@ pragma solidity ^0.8.0; import "../ERC721.sol"; import "../../../interfaces/IERC2309.sol"; import "../../../utils/Checkpoints.sol"; -import "../../../utils/math/SafeCast.sol"; import "../../../utils/structs/BitMaps.sol"; /** * @dev Implementation of the ERC2309 "Consecutive Transfer Extension" as defined in * https://eips.ethereum.org/EIPS/eip-2309[EIP-2309]. * - * This extension allows the minting of large batches of tokens during the contract construction. These batches are - * limited to 5000 tokens at a time to accommodate off-chain indexers. + * This extension allows the minting of large batches of tokens, during contract construction only. For upgradeable + * contracts this implies that batch minting is only available during proxy deployment, and not in subsequent upgrades. + * These batches are limited to 5000 tokens at a time by default to accommodate off-chain indexers. * - * Using this extension removes the ability to mint single tokens during the contract construction. This ability is + * Using this extension removes the ability to mint single tokens during contract construction. This ability is * regained after construction. During construction, only batch minting is allowed. * * IMPORTANT: This extension bypasses the hooks {_beforeTokenTransfer} and {_afterTokenTransfer} for tokens minted in @@ -35,6 +35,18 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { Checkpoints.Trace160 private _sequentialOwnership; BitMaps.BitMap private _sequentialBurn; + /** + * @dev Maximum size of a batch of consecutive tokens. This is designed to limit stress on off-chain indexing + * services that have to record one entry per token, and have protections against "unreasonably large" batches of + * tokens. + * + * NOTE: Overriding the default value of 5000 will not cause on-chain issues, but may result in the asset not being + * correctly supported by off-chain indexing services (including marketplaces). + */ + function _maxBatchSize() internal view virtual returns (uint96) { + return 5000; + } + /** * @dev See {ERC721-_ownerOf}. Override that checks the sequential ownership structure for tokens that have * been minted as part of a batch, and not yet transferred. @@ -53,13 +65,18 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { } /** - * @dev Mint a batch of tokens of length `batchSize` for `to`. + * @dev Mint a batch of tokens of length `batchSize` for `to`. Returns the token id of the first token minted in the + * batch; if `batchSize` is 0, returns the number of consecutive ids minted so far. * - * WARNING: Consecutive mint is only available during construction. ERC721 requires that any minting done after - * construction emits a `Transfer` event, which is not the case of mints performed using this function. + * Requirements: * - * WARNING: Consecutive mint is limited to batches of 5000 tokens. Further minting is possible from a contract's - * point of view but would cause indexing issues for off-chain services. + * - `batchSize` must not be greater than {_maxBatchSize}. + * - The function is called in the constructor of the contract (directly or indirectly). + * + * CAUTION: Does not emit a `Transfer` event. This is ERC721 compliant as long as it is done outside of the + * constructor, which is enforced by this function. + * + * CAUTION: Does not invoke `onERC721Received` on the receiver. * * Emits a {ConsecutiveTransfer} event. */ @@ -70,7 +87,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { if (batchSize > 0) { require(!Address.isContract(address(this)), "ERC721Consecutive: batch minting restricted to constructor"); require(to != address(0), "ERC721Consecutive: mint to the zero address"); - require(batchSize <= 5000, "ERC721Consecutive: batch too large"); + require(batchSize <= _maxBatchSize(), "ERC721Consecutive: batch too large"); // hook before _beforeConsecutiveTokenTransfer(address(0), to, first, batchSize); @@ -99,7 +116,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { } /** - * @dev See {ERC721-_afterTokenTransfer}. Burning of token that have been sequentially minted must be explicit. + * @dev See {ERC721-_afterTokenTransfer}. Burning of tokens that have been sequentially minted must be explicit. */ function _afterTokenTransfer( address from, @@ -108,7 +125,7 @@ abstract contract ERC721Consecutive is IERC2309, ERC721 { ) internal virtual override { if ( to == address(0) && // if we burn - tokenId <= _totalConsecutiveSupply() && // and the tokenId was minted is a batch + tokenId < _totalConsecutiveSupply() && // and the tokenId was minted in a batch !_sequentialBurn.get(tokenId) // and the token was never marked as burnt ) { _sequentialBurn.set(tokenId); diff --git a/test/token/ERC721/extensions/ERC721Consecutive.test.js b/test/token/ERC721/extensions/ERC721Consecutive.test.js index de3af0a07..5e480abb8 100644 --- a/test/token/ERC721/extensions/ERC721Consecutive.test.js +++ b/test/token/ERC721/extensions/ERC721Consecutive.test.js @@ -122,7 +122,7 @@ contract('ERC721Consecutive', function (accounts) { expect(await this.token.ownerOf(1)).to.be.equal(receiver); }); - it('tokens can be burned and re-minted', async function () { + it('tokens can be burned and re-minted #1', async function () { expectEvent( await this.token.burn(1, { from: user1 }), 'Transfer', @@ -139,6 +139,39 @@ contract('ERC721Consecutive', function (accounts) { 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); + }); }); });