Ability to set starting token id for ERC721Consecutive

[kfishnchips]

Many of our contracts start with token id 1, ERC721Consecutive currently assumes the starting token id to be zero.

Add a constructor which allows setting the starting token id.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 7959df3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Amxx]

Hello @kfishnchips

There is something troublesome with you implementation: _ownerOf will revert for tokenId below the _startingTokenId. This causes the corresponding tokens to not be mintable at all, including by _mint later in the lifecycle of the contract.

Additionally, reading _startingTokenId makes all token operation that read the owner cost one more sload.

IMO, if this is a feature we want, we should implement it by allowing _mintConsecutive to address(0):

• we would not call the hooks
• we would not increase the balance
• we would not emit an event
• we would still push a "checkpoint" to the ownership trace

That way you could create gaps in your allocation, not only before the first _mintConsecutive but also between multiple _mintConsecutive operation.

[kfishnchips]

Hello @kfishnchips

There is something troublesome with you implementation: _ownerOf will revert for tokenId below the _startingTokenId. This causes the corresponding tokens to not be mintable at all, including by _mint later in the lifecycle of the contract.

Additionally, reading _startingTokenId makes all token operation that read the owner cost one more sload.

IMO, if this is a feature we want, we should implement it by allowing _mintConsecutive to address(0):

• we would not call the hooks
• we would not increase the balance
• we would not emit an event
• we would still push a "checkpoint" to the ownership trace

That way you could create gaps in your allocation, not only before the first _mintConsecutive but also between multiple _mintConsecutive operation.

Was it naive from me to think that minted tokens would only have token ids larger than the batch? Now that I think of it, you could mint batches with gaps and then have _mint fill those gaps.

[kfishnchips]

mintConsecutive to address(0) would effectively "burn" the unwanted token ids, makes sense

[kfishnchips]

@Amxx is this last commit somewhere along the lines of what you were thinking?

[Amxx]

Thanks for changing your PR so fast. Usually we discuss the feature before going into an actual implementation.

@frangio how do you feel about it?

[Amxx]

Tests needs more checks, but please wait until we discuss that with @frangio and @ernestognw before putting work into it.

[frangio]

Allowing batches to start with a larger token id makes sense to me.

However, I would not reuse _mintConsecutive for this. I would instead introduce a new function _offsetConsecutive (or similar name) that would only create the checkpoint. @Amxx what do you think?

What are the performance implications of this implementation approach?

[Amxx]

_offsetConsecutive works for me, provided its documented well.

The performance implication is that the the trace array will be longer, making ownerOf lookup more expensive (growth is logarithmic in the number of checkpoints)

[frangio]

That seems like a big price to pay for a feature that appears to be implementable essentially for free with a single immutable variable (or pure virtual function) to mark the start of the consecutive mints. Is there a reason I'm not seeing why that wouldn't work?

It's not exactly the same as _offsetConsecutive because it doesn't allow leaving gaps in between batches, but the feature that was requested by @kfishnchips was just to set the starting id.

[Amxx]

That seems like a big price to pay for a feature that appears to be implementable essentially for free with a single immutable variable (or pure virtual function) to mark the start of the consecutive mints. Is there a reason I'm not seeing why that wouldn't work?

See the first part of this message.

TLDR:

• Depending on how the offset is implemented, we must make sure that all tokenId in the range are not blocked forever, and can later be minted using the normal _mint function. Also, I expect calls to _ownerOf to not revert. It might be possible to implement that cleanly, but the first draft of this PR was not clean in that regard.

It's not exactly the same as _offsetConsecutive because it doesn't allow leaving gaps in between batches, but the feature that was requested by @kfishnchips was just to set the starting id.

While we are at it, don't we want to support gaps ? That sounds like a reasonable feature.

[kfishnchips]

From a user standpoint, I think most comfortable solution would be to have a virtual function that I can override to specify a starting token id. I'm not sure that this contract is scoped only for such as use case so I understand the discussion around creating gaps.

[frangio]

Gaps sound "reasonable" but no one has requested it and I feel that we should favor the more efficient option here.

[Amxx]

Gaps sound "reasonable" but no one has requested it and I feel that we should favor the more efficient option here.

To me, it felt efficient in the sens that it reuses existing logic, and doesn't add more logic to this contract.

An immutable initial offset might be more gas effective, but it also adds code complexity.

