Start Sequentially Minting at _startTokenId() instead of hardcoded 0

[peetzweg]

As described in #45 NFTs with token ID 0 can cause some headaches for developers down the line.

I renamed the currentIndex to _nextTokenId as this describes its use better as well it's an internal variable. But let me know your thoughts about it.
During development I stumbled over this function declaration
function getOwnershipAt(uint256 index) in ERC721AExplicitOwnershipMock.sol
For me it was a bit misleading. I expected _ownership to be an multidimensional array containing the history of the ownership of an NFT. However, it's a flat mapping between basically the tokenId to the TokenOwnership. I would suggested renaming the index parameter to tokenId.
https://github.com/chiru-labs/ERC721A/blob/main/contracts/mocks/ERC721AExplicitOwnershipMock.sol#L19-L21

[MrMcGoats]

You should probably add .vscode to the git ignore

[peetzweg]

@MrMcGoats Done. Thanks for pointing that out.

I've added new tests as well. One for tokenOfOwnerByIndex is currently failing.
Any suggestions how to adapt it?

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 is ready for review, happy to get some feedback and merge it if approved. 👍

[jpegdigital]

Hi, thought of one more edge case with _exists(). I think return tokenId < currentIndex; fails when checking tokenId 0 or the last tokenId? Maybe something like this to fix return tokenId > 0 && tokenId <= currentIndex;

I'm surprised there aren't existing tests already that fail for the boundaries. Haven't had a chance to run the test suite though.

[peetzweg]

@Vectorized Thanks for the review. The change requests check out, will incorporate them probably today. Not sure what you mean by Can be unchecked.? I guess it relates to the option to make the starting index flexible?
I wonder if you think it should start at 0 or 1 or if even other starting indexes should be valid as well.

@jpegdigital good call, I've added some _exists tests and adapted the function my changes yesterday already. ✌️
https://github.com/chiru-labs/ERC721A/pull/66/files#diff-833552e5bd4a4be3ada89b7874eedc6762fb91ed0890dda22e82a1936e6d60d8R280-R282

[Vectorized]

@Vectorized Thanks for the review. The change requests check out, will incorporate them probably today. Not sure what you mean by Can be unchecked.? I guess it relates to the option to make the starting index flexible?

Wrap those lines in unchecked {} blocks for gas savings, since they cannot overflow and underflow.

See: https://gist.github.com/Vectorized/e6f47b0c1de008bb5983852224e41f69

In my opinion, having the flexibility to start at either 0 and 1 should be enough for most practical purposes.
Being able to start at any index is good to have, but not if it comes with significantly more code / gas costs.
The function override approach should be able to allow starting at any index with minimal extra code / gas costs.

ERC721A/contracts/ERC721A.sol

Line 403 in a4b90eb

if (_exists(nextTokenId)) {

^ For this line in the _transfer function, you can replace _exists(nextTokenId) with nextTokenId < _nextTokenId to save more gas.

[peetzweg]

@Vectorized thanks for the gist, it helped a lot. I've adapted my fork to your suggestions. Works fine now. It needed minor adjustments.
I've brought back the original test files and plainly copied them for new mock contracts which uses the override to set the starting token id to one. Not sure how I feel about the copy-pasted code, not sure how to do it more elegantly atm.

[ahbanavi]

@Pczek I've opened a PR peetzweg#3 to your branch for adding _totalMinted function (#124).
CC: @cygaar @Vectorized

[cygaar]

Thanks for your PR @ahbanavi let's wait for that to get merged and then I'll do an in-depth review of this PR

[peetzweg]

@Vectorized @ahbanavi just merged your PR peetzweg#3

Test seem to run fine, I haven't adapted anything else since the PR of @ahbanavi .

Anything left to do? I saw you opened up a new PR @Vectorized ?

Sorry for being a bit unresponsive lately, having a few deliverables this week. ✌️

[Vectorized]

@Pczek Welcome back!

Added some minor comment and README edits and opened a PR to your fork.

I'll close #129 first.

Let's wait for @cygaar.

[cygaar]

Going to review this today

[cygaar]

The actual code looks fine, @Vectorized I think we can make the readme/comment changes in another PR. I'm more concerned about test quality and reducing copy-pasted code

[peetzweg]

@Vectorized @cygaar please check my recent changes to make a parameterized test. How do you like it this way?
If this is fine for you we can adapt the rest of the duplicated tests for the ExplicitOwnership extension as well.

2ba810d (#66)

[Vectorized]

@Vectorized @cygaar please check my recent changes to make a parameterized test. How do you like it this way? If this is fine for you we can adapt the rest of the duplicated tests for the ExplicitOwnership extension as well.

2ba810d (#66)

Looks good so far. Just go ahead with ExplicitOwnership too.

[cygaar]

LGTM. @Pczek parameterize the ownership explicit test and this is good to go. Appreciate all the work you and everyone has put in to get this out

[peetzweg]

Alright, I've parameterized now the following tests:
ERC721A.test
ERC721AOwnersExplicit.test
ERC721ABurnable.test

All of them now also run their test suite for a contract which is modified with the StartTokenIdHelper.sol contract to set the override the _startTokenId() function dynamically.

Afaik no further tasks are open on this PR until another review or it's ready to be merged.

CC @Vectorized @cygaar