Skip to content

Conversation

@steveluscher
Copy link
Contributor

@steveluscher steveluscher commented Dec 3, 2024

Warning

I'm pretty sure that this is not landable, because if the legacy built-in and this program have different discriminator sizes, then the actual instruction data will not line up when we switch from one to the other. I couldn't find a way to configure Shank macros to produce 4-byte discriminants.

Fixes: #2.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paging @lorisleiva, Shank wizard. Is there a way to convince Shank to produce 4-byte instruction discriminators other than to hot-patch the idl?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can maybe update the comment here to say "this program" instead of "the legacy native program", since they are both the same, but idc.

Copy link
Member

Choose a reason for hiding this comment

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

Sadly no, Shank doesn't support custom enum discriminants. You could use a Codama visitor to update this but since we're currently saving the Anchor IDL and not the Codama one, your solution is currently the best thing to do.

@buffalojoec
Copy link
Contributor

buffalojoec commented Dec 4, 2024

This can land. The program's instructions aren't serialized or deserialized using Shank, but rather bincode.

Instruction::new_with_bincode(
crate::id(),
&LoaderV3Instruction::InitializeBuffer,
accounts,
)

This patch would just be for clients, and until Shank supports u32 (or we have Codama macros), this is fine IMO.

@steveluscher
Copy link
Contributor Author

But hang on, if I call, say DeployWithMaxDataLen which actually has instruction data, and the old program is like:

2 0 0 0 1 1 1 1 1 1 1 1
^^^^^^^ ^^^^^^^^^^^^^^^
disc    max_data_len

But the new program is like:

2 1 1 1 1 1 1 1 1
^ ^^^^^^^^^^^^^^^
d max_data_len

And then we have clients sending the old data to the new program, wouldn't it get interpreted like this?

2 0 0 0 1 1 1 1
^ ^^^^^^^^^^^^^
d max_data_len

@buffalojoec
Copy link
Contributor

buffalojoec commented Dec 4, 2024

But the new program is like:

This is what I'm saying, the new program is not using u8 discriminators. It's using u32, since I've derived serde's Serialize and Deserialize on them and I use bincode to pack/unpack within the helpers and processor.

fn limited_deserialize<T>(input: &[u8]) -> Result<T, ProgramError>

In other words, it's exactly the same instruction implementation as the builtin.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can maybe update the comment here to say "this program" instead of "the legacy native program", since they are both the same, but idc.

...instruction,
discriminant: {
...instruction.discriminant,
type: "u32", // The legacy native program only accepts 4-byte instruction discriminants.
Copy link
Member

Choose a reason for hiding this comment

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

Sadly no, Shank doesn't support custom enum discriminants. You could use a Codama visitor to update this but since we're currently saving the Anchor IDL and not the Codama one, your solution is currently the best thing to do.

@buffalojoec buffalojoec merged commit a8d0bc0 into main Dec 4, 2024
9 checks passed
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.

Discriminator should be u32

4 participants