Skip to content
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

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

ecioppettini
Copy link
Contributor

@ecioppettini ecioppettini commented Mar 15, 2024

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)?

@ecioppettini ecioppettini self-assigned this Mar 15, 2024
@ecioppettini ecioppettini marked this pull request as ready for review March 19, 2024 21:40
@SebastienGllmt
Copy link
Contributor

SebastienGllmt commented Mar 29, 2024

What's the appropiate utils function for this? I imagine we may want a getBurnedNfts(cde, address)?

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)

@ecioppettini ecioppettini force-pushed the enzo/erc721-keep-track-of-burnt-assets branch from 07bb220 to 52ecf93 Compare April 5, 2024 00:45
@ecioppettini
Copy link
Contributor Author

We could build it as a separate primitive like erc20-deposit but add a special shortcut for the burn address

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 erc721 and this new primitive enabled at the same time. But I guess that would be useful if someone only wanted the burn events, is that a potential use-case?

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
Copy link
Contributor

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

Copy link
Contributor Author

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.

@SebastienGllmt SebastienGllmt merged commit 6c0af96 into master Apr 5, 2024
@SebastienGllmt SebastienGllmt deleted the enzo/erc721-keep-track-of-burnt-assets branch April 5, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants