Merge branch 'master' into next-v5.0

frangio · frangio · commit 4c713f8cea2c · 2023-05-30T17:02:47.000-03:00
diff --git a/.changeset/spotty-hotels-type.md 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
@@ -11,13 +11,18 @@ import "../../token/ERC721/extensions/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
@@ -20,8 +20,8 @@ import "../../../utils/structs/Checkpoints.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
@@ -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

-Original file line number
+Diff line change
@@ @@ -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`.