WIP: Add support for out of order minting

[dozer-eth]

Hi! Wanted to get all of your thoughts on whether some version of these changes/ideas make sense to merge into ERC721A before writing more tests. Open to any and all feedback!

Summary

This PR adds new functions to allow batch minting of tokens out of order, while keeping existing sequential batch minting functions. The extra bookkeeping required to support non-sequential minting adds about 300-400 total gas to sequential minting functions (mintOne and mintTen add roughly the same total gas). Gas impact on other functions is minimal (I think it's actually less for every other function in unit tests).

Motivation

The sequential minting of ERC721A works great for most projects, since most mints just need to mint tokens from M to N in order. For certain drops though (especially secondary drops), out of order batch minting is required to achieve the maximum possible gas savings. Some examples:

• Chain Runners XR - this is a 20k 3D collection, where we wanted each holder of our 10k genesis collection to receive the corresponding 3D version with the same tokenId (e.g. holder of token ID 1 in the genesis collection could claim token ID 1 in the XR collection). In addition, we had a public mint of tokens 10,001-20,000 before opening up claims for tokens 1-10k. An early version of this PR is included in that contract (https://etherscan.io/address/0x4e1824ca2e3dcef21d8eabcf11ccd2b5fd46774b#code#F5#L1).
• Otherdeeds/Otherside - To start, contributors could claim tokens starting at id 30k. Next, the public sale continued after the last contributor claim token ID. Finally, BAYC and MAYC holders could claim token IDs 0-29,999.

Since low token IDs are valued more highly by some, it may not be too uncommon for other secondary drops to follow a similar pattern, granting low token IDs to existing holders and offering higher token ids in a public sale. In addition, if hyped projects like Otherdeeds are not able to take advantage of the gas savings of ERC721A (due to out of order token ID minting requirements), they'll resort to non-optimized ERC721 implementations that end up clogging Ethereum and costing users non-trivial amounts of gas money.

How?

The main idea behind these changes is to track one additional value (quantity) in the _packedOwnerships struct on mint, and update it on transfers and burns. In addition, we enforce a maxBatchSize since we can no longer rely on the invariant of having an explicit ownership record below an existing token id.

As an example, say we want to mint a batch of 3 tokens to address A starting at index 1. Additionally, we configure maxBatchSize=5. The (simplified) packed ownership records will now look like this:

0
1 - quantity=3, address A
2
3
4

Here's how existence/owner checks work for each token:

0 - No packed ownership within `tokenId - maxBatchSize` and `>= minimumTokenId`.  Does not exist.
1 - Packed ownership record with quantity=3.  1+3-1=3, which is >= this token id (1).  Exists and owned by A.
2 - Packed ownership at token 1 with quantity=3.  1+3-1=3, which is >= this token id (2).  Exists and owned by A.
3 - Packed ownership at token 1 with quantity=3.  1+3-1=3, which is >= this token id (3).  Exists and owned by A.
4 - Packed ownership at token 1 with quantity=3 (note that we search for packed ownerships within `tokenId - maxBatchSize` and `>= minimumTokenId`, so still evaluate the ownership at token id 1).  1+3-1=3, which is NOT >= this token id (4).  Does not exist.

Now let's say address A wants to transfer token 2 to address B. The packed ownership records will be updated to look like this:

0
1 - quantity=3, address A.  Note that this quantity does not need to be updated!
2 - quantity=1, address B
3 - quantity=1, address A
4

Let's look at the updated existence checks for each token:

0 - No packed ownership within `tokenId - maxBatchSize` and `>= minimumTokenId`.  Does not exist.
1 - Packed ownership record with quantity=3.  1+3-1=3, which is >= this token id (1).  Exists and owned by A.
2 - Packed ownership at token 2 with quantity=1.  2+1-1=2, which is >= this token id (2).  Exists and owned by B.
3 - Packed ownership at token 3 with quantity=1.  3+1-1=3, which is >= this token id (3).  Exists and owned by A.
4 - Packed ownership at token 3 with quantity=1.  3+1-1=3, which is NOT >= this token id (4).  Does not exist.

One important observation is that we don't actually need to update token id 1's quantity to maintain existence/ownership correctness (shout out to beans for noticing this 🙏). During a transfer, we will always explicitly set 1) the transferred token id's packed ownership quantity to 1 (token id 2 in this example), and 2) if not explicitly set, the next token id's packed ownership quantity to oldOwnershipTokenId+oldOwnershipQuantity-transferredTokenId-1 (1+3-2-1=1 in this example) if that value is > 0. With these explicit ownership updates, we will never run into a situation where token id 1's packed ownership is being used to determine ownership of a token it no longer should - we'll hit the packed ownership of either token id 2 or 3 before hitting token id 1's.

Limitations

• In the new minting functions that support a startTokenId, there are no checks to ensure a given token id range does not already exist (to save gas). Both the OpenZeppelin ERC721 and ERC721A implementations provide assurances that a token id doesn't exist before minting - OZ has an explicit check (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L281) and ERC721A implicitly enforces this by limiting minting to an increasing range. This makes these new functions somewhat unsafe. However, in the 2 examples above (Chain Runners XR and Otherdeeds), the calling contracts track which tokens can be minted separately (based on ownership in other contract(s)), so an existence check isn't necessary. Even so, this can be dangerous, so it might make sense to rename these functions to something that indicates no existence check is made.
• To minimize gas while supporting both the most common use-case (sequential minting) and non-sequential minting, I've packed 2 variables (currentIndex and mintCounter) into a single uint256. This means both of those variables are now 128 bits, so the supply is effectively capped at the max 128 bit number (vs the max 256 bit number). Even at 128 bits, this amount should be more than enough for most use cases. The ERC721 spec even mentions 128 bits is scalable enough: https://eips.ethereum.org/EIPS/eip-721.

Alternatives Considered

• Rather than keeping the existing sequential minting functions and adding new non-sequential mint functions, I considered removing the sequential minting functions and requiring implementing contracts to track/supply their own currentIndex. This is cleaner in some ways, but not as gas efficient as packing both currentIndex and mintCounter into a single uint256 - mintCounter needs to be tracked regardless, so requiring implementers to track their own currentIndex adds another uint256 SSTORE. Also sequential minting is by far the more common use-case, so I wanted to disrupt that API as little as possible.

[Vectorized]

I understand a lot of effort and thought has been spent on this PR.

However, I think this should be in a separate fork.

Imo, there is nothing wrong with using an extra SSTORE if you need to write store some on-chain metadata (e.g. the tier of the NFT) upon mint.

Personally, I’d just pack the tiers into the ownership data like in #323 if I really want to save on the SSTORE.

If the business requirement is to have different tokenId ranges for different tiers, I would recommend forking ERC721A or using other implementations. Or simply have two or more collections.

[0xBeans]

Great job @dozer-eth !

Can attest that the experience in batch claiming and minting was 👌. Even if this cant be directly added to ERC721A as a different version/"extension", having it as a separate fork would be cool for other projects to look into or use :)

