Skip to content

IDL: Update discriminants to u32 instead of u8#54

Merged
joncinque merged 7 commits into
solana-program:mainfrom
joncinque:u32discriminant
Mar 3, 2025
Merged

IDL: Update discriminants to u32 instead of u8#54
joncinque merged 7 commits into
solana-program:mainfrom
joncinque:u32discriminant

Conversation

@joncinque
Copy link
Copy Markdown
Contributor

Problem

The IDL used to generate the stake program client declares the instruction discriminant as a u8, when the program actually uses u32s for the discriminant.

Summary of changes

Update the IDL, update the generate script, then regenerate all of the clients.

NOTE: This doesn't cover the StakeState and StakeStateV2 enums, which both use a u32 discriminant too. I wasn't sure how to do that in the IDL, so I omitted it.

#### Problem

The IDL used to generate the stake program client declares the
instruction discriminant as a `u8`, when the program actually uses
`u32`s for the discriminant.

#### Summary of changes

Update the IDL, update the generate script, then regenerate all of the
clients.

NOTE: This doesn't cover the `StakeState` and `StakeStateV2` enums,
which both use a `u32` discriminant too. I wasn't sure how to do that in
the IDL, so omitted it.
@joncinque joncinque requested a review from lorisleiva February 27, 2025 21:11
@joncinque
Copy link
Copy Markdown
Contributor Author

Looks like there some codegen messing up for Rust -- do I need to update something? I tried updating all of the codama deps, but it didn't resolve the issue.

I then removed the trait overrides from the generate script, but that caused other issues because of f64s in the structs, but I'm not sure how to get around that. After that, I just need to update the hooked file. So once the f64 thing is resolved, this should be ok

@lorisleiva
Copy link
Copy Markdown
Member

It looks like the new default traits for Rust items are causing an issue in this upgrade. You could use the following base traits to avoid using Eq on all items.

const traitOptions = {
    baseDefaults: [
        'borsh::BorshSerialize',
        'borsh::BorshDeserialize',
        'serde::Serialize',
        'serde::Deserialize',
        'Clone',
        'Debug',
        // 'Eq', <- Remove 'Eq' from the default traits.
        'PartialEq',
    ],
};

You can also be more granular with this if you wanted to (see option docs).

Regarding updating the StakeState and StakeStateV2 enums, I don't believe you can do that on the Anchor IDL (which is what we currently have for that program) but you can use a transformer visitor on the generate-clients script to adjust the tree before rendering.

[2min later]

Looking at the current script though it looks like something like this already exists. Is is not materialising as a u32 in the generated code?

codama.update(
  c.bottomUpTransformerVisitor([
    {
      // enum discriminator -> u32
      select: '[definedTypeNode]stakeStateV2.[enumTypeNode]',
      transform: (node) => {
        c.assertIsNode(node, 'enumTypeNode');
        return {
          ...node,
          size: c.numberTypeNode('u32'),
        };
      },
    },
  ])
);

@joncinque
Copy link
Copy Markdown
Contributor Author

It looks like the new default traits for Rust items are causing an issue in this upgrade. You could use the following base traits to avoid using Eq on all items.

Ah ok great, done!

Looking at the current script though it looks like something like this already exists. Is is not materialising as a u32 in the generated code?

You're right, it's correct for StakeStateV2, but not StakeState, so I added it for StakeState as well

@joncinque
Copy link
Copy Markdown
Contributor Author

Oh, but note that the u32 enum discriminant on structs isn't possible with Rust, since it's just using the normal derive macro for borsh

@lorisleiva
Copy link
Copy Markdown
Member

Oh yeah. We really need Rust codecs so anything doable in JS can be done in Rust. Aside from that is this okay to merge? I'm gonna rebase my @solana/kit upgrade on top of that afterwards.

@joncinque
Copy link
Copy Markdown
Contributor Author

Yeah it's ready from my side! So if you approve, I'll merge

Copy link
Copy Markdown
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Thanks!

@joncinque joncinque merged commit f22817c into solana-program:main Mar 3, 2025
@joncinque joncinque deleted the u32discriminant branch March 3, 2025 17:55
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