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-2022: Add Pod-compatible versions of StateWithExtensions #6336

Merged
merged 6 commits into from
Mar 6, 2024

Conversation

joncinque
Copy link
Contributor

Problem

Towards zero-copy deserialization in token-2022 in #6316, we need a Pod-compatible version of StateWithExtensions, where the underlying BaseState implements Pod. That way, we can unpack something in zero-copy.

Solution

Add the new types. I tried to do as much dedupe as possible by introducing unpack_tlv_data, but it's tough to reduce too much more than that.

Also, the unpack functions on the new types are extremely similar to their non-Pod versions, and the only difference is calling pod_from_bytes instead of unpack.

We could do this with a macro, but macros always feel a little more brittle / hacky. I don't feel too strongly though, so I'm happy to do whatever you think is best!

Also, unpack_tlv_data is just an immutable version of the previous unpack_type_and_tlv_data, not sure how to reduce the copy-pasta there.

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Do you mind splitting this one into multiple commits? It looks like some of the changes are just renaming (mut), and it would be easier to ensure nothing's changed in the existing call sites if the addition of unpack_tlv_data() and check_account_type_matches_extension_type() were in their own commits as well.

@joncinque
Copy link
Contributor Author

Certainly, done! Let me know if there's anything else that needs more clarity

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks for the reorg! Much easier to review :)
r+ one teeny nit

@@ -847,6 +834,17 @@ impl<'a, S: BaseState> BaseStateWithExtensionsMut<S> for StateWithExtensionsMut<
}
}

fn unpack_tlv_data<S: BaseState>(rest: &[u8]) -> Result<&[u8], ProgramError> {
if let Some((account_type_index, tlv_start_index)) = type_and_tlv_indices::<S>(rest)? {
let account_type = AccountType::try_from(rest[account_type_index])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you restore the comment here? I do think it was helpful so that we can feel confident indexing into rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, will do!

@joncinque joncinque merged commit eb89745 into solana-labs:master Mar 6, 2024
35 checks passed
@joncinque joncinque deleted the tk22podswe branch March 6, 2024 12:29
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