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

[token] Add is_deleted to token datas (v2 nfts only) #359

Merged
merged 6 commits into from
May 9, 2024

Conversation

bowenyang007
Copy link
Collaborator

@bowenyang007 bowenyang007 commented Apr 30, 2024

Summary

For context we want to be able to know if a token was deleted from the current_token_datas_v2 table. This only works for v2 tokens because v1 tokens can be fungible and non fungible interchangeably so it's impossible to get this field accurately.

Backfill

Testnet: 553151941
Mainnet: 199271122

Testing

is_deleted_v2 in!
image

@bowenyang007 bowenyang007 requested a review from rtso April 30, 2024 23:13
@rtso
Copy link
Collaborator

rtso commented Apr 30, 2024

How big of an effort is it to index is_deleted for v1 tokens?

@bowenyang007
Copy link
Collaborator Author

How big of an effort is it to index is_deleted for v1 tokens?

If we can disregard fungible v1 tokens, then we can do it. Actually it's a good point they might want for v1 as well.

@rtso
Copy link
Collaborator

rtso commented May 2, 2024

@bowenyang007 yeah just disregard fungible tokens because the logic will be separated after my PR anyway

@bowenyang007
Copy link
Collaborator Author

@rtso not v1 fungible tokens lol. v1 fungible tokens look the same as v1 nfts.

@bowenyang007
Copy link
Collaborator Author

Ok confirmed with stakeholders that we only need v2 for now

@bowenyang007 bowenyang007 changed the title [token] Add is_deleted to token datas [token] Add is_deleted to token datas (v2 nfts only) May 3, 2024
@bowenyang007 bowenyang007 force-pushed the token_data_improve branch from b45c40c to 31e462a Compare May 6, 2024 20:15
},
)))
} else {
Ok(None)
}
}

/// This handles the case where token is burned but objectCore is still there
pub async fn get_burned_nft_v2_from_write_resource(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this case already covered by get_burned_nft_v2_from_delete_resource below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. this is write resource.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a token gets burnt won't it always have a delete resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's a partial burn and there are resources left?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't the token resource get deleted so there will always be a delete resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no that's not how it works. unless the resource group is entirely destroyed it'll emit a write resource event :(

@@ -893,6 +945,22 @@ async fn parse_v2_token(
);
}

// Add burned NFT handling for token datas (can probably be merged with below)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah can we merge this with below since it's doing the same thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue is that the code below is more complex than needed here b/c it needs previous owner as well. I'm hoping that as you guys look at sdk you can also rewrite this whole logic. The logic has kind of ballooned out of control a bit. For now I don't see a clean way to merge the paths so just leaving a comment for future.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i'll take a stab at refactoring this for sdk

@bowenyang007 bowenyang007 force-pushed the token_data_improve branch from 60e4f28 to 3138773 Compare May 8, 2024 06:32
@bowenyang007 bowenyang007 merged commit 2260208 into main May 9, 2024
8 checks passed
@bowenyang007 bowenyang007 deleted the token_data_improve branch May 9, 2024 19:59
yuunlimm pushed a commit that referenced this pull request May 9, 2024
* add is_deleted to token datas

* lint

* comment to avoid confusion

* delete works for historical events too

* rebase

* lint
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