-
Notifications
You must be signed in to change notification settings - Fork 32
IDL: update to use codama macros #180
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
Conversation
grod220
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant feedback from another PR: solana-program/record#130 (comment). The feedback on build.rs, makefile+ci/cd support, and feature flagging is worth including here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tip, added all of this
interface/src/instruction.rs
Outdated
| Initialize(Authorized, Lockup), | ||
| #[codama(account(name = "stake", writable, docs = "Uninitialized stake account"))] | ||
| #[codama(account(name = "rent_sysvar", docs = "Rent sysvar"))] | ||
| Initialize { arg0: Authorized, arg1: Lockup }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need codama support for the tuple variant as this is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRed upstream at codama-idl/codama-rs#73, and now the tuples are restored in this PR
scripts/generate-clients.mts
Outdated
| }, | ||
| { | ||
| // Use omitted optional account strategy for all instructions. | ||
| // + fix discriminator u8 -> u32. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the story with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to do this if the macro is
#[codama(enum_discriminator(size = number(u32)))]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally fixed at this level in #54
This is still the behavior of things. Despite the macro specifying u32, the IDL ends up with u8 and the clients do as well.
If you remove this tiny fix, all clients will have u8s and U8Decoder etc.
| // Resolve default values. | ||
| if (!accounts.clockSysvar.value) { | ||
| accounts.clockSysvar.value = | ||
| 'SysvarC1ock11111111111111111111111111111111' as Address<'SysvarC1ock11111111111111111111111111111111'>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can specify default values in the macro like:
#[codama(account(
name = "clock_sysvar",
docs = "Clock sysvar",
default_value = "SysvarC1ock11111111111111111111111111111111"
))]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a visitor here instead since Codama doesn't (yet) handle Pubkey types correctly without custom handling. It should create a publicKeyValueNode but doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grod220 I found out that codama does these differently. This works!
default_value = sysvar("clock")
interface/src/instruction.rs
Outdated
| } | ||
|
|
||
| Instruction::new_with_bincode(ID, &StakeInstruction::Withdraw(lamports), account_metas) | ||
| Instruction::new_with_bincode(ID, &StakeInstruction::Withdraw { lamports }, account_metas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we should keep these fns for forward compatibility (for now) and swap out the innards where the account metas / inputs are manually being written---for the new builders. Given these are used in tests (I think) it will also validate the builders work against test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! Will work on this a subsequent PR, if that's okay with you.
| "$schema": "https://codama.gg/schemas/codama.json", | ||
| "idls": [ | ||
| { | ||
| "path": "interface/idl.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separately: what's the deal with program/idl.json? Should that be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, old file. Removed
| pub enum StakeAuthorize { | ||
| Staker, | ||
| Withdrawer, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a codama enum attr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 🙏
ef0e9e8 to
63b546c
Compare
d2ac5e8 to
3faa2c7
Compare
grod220
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, think we are close!
| // Add Epoch type alias | ||
| c.definedTypeNode({ | ||
| name: 'epoch', | ||
| type: c.numberTypeNode('u64'), | ||
| }), | ||
| // Add UnixTimestamp type alias | ||
| c.definedTypeNode({ | ||
| name: 'unixTimestamp', | ||
| type: c.numberTypeNode('i64'), | ||
| }), | ||
| ...node.definedTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any motivation for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clarity of named types (as in the program itself)
| // Unwrap defined type links used only as instruction args, then flatten them | ||
| codama.update(c.unwrapInstructionArgsDefinedTypesVisitor()); | ||
| codama.update(c.flattenInstructionDataArgumentsVisitor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, having a hard time understanding this one. Can you give more context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grod220 When we have an instruction with a tuple argument that is actually a struct type, such as AuthorizeWithSeed(AuthorizeWithSeedArgs), Codama needs to call these unwrap and flatten methods so that defined types used solely as instruction arguments are inlined and flattened.
Otherwise the renderer chokes and produces an arg0 as a nested struct argument - this represents the Rust code, but not in the way we want. We want the members of AuthorizeWithSeedArgs to be flattened into top-level arguments for the instruction, and these codama lines accomplish that.
| arguments: node.arguments.map((arg) => | ||
| arg.name === 'discriminator' | ||
| ? { ...arg, type: c.numberTypeNode('u32') } | ||
| : arg | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this due to the codama dep bump where discriminators are added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the aforementioned u8 -> u32 fix. See #54. Codama doesn't do this for the discriminator otherwise. I don't know if we need to fix codama upstream for this; if so, I'll be able to remove this in a followup.
c6bba92 to
14b86f4
Compare
cf868a2 to
efcf5d5
Compare
grod220
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🎊 Think these efforts not only helped the stake program, but even pushed the codama library forward too.
Left a few remaining comments. In future work, think it would be great for our tests to make use of the generated builders so our test suite validates client code directly.
Most of the diff is generated code (clients and lockfiles) - CI demands it
Problem
Client generation uses a lot of fragile manual scripting.
Solution
Update to using Codama macros, including the new Codama support for doc fields and tuples as instruction arguments.