[frangio]

Generally I agree but when we're talking about removing SSTORE/SLOAD, I think additional code complexity is justifiable. In particular this contract has the specific goal of being efficient.

[Amxx]

Ok @kfishnchips lets go back to implementing this with a

function _firstConsecutiveId() internal view virtual returns (uint96) { return 0; }

We need to make sure that _ownerOf doesn't revert for tokens before the first batch

[ernestognw]

Just leaving some small suggestions, but I agree with the approach of an overrideable function so far.

[kfishnchips]

Ok @kfishnchips lets go back to implementing this with a

function _firstConsecutiveId() internal view virtual returns (uint96) { return 0; }

We need to make sure that _ownerOf doesn't revert for tokens before the first batch

Wouldn't we need to revert for _ownerOf of tokens before the starting token id? Say if the _firstConsecutiveId() returns 1, would checking ownership of token id 0 revert?

[kfishnchips]

@ernestognw @Amxx I believe that what was requested is ready, additional tests using bound and the internal overridable function.

[ernestognw]

We still need to address the consistency of the range checks when there's a _firstConsecutiveId() and add tests for that. Currently, although the _maxBatchSize() is 5000, there are missing edge cases to consider:

1. What happens if a regular _mint(...) is before the _firstConsecutiveId()?
2. Are we testing correctly the effects on _afterTokenTransfer and ownerOf(...)?

I added detailed comments with my thoughts, but still, thank you @kfishnchips for the fast response.

[ernestognw]

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 burned tokenIds

For example:

1. Someone replaces _firstConsecutiveId() with a 5
2. They consecutively mint tokenIds from 5 to 9 (5 mints)
3. After that, someone else mints the token 2 via normal minting
4. Then, they burn it, and the token will make it to the _sequentialBurn on the ownerOf(...) method, which is not what we want

I think this is what we should aim for:

Suggested change

	return exists ? latestId + 1 : _firstConsecutiveId();
	return exists ? latestId + 1 - _firstConsecutiveId() : 0;

[kfishnchips]

Would it make more sense for someone to consecutively mint token 10 instead of 2? I was thinking that there could be no minting below the first consecutive id.

[kfishnchips]

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...

    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);
    }

[Amxx]

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.

[Amxx]

note that mint already check if the tokenId has an owner, and revert if that is the case.

[kfishnchips]

I see, I'm still wrapping my head around the internals of this but I think I got it.

[ernestognw]

Why is this breaking? If it's breaking because of the ownerOf range check, we definitely need to address that.

[kfishnchips]

The comment about the breaking test is from a previous commit which I still can't decipher

[Amxx]

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 in ERC721ConsecutiveTarget , you should make sure it is reflected here.

[kfishnchips]

Ah, understood. Thanks for clarifying!

[kfishnchips]

    function _ownerOf(uint256 tokenId) internal view virtual override returns (address) {
        address owner = super._ownerOf(tokenId);

        // If token is owned by the core, or beyond consecutive range, return base value
        if (owner != address(0) || tokenId > (uint256(type(uint96).max) + _firstConsecutiveId()) || tokenId < _firstConsecutiveId()) {
            return owner;
        }

        // Otherwise, check the token was not burned, and fetch ownership from the anchors
        // Note: no need for safe cast, we know that tokenId <= type(uint96).max
        return _sequentialBurn.get(tokenId) ? address(0) : address(_sequentialOwnership.lowerLookup(uint96(tokenId)));
    }

if address owner = super._ownerOf(tokenId); returns address(0) shouldn't we revert with "ERC721: invalid token ID"?

    function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
        _requireMinted(tokenId);

        string memory baseURI = _baseURI();
        return bytes(baseURI).length > 0 ? string(abi.encodePacked(baseURI, tokenId.toString())) : "";
    }

for some reason calling tokenURI with any token id is returning a string even for non existent IDs, I suspect it's because _exists calls _ownerOf from ERC721Consecutive since we have it overriden

function _exists(uint256 tokenId) internal view virtual returns (bool) {
        return _ownerOf(tokenId) != address(0);
    }

------------ edit

This is how we solved it

/**
  * @dev Reverts if the tokenId has not been minted yet.
  */
function _requireMinted(uint256 tokenId) internal view override {
    require(tokenId >= _firstConsecutiveId() && tokenId < _nextTokenId, "ERC721: invalid token ID");
}