Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Ability to set starting token id for ERC721Consecutive #4097
Ability to set starting token id for ERC721Consecutive #4097
Changes from 16 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
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
0 to _firstConsecutiveId()
should be accounted for the_totalConsecutiveSupply
, it affects line 135, which will be excluding the after hook for real burnedtokenId
sFor example:
_firstConsecutiveId()
with a5
tokenId
s from 5 to 9 (5 mints)2
via normal minting_sequentialBurn
on theownerOf(...)
method, which is not what we wantI think this is what we should aim for:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense for someone to consecutively mint token
10
instead of2
? I was thinking that there could be no minting below the first consecutive id.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
_mint
function be a viable option? It might be too inefficient...There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
_mint
a token that was first minted as part of a batch then burnt.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this breaking? If it's breaking because of the
ownerOf
range check, we definitely need to address that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
_maxBatchSize
. Since the contract is not yet constructed, we assume the value is 5000. If you ever override_maxBatchSize
inERC721ConsecutiveTarget
, you should make sure it is reflected here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, understood. Thanks for clarifying!