-
Notifications
You must be signed in to change notification settings - Fork 7
Pointer-aware metadata sync #234
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
Conversation
4c35b66
to
4e16018
Compare
spl_token_metadata_interface::state::TokenMetadata, std::collections::HashMap, | ||
}; | ||
|
||
pub fn assert_metaplex_fields_synced( |
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.
No change to this function, just relocated as it's shared now
4e16018
to
749bbca
Compare
#[test] | ||
fn test_fail_unwrapped_mint_has_no_metadata() { | ||
let unwrapped_mint = MintBuilder::new() | ||
.token_program(TokenProgram::SplToken2022) | ||
.build(); // No metadata extension | ||
|
||
SyncMetadataBuilder::new() | ||
.unwrapped_mint(unwrapped_mint) | ||
.check(Check::err( | ||
TokenWrapError::UnwrappedMintHasNoMetadata.into(), | ||
)) | ||
.execute(); | ||
} |
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.
Covered in program/tests/test_pointer_sync.rs
now
let pointer = mint_state | ||
.get_extension::<MetadataPointer>() | ||
.map_err(|_| TokenWrapError::MetadataPointerMissing)?; |
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.
Would love some guidance here. For token-2022's, is their metadata legit only if they have a pointer? Aka, should we support the scenario of a token-2022 with metadata extension with no pointer?
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.
That's right, the Token Metadata standard in Token-2022 only recognizes metadata explicitly registered with a MetadataPointer
extension. It's also expected that the metadata adheres to the SPL Token Metadata interface (name
, symbol
, uri
, plus optional additional keys).
However, Token-2022 has no way to enforce this interface adherence on the fields specifically. So, maybe we give it our best effort, and default to blank fields for name
, symbol
, and uri
, while populating all of the additional fields as you do for Metaplex?
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'm not quite sure what you mean that it adheres to the interface, but not the fields specifically. The section with update_fields_if_changed()
will ensure the fields will be in sync so if the source data can successfully deserialize then I think we're good.
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.
Really nice work!! Take or leave my comments about the accounts list. The flow looks okay either way.
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.
Looks great (with Joe's comments)
} | ||
|
||
if metadata_info.owner == &token_2022_id() { | ||
// Scenario 2: points to another token-2022 mint |
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.
Is this actually a possible scenario? I wasn't aware 😅
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.
The metadata pointer can be updated for any pubkey. However, if you find this scenario is not legitimate, I can follow up and remove it. I'm not super familiar how folks are using the pointer outside the self and PDA case. I was surprised to find the third-party program case myself.
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'd prefer to remove it -- if a mint is pointing its metadata at another mint, I would worry that it's trying to spoof that other mint.
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.
Will follow up with a PR!
Follow up from #229 (comment)
SyncMetadataToToken2022
now follows a Token-2022 mint’s MetadataPointer to load metadata from the following sources:In the case of the third-party program, a new test program has been added to exercise that flow.