-
Notifications
You must be signed in to change notification settings - Fork 43
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
Upstream Token Extensions From Helius #179
Upstream Token Extensions From Helius #179
Conversation
042bb0d
to
16e6f0c
Compare
16e6f0c
to
df72080
Compare
db: &'b DatabaseConnection, | ||
download_metadata_notifier: &DownloadMetadataNotifier, | ||
) -> ProgramTransformerResult<()> { | ||
match &parsing_result { |
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.
Does there need to be handling for burned tokens?
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.
There should but doesn't look to be handled. Getting an integration test in to confirm. This is a issue with token-metadata as well.
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.
Token Metadata has this handling if the metadata account is empty: https://github.com/metaplex-foundation/digital-asset-rpc-infrastructure/blob/main/program_transformers/src/token_metadata/mod.rs#L24-L27
But then I think how DAS effectively leaves out burned accounts is because the supply is zero, like here: https://github.com/metaplex-foundation/digital-asset-rpc-infrastructure/blob/main/digital_asset_types/src/dao/scopes/asset.rs#L67
So then supply is probably being set by handle_token2022_mint_account
so maybe it is already not going to return them, but they would never be marked as burnt?
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.
Token Metadata has this handling if the metadata account is empty: https://github.com/metaplex-foundation/digital-asset-rpc-infrastructure/blob/main/program_transformers/src/token_metadata/mod.rs#L24-L27
Is the account owner not set to the system program so would this account update ever come through? Looking through this EmptyAccount creation though.
But then I think how DAS effectively leaves out burned accounts is because the supply is zero, like here: https://github.com/metaplex-foundation/digital-asset-rpc-infrastructure/blob/main/digital_asset_types/src/dao/scopes/asset.rs#L67
This is based on token_account same issue occurs if token account is closed and ownership transferred system program.
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.
I've looked into EmptyAccount which is what triggers flagging an asset as burned.
It is set within the handle_account of blockbuster when the data slice for the account is empty.
This occurs in burn_nonfungible within the token-metadata program.
The token metadata account is re-assigned to the system program so does geyser send a change event under the owner of token-metadata or system program? If its the later DAS will never register the change since its account selector does not include system program.
The token extensions blockbuster parser does include the same EmptyAccount but again depends on the ownership assignment.
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.
On a local test validator stack when a traditional NFT is created and then the burn instruction of token-metadata is invoked. The following occurs:
- The mint is left open but supply dropped to zero. This is correctly reflected in DAS.
- The token account is closed and reassigned to the system program but DAS is not notified.
- token metadata account is left open with 1 byte. DAS receives the update event but throws
{"timestamp":"2024-04-09T13:15:37.272578Z","level":"WARN","fields":{"message":"Error parsing account 5Yga8VnpA1GXnYohtH7w3NMKVczA7ZXUUA16T5UJEG16: \"Uninitialized account type\"","log.target":"nft_ingester::metrics","log.module_path":"nft_ingester::metrics","log.file":"nft_ingester/src/metrics.rs","log.line":94},"target":"nft_ingester::metrics"}
The asset is not marked as burnt because it the update event fails to parse. The supply is set to 0 on the asset because of the mint account so this scope does apply to filter out the zombie assets but they should be returned with status of burnt or if filtered out should be on burnt for list fetches unless burnt true a query param on getAsset should always return but correctly marked as burnt and supply 0.
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 is because account closures go through the system program. It's annoying. We'll upstream this fix.
2aaf6e0
to
8595199
Compare
8595199
to
dfe0ced
Compare
…egration test for the token extensions.
} | ||
|
||
const fn is_token_nft(m: &MintAccount) -> bool { | ||
m.account.supply == 1 && m.account.decimals == 0 |
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 check does not trigger when the asset is burnt. The supply on the mint is 0 so it is no longer recognized as a token_nft though it is.
In this state the token supply is set to 0 but the supply on the asset is still 1.
see #214 |
Changes