Conversation
|
lorisleiva
left a comment
There was a problem hiding this comment.
Hey guys, thank you so much for the heroic amount of work here. It's very hard for me to review everything thoroughly so I thought I'd try a quick "first pass" at the PR but randomly jumping into various source files and giving my first impressions. I apologise in advance if I lacked context within my comments but hopefully you can rectify me pretty quickly if that's the case. There's a mixture of small comments and comments that could lead to significant changes so please let me know if you don't have enough time to dedicate to some of these.
| if (!textEncoder) textEncoder = new TextEncoder(); | ||
| return textEncoder; |
There was a problem hiding this comment.
I think here you want the getUtf8Encoder to support all environments.
| * `pdaValueNode > pdaNode` definitions inside instruction account | ||
| * `defaultValue` nodes. Deduplicates by PDA name. |
There was a problem hiding this comment.
I don't think this is necessary anymore since this PR: #984
There was a problem hiding this comment.
Thanks for flagging this!
Here's my understanding:
The extractPdasVisitor from #984 runs in the nodes-from-anchor pipeline (rootNodeFromAnchor -> defaultVisitor) where it extracts pdaNode definitions from instructions into a program-level pdas: PdaNodes[] and replaces them with pdaLinkNode inline references. That works in the Anchor IDL -> Codama IDL conversion.
dynamic-instructions operates with raw user's Codama IDL json. RootNode is created from createFromJson(json) (in create-program-client.ts), which parses the raw user's Codama IDL JSON.
So the RootNode it receives may still contain inline pdaNode entries inside instruction account defaultValue fields.
We could potentially apply extractPdasVisitor early in the createProgramClient flow so that the IDL is always normalized before processing it (user's raw IDL JSON -> createFromJson(json) -> getRoot() -> extractPdasVisitor).
But this would:
- Add a
nodes-from-anchordependency todynamic-instructions. - Change the shape of the user's provided
RootNodewhich might not be transparent.
Does this make sense, or were you thinking of a different approach? Please correct me if i'm missing something.
| ): TClient { | ||
| const json = typeof idl === 'string' ? idl : JSON.stringify(idl); | ||
| const codama = createFromJson(json); | ||
| let root = codama.getRoot(); |
There was a problem hiding this comment.
Why not just get the root after the if-statement?
|
|
||
| const pdaNodes = collectPdaNodes(root); | ||
|
|
||
| const pdas = |
There was a problem hiding this comment.
Since this is technically returning a dynamic client with instructions and PDAs, I wonder if we shouldn't call that package dynamic-client instead?
Perhaps we should also extract the code for instructions and PDAs inside dynamic-instructions and dynamic-pdas package such that dynamic-client becomes a thin wrapper that puts everything together.
If we do all that, we give room for future improvements such as adding client.accounts.* to the dynamic client.
| export async function createAccountMeta( | ||
| root: RootNode, | ||
| ixNode: InstructionNode, | ||
| argumentsInput: ArgumentsInput = {}, | ||
| accountsInput: AccountsInput = {}, | ||
| signers: EitherSigners = [], | ||
| resolversInput: ResolversInput = {}, | ||
| ): Promise<AccountMeta[]> { |
There was a problem hiding this comment.
I wonder if functions like these wouldn't be better suited as Visitors since they require an instruction and a root which you can get with a NodePath<InstructionNode>.
You can achieve that be recording a NodeStack in your visitors and passing them down to sub-visitors if needed so you keep the full history of visits. You can then even use a LinkableDictionary to jump all over the place and push/pop these paths in the stack.
It looks something like this:
const linkables = new LinkableDictionary();
const stack = new NodeStack();
const visitor = pipe(
baseVisitor,
v => recordNodeStackVisitor(v, stack),
v => recordLinkablesOnFirstVisitVisitor(v, linkables),
);Then you can pass the stack or linkables variable to any sub-visitors that need them. Make sure you also use recordNodeStackVisitor on these sub-visitors so they continue to update the stack.
If you haven't already I recommend you look into the visitors documentation here.
| * keypair management and transaction building behind the scenes. | ||
| * Use the config parameter to include additional programs. | ||
| */ | ||
| export class SvmTestContext { |
There was a problem hiding this comment.
Just a note that LiteSVM will soon be publishing a new version that's compatible with Kit so perhaps we can use that when it's released? It will avoid having to install web3.js in Codama's dev dependencies.
There was a problem hiding this comment.
Good timing looks like it was released this morning haha:
https://www.npmjs.com/package/litesvm/v/1.0.0
| "@solana/instructions": "^5.3.0", | ||
| "codama": "workspace:*", | ||
| "commander": "^14.0.2", | ||
| "superstruct": "^2.0.2" |
There was a problem hiding this comment.
Could you just tell me what we use superstruct for here?
There was a problem hiding this comment.
Hey,
We validate user input when building an instruction. And superstruct builds validators for arguments (based on NodeType - arrays, numbers, strings, pubkeys, etc.) and for required accounts (Address validation).
| /** | ||
| * Concatenates multiple byte arrays into a single Uint8Array. | ||
| */ | ||
| export function concatBytes(chunks: ReadonlyUint8Array[]): Uint8Array { |
There was a problem hiding this comment.
We already have mergeBytes in @solana/codecs.
| @@ -0,0 +1,34 @@ | |||
| export class DynamicInstructionsError extends Error { | |||
There was a problem hiding this comment.
Maybe something to do as a very last step but these should be extracted as specific CodamaErrors that have a unique code and message.
| * Evaluates a ConditionalValueNode's condition. | ||
| * Returns the matching branch (ifTrue or ifFalse) as an InstructionInputValueNode or undefined if no branch matches. | ||
| */ | ||
| export async function resolveConditionalValueNodeCondition({ |
There was a problem hiding this comment.
Not sure if you guys had a look at the getResolvedInstructionInputsVisitor from visitors-core (sorry if I missed it) but it could help a lot with the resolution ordering.
--------- Co-authored-by: Alex S <alexander.shibaev@hoodies.team> Co-authored-by: Sergo <rogaldh@radsh.red>
415210b to
d938818
Compare
* fix: replace concatBytes with mergeBytes * fix: types
* chore: update litesvm@1.0.0 * chore: fix tests and svm-test-context with solana kit * feat: fix writable program address while programId optional strategy [edge case] * chore: uninstall @solana/web3js and @types/bn.js
Summary
Adds
@codama/dynamic-instructionsruntime instruction builder that createsInstructionfrom Codama IDLs without code generation.Key Features
programClient.methods.transfer({}).accounts({}).instruction().defaultValue(PDAs, program IDs, constants) are resolved automatically.createProgramClient<MyProgramClient>(idl).programClient.pdas.myPda({seeds}).ProgramClientTypeScript types withnpx @codama/dynamic-instructions generate-client-types <idl.json> <output-dir>for a given program.Package Structure
Instruction Building Pipeline
Under the hood it:
Instruction { programAddress, accounts, data }.Tests
Notes: