cw721 token id (string) vs erc721 token id (uint256)

[taitruong]

Along with #113 about rejecting leading/trailing whitespaces, imo it also makes sense even limiting token id to Uint256. Why?

Because the other of all NFT spec is ERC721, stating token id being of type uint256: https://eips.ethereum.org/EIPS/eip-721

NFTs will be able to be IBC transferred to other Cosmos chains based on ICS721 spec. It is a matter of time they will be able to be transferred to Ethereum based chains. For this CW721 should be compliant to ERC721.

[LeTurt333]

Personally I'd go even further and say I think the spec should migrate to u32 or u64 for token IDs, or even an agreed upon UUID version (might have trouble with the RNG though)

Token ID's shouldn't be used as an arbitrary name, they should be used as dev friendly UUIDs that can be easily interacted with from any client. Anything else should be included in Metadata or should require a custom fork of cw721-base

Please correct me if I'm wrong but leaving them as Strings means that String::from("\\xe2\\x28\\xa1") and String::from("â(¡") are both totally valid token IDs, but anyone trying to work with them in a JS client has to jump through a bunch of hoops to avoid collision

[Art3miX]

This is actually a big problem for collections that doesn't use numbers for token_id, they wont be able to bridge to Ethereum, and they might not be able to use some of the bridges in general.

I do believe this breaking change is needed ASAP, as @LeTurt333 said, token_id should be a number for developers, not a way to pass data, any data should be placed somewhere else.

Stargaze minters are already using token_id as a number (u64), so this wont be a breaking change for stargaze for example, I assume most minters are doing pretty much the same.

We can also enforce it internally?, basically accept a token_id as string, try to parse to Uint256, save it internally as Uint256, and export (query) as a string, this will reduce breakage for any collection that already uses numbers.

1 important note tho is tests, some tests might be using some random string as token_id, this will break those tests

Wonder what others are thinking? @JakeHartnell @jhernandezb @shanev @larry0x

[JakeHartnell]

I'm in support, we should think about other major changes we might want and do a major release.

[yubrew]

There are a number of NFT collections, including NFTs IBC transferring from other EVM chains that have native string token_ids. Stargaze is considering switching from u64 to string so there are less compatibility problems between the different token ids on marketplace, minter, IBC and other contracts. If we do enforce string token_ids, just need to make sure that duplicate token ids and other edge cases are handled properly. Also like @LeTurt333 suggested, maybe a standardized parser / validator, stripping invalid chars, leading/trailing whitespace, etc.

…

On Wed, Jun 14, 2023 at 3:44 PM Jake Hartnell ***@***.***> wrote: I'm in support, we should think about other major changes we might want and do a major release. — Reply to this email directly, view it on GitHub <#114 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADRXUSJEYWBAZ2J3WUB6GTXLIIDTANCNFSM6AAAAAAVQ4QDB4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[JakeHartnell]

Good context @yubrew.

maybe a standardized parser / validator, stripping invalid chars, leading/trailing whitespace, etc.

Could make an excellent package or util to add to cw721 perhaps?

[taitruong]

Reraising this issue. In next cw721 version we should change this to be uint128. Only question is then whether old versions can be migrated then? Like we can use SHA-256, truncate it and use a subset of the hash to fit into 128 bits. The other option is that it new version won't be backward-compatible.