search

hashlips-lab / safe-nft-metadata-provider

A safe metadata provider for your NFT collection.
MIT License
74 stars 77 forks source link

Add COLLECTION_FIRST_INDEX env variable for a better isValidTokenId #34

Closed narghev closed 2 years ago

narghev commented 2 years ago

Hi,

The isValidTokenId does not work for collection whose first token ID is 0 because of the $tokenId > 0 check. Since more and more collections are starting to use ERC721A and the token count starts with 0 I think it adding a COLLECTION_FIRST_INDEX variable is useful.

This PR adds a COLLECTION_FIRST_INDEX with a default value of 1. The idea was to avoid hard-coding the values 0 or 1 as a first index that is why I made a generic variable that will work with any value. If you think the approach or the variable naming needs improvement let me know.

liarco commented 2 years ago

Hi, thank you very much for this contribution. I'm gonna review this as soon as possible.

Edit Just so you know, ERC721A supports custom start token ID since version 3.0.0, here you can see how I implemented this in our ERC721 collection project: https://github.com/hashlips-lab/nft-erc721-collection/blob/de0177bc76d4c0478470091a6a9d7ce56d7b2f2c/smart-contract/contracts/YourNftToken.sol#L101-L103

narghev commented 2 years ago

Yes, I actually saw it while doing some research for the PR, but some collections continue to start from 0 (either they do not know about the change or for some reason do not upgrade).

Let me know when you review the code. Thank you! 🤝

liarco commented 2 years ago

I'm sorry @narghev for the huge delay but I finally had the chance to work on a complete redesign of this tool and it's almost ready now. I'm gonna close this PR since #48 introduces the option to customize the start token ID.

Thank you for taking time to share this, I took this PR into account when I picked the features for V2.