[dozer-eth]

Thanks for taking a look @Vectorized!

Agree that if just storing tiers, storing them in extraData after #323 is merged makes sense.

This PR addresses the latter use-case though, allowing minting different tiers in both different token ranges and in random order. The new quantity value stored in packedOwnerships is just the mechanism that enables out of order minting logic.

Agree it may make sense to just have this be a fork, but wanted to clarify a couple things:

• The existing ERC721A sequential mint functions are all unchanged. This PR just adds some new minting functions that take in a startTokenId arg.
• Having said that, I wonder if that part is actually confusing. Maybe it's better to not support the existing sequential mint functions in a fork, and only support mint functions that take in a startTokenId arg. Didn't do that bc it's not as gas efficient if you do want sequential minting, but curious your thoughts.

In any case, I think the core question is whether ERC721A would benefit from having the extra functionality of being able to mint token batches in any order (without changing the existing sequential minting functionality, and without adding much gas). It's definitely not a common case (sequential is much more common), but it's popped up a couple times (with myself in Chain Runners XR and also in Otherdeeds). In those cases you end up having to roll your own or use a less gas-efficient implementation (e.g. OZ).

[Vectorized]

@dozer-eth

For the fork, remove the sequential mint functions for a cleaner API.

You will also need to warn the users about the extra bookkeeping they will need and the absence of safety checks for performance. Most users will just copy paste code without reading docs or comments, so… yeah.

[dozer-eth]

Thanks @Vectorized. In the fork, thinking I'll add a (costly) safety check by default to check for existing tokens in the desired mint range, with a flag to bypass. That way the copy pasta case is safe, but advanced users can bypass for gas savings.

Will also remove the sequential mint functions as you suggest. That'll add an extra sstore for implementors that have a sequential minting range (vs in this PR, a sequential counter is packed into the mint counter uint256), but agree it's a cleaner API. Even with the extra sstore, should still be cheaper than batch minting in other non-sequential minting contracts like OZ.

Will close this PR out - would you be up to take a look at the fork when it's ready?

[Vectorized]

@dozer-eth ok