-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
erc721 cde: keep track of the address that burned the asset #323
Conversation
Don't think this is needed for anything. The main thing we need is a stf update that says who burned which asset We could build it as a separate primitive like erc20-deposit but add a special shortcut for the burn address or instead of providing only a prefix for the mint event, you can add up to 2 prefixes: one for the mint and one for the burn (technically on erc721 a mint is just a transfer from the 0 address) |
07bb220
to
52ecf93
Compare
what would be the advantage of that approach? It has the issue that we would end up fetching some data twice, if you have the For now I added an optional prefix. If it's not present then the stf events are not emitted. |
if (isBurn) { | ||
if (cdeDatum.burnScheduledPrefix) { | ||
const scheduledInputData = `${cdeDatum.burnScheduledPrefix}|${ownerRow[0].nft_owner}|${tokenId}`; | ||
const scheduledBlockHeight = isPresync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior for handling presync looks different than what we have in cde-erc20-deposit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, right... It's more consistent to not schedule data during the presync at all in this PR.
This keeps track of the address that burned an erc721 asset by putting those into a different table.
Potentially this could be just a new row in the existing table, but I think this may be clearer? This way this could potentially be optional.
Note that this removes the burned asset from the existing table. This means it won't show anymore when searching for the assets of the 0 address. This may nor may not be desired, but I'm not really sure.
The main motivation for doing this is that, while this can potentially be done by adding erc721 transfer events to the cde and doing the indexing on the stf (which is also what's suggested by the current erc721 docs, but with the generic cde instead), this doesn't work for burns found in the presync stage, since there is no stf running for those.
Questions
What's the appropiate utils function for this? I imagine we may want a
getBurnedNfts(cde, address)
?