token 2022: add support for _writing_ repeating fixed-length extensions #5837
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
I just noticed that try_get_total_tlv_len will also break because we dedupe extensions there. To change that, we'll have to go through all of the cases where we call try_calculate_account_len without deduping beforehand.
I'm starting to lean towards not having multiple extensions of one type. We made that assumption pretty strongly , and I worry that we'll break something. What do you think?
| /// Unpack a portion of the TLV data as the base mutable bytes, | ||
| /// for a repeating extension | ||
| fn get_repeating_extension_bytes_mut<V: Extension + Pod>( | ||
| &mut self, | ||
| repetition: usize, | ||
| ) -> Result<&mut [u8], ProgramError> { | ||
| get_repeating_extension_bytes_mut::<S, V>(self.tlv_data, repetition) | ||
| } |
There was a problem hiding this comment.
Is this needed? If not, let's remove it, no need to provide more footguns
| match_critera: impl Fn(&V) -> bool, | ||
| ) -> Result<&mut V, ProgramError> { | ||
| for (index, extension) in self.get_all_extensions::<V>()?.iter().enumerate() { | ||
| let repetition = index + 1; |
There was a problem hiding this comment.
nit: Let's stick with 0-indexing to align with the TLV library
|
|
||
| /// Unpack a portion of the TLV data as the desired type that allows | ||
| /// modifying the type, for a repeating extension | ||
| pub fn get_repeating_extension_mut<V: Extension + Pod>( |
There was a problem hiding this comment.
nit:
| pub fn get_repeating_extension_mut<V: Extension + Pod>( | |
| pub fn get_extension_with_repetition_mut<V: Extension + Pod>( |
| @@ -413,6 +413,37 @@ fn get_extension_bytes_mut<S: BaseState, V: Extension>( | |||
| Ok(&mut tlv_data[value_start..value_end]) | |||
| } | |||
|
|
|||
| fn get_repeating_extension_bytes_mut<S: BaseState, V: Extension>( | |||
There was a problem hiding this comment.
nit for consistency
| fn get_repeating_extension_bytes_mut<S: BaseState, V: Extension>( | |
| fn get_extension_bytes_with_repetition_mut<S: BaseState, V: Extension>( |
| let mut value_start = 0; | ||
| let mut value_end = 0; | ||
|
|
||
| for _ in 0..repetition { |
There was a problem hiding this comment.
nit: let's stick with 0-indexing (or use 0..=repetition if you prefer)
| for _ in 0..repetition { | |
| for _ in 0..repetition+1 { |
| @@ -752,6 +816,18 @@ impl<'data, S: BaseState> StateWithExtensionsMut<'data, S> { | |||
| Ok(extension_ref) | |||
| } | |||
|
|
|||
| /// Packs the default extension data into an open slot, disregarding if | |||
| /// the extension has already been found in the data buffer. | |||
| pub fn init_extension_allow_repeating<V: Extension + Pod + Default>( | |||
There was a problem hiding this comment.
nit:
| pub fn init_extension_allow_repeating<V: Extension + Pod + Default>( | |
| pub fn init_extension_with_repetition<V: Extension + Pod + Default>( |
|
|
||
| /// Returns an unpacked portion of TLV data that allows modifying the type, | ||
| /// based on the specified match criteria | ||
| pub fn get_first_matched_repeating_extension_mut<V: Extension + Pod>( |
There was a problem hiding this comment.
nit: how about just find_extension_mut?
| /// based on the specified match criteria | ||
| pub fn get_first_matched_repeating_extension_mut<V: Extension + Pod>( | ||
| &mut self, | ||
| match_critera: impl Fn(&V) -> bool, |
There was a problem hiding this comment.
nit-up-to-you: Iterator calls this predicate https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.find
| fn alloc_allow_repeating<V: Extension>( | ||
| &mut self, | ||
| length: usize, | ||
| ) -> Result<&mut [u8], ProgramError> { | ||
| if V::TYPE.get_account_type() != S::ACCOUNT_TYPE { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| } | ||
|
|
||
| let mut start_index = 0; | ||
| let mut type_start = 0; | ||
| let mut length_start = 0; | ||
| let mut value_start = 0; | ||
| let mut extension_type = V::TYPE; | ||
| let required_len = add_type_and_length_to_len(length); | ||
|
|
||
| while extension_type != ExtensionType::Uninitialized { | ||
| let indices = get_extension_indices::<V>(&self.tlv_data[start_index..], true)?; | ||
| (type_start, length_start, value_start) = ( | ||
| indices.type_start.saturating_add(start_index), | ||
| indices.length_start.saturating_add(start_index), | ||
| indices.value_start.saturating_add(start_index), | ||
| ); | ||
|
|
||
| if self.tlv_data[type_start..].len() < required_len { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| } | ||
|
|
||
| extension_type = ExtensionType::try_from(&self.tlv_data[type_start..length_start])?; | ||
| start_index = value_start.saturating_add(usize::from(*pod_from_bytes::<Length>( | ||
| &self.tlv_data[length_start..value_start], | ||
| )?)); | ||
| } | ||
|
|
||
| // write extension type | ||
| let extension_type_array: [u8; 2] = V::TYPE.into(); | ||
| let extension_type_ref = &mut self.tlv_data[type_start..length_start]; | ||
| extension_type_ref.copy_from_slice(&extension_type_array); | ||
|
|
||
| // write length | ||
| let length_ref = | ||
| pod_from_bytes_mut::<Length>(&mut self.tlv_data[length_start..value_start])?; | ||
| *length_ref = Length::try_from(length)?; | ||
| let value_end = value_start.saturating_add(length); | ||
|
|
||
| Ok(&mut self.tlv_data[value_start..value_end]) | ||
| } |
There was a problem hiding this comment.
It seems like it would avoid a lot of copy-pasta to change the flag in get_extension_indices to be an enum with three variants: Get, Init, InitWithRepetition, and GetWithRepetition(usize). Do you see what I mean?
Then you can get rid of get_repeating_extension_bytes_mut and have get_repeating_extension avoid fetching all of the repetitions just to extract one.
Orrrrr, we decide to roll members without repetition to begin with 👀
Yeah, I thought about this quite a bit, and more since we spoke offline. I think we should scrap repeating extensions. I haven't merged any code supporting multiple extensions. I say we introduce group members as only being able to be members of one group, and if the need arises in the future for multiple, we can take a closer look at this. But for now, I'm in agreement and much more comfortable with keeping the TLV entries as-is. |
|
Ok cool, glad to hear that we're on the same page! You did some great work, and I did want to integrate it. I think we're just too far along to make big changes to the serialization. Tag me for a review on the other PRs when they're ready |
buffalojoec
deleted the
11-13-token_2022_add_support_for__writing__repeating_fixed-length_extensions
branch
Similar to the PR before this one, we need to expand the API for Token2022's
extensions to support repeating extension entries.
Previously we addresses reading these extensions, now we shall address
writing.
This PR introduces an adjacent API for writing repeatable fixed-length
extensions to Token2022 mints.