search

Vectorized / dn404

Implementation of a co-joined ERC20 and ERC721 pair.
MIT License
476 stars 158 forks source link

🐞 [WIP] Fix token ID increments #134

Closed onchainguy-btc closed 6 months ago

onchainguy-btc commented 6 months ago

Description

I assumed NFT IDs would increment as described in #51. Meaning: 

User A mints 3 NFTs = Token IDs 1,2,3 are minted
User A burns 1 NFT = Token ID 1 is burned
User A mints 1 NFT = Token ID 4 is minted

This behaviour unless addToBurnedPool is returning true. My change makes the token IDs increment like that.

I'm not 100% sure what the desired way to increment token IDs is but I think not automatically recycling helps for different use cases.

Notes

The testMixed is currently failing. Will fix that if my assumption is right.

onchainguy-btc commented 6 months ago

Ok actually while creating more tests I realized the "fix" does not work for all scenarios. Still I wonder what the actual desired behavior is and if there is an easy configuration way to not re-use token IDs of burned tokens. Maybe we could have a mintNFTs(address to, uint32[] tokenIds) function.

Vectorized commented 6 months ago

The last accquired token ID will be the first token ID to be burned.

This is very intentional for gas efficiency and good enough "rerolling".

Long story short, a queue for owned IDs will cause a lot of implementation problems, and users will most likely not care enough to make it worth it. A stack requires only one variable (top of the stack). A queue requires two (head and tail). Changing the stack into a queue will have cascading performance penalities: structs have to be used more extensively to avoid stack too deep, more storage reads/write are needed.

A stack also provides some desirable qualities over a queue:
Consider you have an address with some NFTs you want to keep; you can still use the same address to do some hot trading without auto-transferring the NFTs out.

Might consider including if I have a strong guarantee that changing a stack to a queue will > 2x the market cap of future DN404s.