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 alloc_and_serialize for fixed-len extensions #5672

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Oct 26, 2023

This PR adds the alloc_and_serialize function for fixed-length extensions to Token2022.

Note: I decided to follow already pre-established patterns and rename the existing alloc_and_serialize function to alloc_and_serialize_variable_len_extension. The existing pattern has been to add some form of *variable_len* suffix on functions that deal with variable-length methods, and give the more concisely named functions to the fixed-length extensions.

Regardless, this function allows reallocation of an account only if the extension does not exist yet and requires more uninitialized bytes to write it in.

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This is looking very close! Just a few nits, but the core is good. Could you add a test for the new function?

token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

Could you add a test for the new function?

Yep, I was mostly waiting to see if you were okay with changing this many bits of the TLV/extension module. Didn't want to make too many changes.

Will definitely add some tests!

@buffalojoec buffalojoec force-pushed the 10-26-token_2022_add_alloc_and_serialize_for_fixed-len_extensions branch from 7b557fe to 318e54b Compare October 29, 2023 13:18
@buffalojoec buffalojoec requested a review from joncinque October 29, 2023 13:23
@buffalojoec buffalojoec force-pushed the 10-26-token_2022_add_alloc_and_serialize_for_fixed-len_extensions branch from 318e54b to 0509e06 Compare November 6, 2023 20:57
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just a few last little things, then this is good to go! The tests look great

token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
token/program-2022/src/extension/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@joncinque joncinque 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 integrating those changes!
fixed

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