Fixing walletOfOwner behavior with burned tokens

[tomvanleeuwennl]

This is my proposal for the walletOfOwner function to act in the right way when having burned tokens.

Fixes #128

[liarco]

Thank you @tomvanleeuwennl, for this contribution.

It looks good to me, let me know what you think about the changes I requested (especially the one about the "burned" status). If you apply those, then I can test this implementation locally to ensure everything works as expected with a basic contract supporting the burning feature.

[tomvanleeuwennl]

@liarco I've made the requested changes

[explorion]

Why cant we use this line directly?

function burn(uint256 tokenId, bool approvalCheck) public { _burn(tokenId, approvalCheck); }

the token burning works properly using this

Let me know if I am missing anything though...

[tomvanleeuwennl]

Yes you can use that burn function. This is something totally different.

This has to do with retrieving the right tokens belonging to the address used as the _owner parameter in the walletofowner function.

When you allow to burn tokens the current walletofowner function acts in the wrong way. Therefor this change is needed to act in the right way.

[liarco]

The implementation seems to be fine now, I tested it with a dedicated project using the "burn" feature and the returned tokens were correct.

@tomvanleeuwennl thank you for your contribution, please let me know if this works for you as well.

[tomvanleeuwennl]

@liarco Can you please explain why still this line is needed:

if (ownership.addr != address(0)) {
latestOwnerAddress = ownership.addr;
}

[liarco]

@tomvanleeuwennl
That's required because of how ERC721A handles token ownerships. Let's say address 0xabc123... mints 3 tokens and 0x123abc... mints 2, the data structure will hold the following values:

token ID	Owner address
1	0xabc123...
2	0x0
3	0x0
4	0x123abc...
5	0x0

And this is how we can save so much gas when minting multiple tokens: we don't update the ownership data for all of the tokens, but just for the first one.
Because of this, once we get to token 1 (ownership.addr != address(0)) we must store that address so we can use it for 2 and 3 too.

P.S. the reason why you didn't need it in your previous implementation was because you were using _ownershipOf(...) which already took care of this internally, but in order to do that it was iterating over the whole collection once per token which could end up doing NUM_TOKENS*NUM_TOKENS iterations in the worst case. By doing this ourself we take it down to NUM_TOKENS at most.

[tomvanleeuwennl]

So, just for my own validation...

This is the start situation:

token ID	Owner address
1	0xabc123...
2	0x0
3	0x0
4	0x0
5	0x0
6	0x0
7	0x123abc...
8	0x0

What happen to tokens 3, 5 and 6 when first token 2 is burned and token 4 is transferred to 0x123aaa?

[liarco]

@tomvanleeuwennl the final situation should be the following:

token ID	Owner address	burned status
1	0xabc123...	true
2	0xabc123...	true
3	0xabc123...	false
4	0x123aaa...	false
5	0xabc123...	false
6	0x0	false
7	0x123abc...	false
8	0x0	false

This behavior is well documented here: https://github.com/chiru-labs/ERC721A/blob/f1f202c782930a74951b52574d53904972303ad1/contracts/ERC721A.sol#L485

[tomvanleeuwennl]

Should be the following then I think. Because token 1 is untouched. But now I understand. Thank you!

token ID	Owner address	burned status
1	0xabc123...	false
2	0xabc123...	true
3	0xabc123...	false
4	0x123aaa...	false
5	0xabc123...	false
6	0x0	false
7	0x123abc...	false
8	0x0	false

[liarco]

@tomvanleeuwennl You are right, sorry. I read first 2 tokens are burned instead of first token 2 is burned 😅