-
Notifications
You must be signed in to change notification settings - Fork 841
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
Non-sequential minting (a.k.a spot-minting) support #479
Conversation
Clever approach to implement this I think. I went through all the changes and didn't see any red flags. The only obvious thing that popped into my head is whether it makes sense to package spot + sequential mint functionality together in a single contract as I don't think a lot of mints will need both together. That being said, I do like having the capability now, and it added a lot less code to the base contract than I expected. Nicely done. |
@mvillere Thanks so much for your review. One possible use case is multi-chain NFTs. |
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 overall approach looks sound to me, however it does seem like this would be better off as an extension rather than be placed in the base ERC721A contract. The usage of this contract seems pretty niche as it's pretty rare to find a contract that needs to mint sequentially and at specific spots
Was initially thinking of a subclass that wraps the ERC721A's functions. Decided on the current approach after hard marinating over a week:
|
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 focused on reviewing the integrity of _packedOwnerships
data structure, and it looked good to me. My observations supporting the integirty are as follows:
_packedOwnerships
is divided into two parts for sequential and spot mintings. While the latter part doesn't hold some sequential properties of the former, they are used differently by identifying which part a given token id belongs to._currentIndex - 1
stays within_sequentialUpTo
, ensuring that sequential minting data doesn't spill over into the spot minting area.- The next slot initialization in transferFrom() and burn() doesn't occur for spot-minted tokens, as their
_BITMASK_NEXT_INITIALIZED
flag is always set true.
Minor comments are added below.
Could we consider implementing batch spot minting as a highly anticipated use case? Nice work. |
could be less fee for NFTs redeem |
I did not want to create new issue for this as I assume answer is clear, but I want to make sure by checking with you. As you mentioned I use To give you some view of my project architecture I use batchMint, batchTransfer and batchBurn (all sequential) for my There is also one more thing as your docs says Thanks in advance for any confirmations/explanation if I'm right or there is a way to solve those problems somehow |
This PR gives ERC721A the ability to mint tokens in non-sequential order.
Devs will need to override a new
_sequentialUpTo()
function to return a value less than2**256 - 1
to activate this feature.About:
_startTokenId() <= tokenId <= _sequentialUpTo()
_sequentialUpTo() < tokenId
.Zero-cost abstraction:
_sequentialUpTo() == type(uint256).max
(by default), all expressions ofx > _sequentialUpTo()
,x != _sequentialUpTo()
will evaluate to false on compile time, and the dead code will be removed. If you don't override_sequentialUpTo()
, you won't pay any extra gas.Notes:
ERC721AQueryable
can still be used with spot mints. ThetokensOfOwner
function is disabled if spot mints is enabled, as it would need to scan the entire uint256 range, which is unfeasible. ThetokensOfOwnerIn
function, however, is still available for use with spot mints, as the search range can be manually restricted. Added some minor optimizations to the scanning logic.Todo: