runtime: fix owner update for burned NFT instances #589
Conversation
pro-wh
added
bug
| @@ -121,7 +121,11 @@ type NFTKey struct { | |||
| } | |||
|
|
|||
| type PossibleNFT struct { | |||
| NewOwner *apiTypes.Address | |||
| // NewOwner has the latest owner if .NumTransfers is more than zero. If | |||
| // the last transfer burned this NFT instance, then it's the empty string. | |||
There was a problem hiding this comment.
Nice, thank you for the docstrings! Can we keep it as a pointer though? Right now, to realize that the NewOwner value should be ignored, one needs to check NumTransfers == 0, which is far from obvious.
Optional: I also suggest the special value be something other than the empty string, e.g. "__BURN__" or "0". It's currently set very implicitly, via uninitialized values, and it's easy to miss it when reading the code. Also when checking the variable value in an if, "__BURN__" is a lot more informative than "".
There was a problem hiding this comment.
I'm not feeling that special string idea. I'll add a Burned bool field.
| RuntimeEVMNFTUpsert = ` | ||
| INSERT INTO chain.evm_nfts AS old | ||
| (runtime, token_address, nft_id, owner, num_transfers, last_want_download_round) | ||
| (runtime, token_address, nft_id, num_transfers, last_want_download_round) | ||
| VALUES | ||
| ($1, $2, $3, $4, $5, $6) | ||
| ON CONFLICT (runtime, token_address, nft_id) DO UPDATE | ||
| SET | ||
| owner = COALESCE(excluded.owner, old.owner), | ||
| num_transfers = old.num_transfers + excluded.num_transfers` | ||
| ($1, $2, $3, 0, $4) | ||
| ON CONFLICT (runtime, token_address, nft_id) DO NOTHING` | ||
|
|
||
| RuntimeEVMNFTUpdateTransfer = ` | ||
| UPDATE chain.evm_nfts SET | ||
| owner = $4, | ||
| num_transfers = num_transfers + $5 | ||
| WHERE | ||
| runtime = $1 AND | ||
| token_address = $2 AND | ||
| nft_id = $3` |
There was a problem hiding this comment.
I would prefer to keep the single-statement setup here. It will keep the upsert a lot more similar to other upserts, both on the Go and SQL side, and thus easier to read and reason about.
We'll just need a special value for owner (or rather $4) that SQL can recognize. So in the old query, only a single line would need to change, something like
ON CONFLICT (runtime, token_address, nft_id) DO UPDATE
SET
owner = CASE WHEN excluded.owner = '__BURN__' THEN NULL ELSE COALESCE(excluded.owner, old.owner) END,There was a problem hiding this comment.
"please do this completely differently. approved"
There was a problem hiding this comment.
Sorry if it looked weird. As explained on slack, approved to unblock you after I'm gone in my timezone.
Also, "completely differently" is ... one possible interpretation in that the whole PR is not very large, so the percentage of affected lines is large. But I agree with all the debugging work that led to the small PR, and with the semantics of the fix, which is to sometimes force the NULL into the DB. Those are the more important parts IMO, and my comment is about a technicality and code readability.
Really signing off now.
|
nah closing this was too rash. I'm gonna merge this as-is and do a different version tomorrow. which is rasher |
previously I tried using COALESCE(new owner, old owner) so that we can pass nil to leave the owner as it is when we don't see any transfers, but that didn't leave a way to NULL out the owner when an NFT instance gets burned. the old code turned out to try to set a non-NULL empty string owner, which the Oasis address type doesn't allow.
this PR splits up the runtime analyzer's upsert into an upsert for any NFT detected, plus a new update for when we see transfers, which sets a new owner that may be NULL and bumps the num_transfers.