-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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.
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
Changes from all commits
b693f7f
bf8c076
31a565b
b651b93
854b461
1c7570d
ab40061
1835727
1fdcbff
44cc563
3f8b0f9
573fa0d
5d9b21b
0fcb048
dd1d6bf
361aa8d
114a3f6
6b4e4ee
78ed610
7959df3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff 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`. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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) { | ||||||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| 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(); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think For example:
I think this is what we should aim for:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense for someone to consecutively mint token
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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);
}
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this breaking? If it's breaking because of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment about the breaking test is from a previous commit which I still can't decipher
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test tries to mint a batch that is too large. too large means bigger than
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, understood. Thanks for clarifying! |
||
| 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]); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.