Question/Change Request: Init currentIndex with 1 instead of 0

[peetzweg]

We've been using ERC721A in our latest NFT drop. It worked great, however, having the token indexes start 0 caused some edgecases in our other contracts for the token with the id 0.

For example we have a simple staking contract were we map the owner address to their staked token. Each address can only stake a single token so its a mapping(address => uint256) ownerToToken. However, each address of the map get's initialized with 0 by solidity afaik. So doing stuff like require(ownerToToken[msg.sender] == 0, "Already staked a token"); does not work properly when supporting the token id 0 in the ERC721A contract.

[chiru-labs]

Yup this is on the roadmap. Feel free to open a PR if you have one ready!

[peetzweg]

👍 Started working on a PR.

[peetzweg]

Will leave the PR link here as a reference. It's not finished yet. Had only limited time today.
Let me know what you think about the renaming of the variable and it's use.

#66

[peetzweg]

PR is ready for review. ✌️
Tests pass, although a lot of them needed some adaptations to succeed. 😅
#66

[ottogutierrez]

Quick question about this, if this PR gets approved, the default behaviour would be for the index to start at 1 right?

[VRPunk9036]

Wow great work @Pczek , I will do some tests with this to use on one of my projects! Collections starting with 1 are a lot less confusing

[peetzweg]

Quick question about this, if this PR gets approved, the default behaviour would be for the index to start at 1 right?

Yes. Having token id 0 can lead to confusion in smart contracts as the initial value for uint is 0.

[jpegdigital]

Looking to implement this in a project and had a few questions:

Does function tokenByIndex(uint256 index) need to be adjusted? Token at index 0 is now tokenId 1.
Something like return index + 1.

Also ownershipOf checks all the way down to zero, should change to curr > 0?

for (uint256 curr = tokenId; curr >= 0; curr--) {
    TokenOwnership memory ownership = _ownerships[curr];
    if (ownership.addr != address(0)) {
        return ownership;
    }
}

[peetzweg]

@jpegdigital yes you are right. I've adapted it in my PR.

Fixing this, i've stumbled over the tokenOfOwnerByIndex function. Which also seems to need an adaptation. I've added a test for it in my PR however, it's failing and I'm not yet sure what do adapt. Not yet had the time to wrap my head around the ownership mapping in the code. 🤔

https://github.com/chiru-labs/ERC721A/blob/main/contracts/ERC721A.sol#L85-L108

[peetzweg]

Fixed the tokenOfOwnerByIndex function now as well. I wrote some test for the. The PR #66 is ready for review, happy to get some feedback and merge it if approved. 👍

[peetzweg]

After some rework #66 is again ready for a second review before merging it.

[kyokosdream]

Since the Hashlips library which has a large following and is used by many beginners to help programatically generate artwork begins indexing at 1, we followed the OPs suggestion and begin indexing at 1 as well.

uint256 internal _currentIndex = 1;

It seems to work fine for us but that may be because we removed ERC721Enumerable from ERC721A and so we don't encounter any issues with the aforementioned tokenOfOwnerByIndex and tokenByIndex.

Because of the fact that the _currentIndex variable is now always 1 more than the _totalSupply() we modified the function to this:

    function _totalSupply() internal view returns (uint256) {
        unchecked {
             return _currentIndex - 1;
        }
    }

Please let me know if there's any problems with these modifications. Our contract implements a previous version of ERC721A that did not include the burn functionality which is why you don't see burnCounter here.

I'll be updating our contract that implements ERC721A to the recent changes made over the past 2 weeks but we already deployed a contract with the above modifications in production and just want to make sure there's no serious problem.

[peetzweg]

@kyokosdream Which specific released version did you use to modify with your code?
You can see all the changes we've made to the most recent version in this PR:
https://github.com/chiru-labs/ERC721A/pull/66/files#diff-833552e5bd4a4be3ada89b7874eedc6762fb91ed0890dda22e82a1936e6d60d8R339-R342

One thing which comes to mind, which could cause problems is the internally used _exists() function. Haven't tracked it completely where it could lead to some problems. It might be worth exploring in your modified contract. 🤔

[kyokosdream]

I'm going to integrate the latest changes to the repo (re-entrant safe mint and start at index 1) regardless of whether it's merged or not since we need to put this into production ASAP.

We've edited our contract to use the latest version of ERC721A as of today.

The only changes it makes to ERC721A is that we include a placeholderURI variable for delayed reveal functionality, update tokenURI() function to append {tokenId}.json to the baseURI and hardcodes the currentIndex to be 1 since our users will not be customizing the start index as proposed in #129 / #66

@Vectorized Should I open a pull request for projects.md or request one of the maintainers to add our contract to the projects.md?

To be honest, I've been the only technical member of my company's founding team so I am used to using git alone and not sure of proper protocol for pull requests etc.

[Vectorized]

@kyokosdream Open a PR for projects.md.

[cygaar]

This has now been addressed in #66