-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Base is optional #57
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
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.
Ah good catch, thanks for this. Would you be able to add a couple of tests for this change? I'm thinking two createAccountWithSeed
tests, one that uses the optional base account and one that doesn't.
The change seems to introduce a lint issue as well. I'm guessing the linter doesn't like the double ternary generated in the type parameters so I might need to adjust the lint settings.
There is now a test, i don't know codama well but is it possible to add more validation regarding baseAccount and base? |
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.
Thanks I've made a couple of comments. Lmk if you're able to see what's going on with the lint issue but no worries otherwise I'll tackle this when I have a moment.
Regarding more complex account validation, yes you can use things like ConditionalValueNodes
to provide different default values based on the value of some other account or argument. Maybe something to think about for another PR if you'd like?
For I had a look to the lint issue but i can't locate what causes the issue, the renderer seems to generate the right code but somehow a comma is added afterwards This is what the linter wants to see
So i'll let you handle it |
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.
Thanks! The lint issue is weird. Codama properly uses the given Prettier options but — for some reason — commas after type parameters seem to be an artistic liberty.
Instead of entering a formatting rabbit hole, I decided to just run pnpm format:fix
on the JS client after generating clients.
This causes explorer crash and needs to be fixed to allow explorer decoding
https://github.com/anza-xyz/solana-sdk/blob/7d7ff1dcf88849fa1f8f11b915e89a48b75cbba7/system-interface/src/instruction.rs#L120-L122
Strangely it isn't documented properly on all other system instructions but that is the case except forTransferWithSeed
The legacy web3.js instruction https://github.com/solana-foundation/solana-web3.js/blob/99aa3c841895848ea407c85340b036f6aceb877d/src/programs/system.ts#L863-L868
Adds it like this, not sure if codama is capable of automating this or we leave this to the consumer.