-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 2022: add support for _reading_ repeating fixed-length extensions #5836
token 2022: add support for _reading_ repeating fixed-length extensions #5836
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
3cf8ea8
to
9e3250a
Compare
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 good overall! Just some little comments that I hope make it better
/// Unpack a portion of the TLV data as the desired type | ||
fn get_extension<V: Extension + Pod>(&self) -> Result<&V, ProgramError> { | ||
pod_from_bytes::<V>(self.get_extension_bytes::<V>()?) | ||
} | ||
|
||
/// Unpack a portion of the TLV data as the desired type, where repetitions | ||
/// are allowed | ||
fn get_repeating_extension<V: Extension + Pod>( |
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.
nit: get_extension_with_repetition
/// Fetch the bytes for a TLV entry, where repetitions are allowed | ||
fn get_repeating_extension_bytes<V: Extension>( | ||
&self, | ||
repetition: usize, | ||
) -> Result<&[u8], ProgramError> { | ||
get_all_extension_bytes::<S, V>(self.get_tlv_data()).map(|x| { | ||
*x.get(repetition.saturating_sub(1)) | ||
.ok_or::<ProgramError>(ProgramError::InvalidAccountData) | ||
.unwrap() | ||
}) | ||
} | ||
|
||
/// Fetch all bytes for each entry of a particular TLV entry, where | ||
/// repetitions are allowed | ||
fn get_all_extension_bytes<V: Extension>(&self) -> Result<Vec<&[u8]>, ProgramError> { | ||
get_all_extension_bytes::<S, V>(self.get_tlv_data()) | ||
} |
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.
Are these needed? get_extension_bytes
is only used for emitting a slice of bytes for token metadata, and we won't need to do that with the group interface, so I'd prefer to remove these functions if they're not needed
repetition: usize, | ||
) -> Result<&[u8], ProgramError> { | ||
get_all_extension_bytes::<S, V>(self.get_tlv_data()).map(|x| { | ||
*x.get(repetition.saturating_sub(1)) |
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 TLV library is 0-indexed, so let's keep that consistent here
let state = | ||
StateWithExtensions::<Mint>::unpack(MINT_WITH_DUPLICATED_EXTENSION_AND_ONE_EXTRA) | ||
.unwrap(); | ||
assert_eq!(state.base, TEST_MINT); | ||
let all_extensions = state.get_all_extensions::<MintCloseAuthority>().unwrap(); | ||
assert_eq!(all_extensions.len(), 3); | ||
assert_eq!( | ||
state.get_extension::<TransferFeeConfig>(), | ||
Err(ProgramError::InvalidAccountData) | ||
); | ||
assert_eq!( | ||
StateWithExtensions::<Account>::unpack(MINT_WITH_DUPLICATED_EXTENSION_AND_ONE_EXTRA), | ||
Err(ProgramError::InvalidAccountData) | ||
); |
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.
Can you also check that you can fetch the extra extension, just to be safe?
In order to support Token2022 extensions that can repeat, we need to add support
to read such extensions!
Almost every read function for Token2022 extensions is expecting one single
entry, so it will return the first encountered.
Here. we're introducing an adjacent API for reading repeating extensions,
specifically fixed-length extensions.