Recommended improvement to ERC721Consecutive (#3712)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

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(

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
+ * This extension allows the minting of large batches of tokens, during contract construction only. For upgradeable
- * limited to 5000 tokens at a time to accommodate off-chain indexers.
+ * 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
+     * Requirements:
-     * construction emits a `Transfer` event, which is not the case of mints performed using this function.
      *
-     * WARNING: Consecutive mint is limited to batches of 5000 tokens. Further minting is possible from a contract's
+     * - `batchSize` must not be greater than {_maxBatchSize}.
-     * point of view but would cause indexing issues for off-chain services.
+     * - 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);

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);
+      });
     });
   });