Conversation
🦋 Changeset detectedLatest commit: a4ce63c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
| * `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.
packages/dynamic-instructions/src/program-client/create-program-client.ts
Outdated
Show resolved
Hide resolved
|
|
||
| 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.
There was a problem hiding this comment.
@mikhd Before I dig too much into the other parts of this review, I'd love your opinion on that one. I think splitting this package into 3 might make more sense and be consistent with the way we currently do things with dynamic-codecs and dynamic-parsers. Wdyt?
There was a problem hiding this comment.
Hey,
Reviewed this with guys, at the moment, it seem that the current package contains single united feature.
The question is would dynamic-instructions and dynamic-pdas be used as standalone packages or are they for internal-only usage?
It seems that renaming to dynamic-client makes sense but splitting into three packages would turn internal coupling into cross-package dependencies - more maintenance overhead without real decoupling.
| 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.
There was a problem hiding this comment.
Thank you!
We checked NodeStack + LinkableDictionary adoption for the resolution pipeline.
Key constraint: all visitor infrastructure is synchronous (recordNodeStackVisitor does sync push/pop), while our resolution pipeline is async due to getProgramDerivedAddress and async custom resolver functions.
But here is a minimal example of what can be implemented (as I understood it). I guess, it doesn't fully align to your suggestion, but:
- The NodeStack holds the [Root, Program, Instruction] path. It is created once before the sequential resolution loop and never modified during async operations. NodeStack path replaces root/ixNode parameters with getting them from Stack.
- LinkableDictionary is created and passed as args for PDA and linked accounts lookup.
The main issue for full visitor conversion and push/pop NodeStack is that the current runtime resolution is not a normal tree transform - it is async. It also stateful and depends on runtime inputs: accountsInput, argumentsInput, custom resolvers.
What was your thoughts?
| "@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).
| * 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.
There was a problem hiding this comment.
Thank you! Can you please explain what was your thougs about its usage?
Not sure I understood it correctly, but we created a minimal example of getResolvedInstructionInputsVisitor integration in a separate PR hoodieshq#14.
Here promise.all accounts were replaced by sequential resolution from getResolvedInstructionInputsVisitor.
It can help with catching circular dependencies early, but other that that we still need visitors and resolvers to evaluate conditionals, derive pdas, encode seeds based on user inputs at runtime. Maybe we are addressing different things?
Also one real-world limitation we hit was that the getResolvedInstructionInputsVisitor visitor static analysis cant resolve nested argument dependencies in resolverValueNode with dependsOn.
For example, create instruction from Metaplex mpl-token-metadata has resolverValueNode({ dependsOn: [argumentValueNode('tokenStandard')] }), where tokenStandard is a key of a struct createArgs {}.
getResolvedInstructionInputsVisitor uses getAllInstructionArguments which only returns top-level args discriminator, createArgs, doesn't find "tokenStandard" and throws CODAMA_ERROR__VISITORS__INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCY.
CodamaError: Dependency [tokenStandard] of kind [argumentValueNode] is not a valid dependency of [splTokenProgram] of kind [instructionAccountNode] in the [create] instruction
Our solution was to use fallback strategy: when getResolvedInstructionInputsVisitor throws, we fallback to a custom iterative loop.
415210b to
d938818
Compare
--------- Co-authored-by: Alex S <alexander.shibaev@hoodies.team> Co-authored-by: Sergo <rogaldh@radsh.red>
* 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
* fix: move errors to @codama/errors and adjust * feat: replacing dynamic-instruction errors to codama/errors [WIP] * feat: fix errors * fix: errors * fix: tests * chore: reorder error codes * fix: cleanup * fix: cleanup * fix: availableIxs * chore: implement getMaybeNodeKind helper
6549faa to
64dc6cc
Compare
|
@trevor-cortex Please provide a thorough review of this PR. Take all the time you need as this is a big PR and we wanna make sure we're not missing anything important before merging. Double check your findings before adding comments to your review to minimise noise. God speed. |
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
This is a substantial and well-structured PR that adds @codama/dynamic-instructions — a runtime instruction builder that creates Instruction objects from Codama IDLs without code generation. The package provides an Anchor-like builder API with auto-resolution of PDA accounts, program IDs, and conditional defaults.
The architecture is solid: clean separation between the public API (program-client/), instruction encoding pipeline (instruction-encoding/), and CLI. The visitor pattern for node resolution is well-applied. The test suite is impressively thorough — integration tests against real Solana programs (System, Token, ATA, MPL Token Metadata, PMP) via LiteSVM, plus edge cases for circular dependencies, custom resolvers, and collection types.
Key Issues
Must fix
- Wrong package name in generated types header —
@hoodieshq/dynamic-instructionsshould be@codama/dynamic-instructions - CI path mismatch —
setup-anchor/action.ymldefaults referencetest/anchor/but the actual path istest/programs/anchor/ - Unused
Codecvalue import — should beimport type { Codec }(tree-shaking concern)
Worth reviewing
- Parallel account resolution with
Promise.all— resolving accounts in parallel may cause issues with dependent accounts that have shared mutable state (theresolutionPathcircular dependency detection works correctly since each path is forked, but this is worth noting) createAccountMetarole downgrade — all accounts matchingprogramAddressget downgraded to readonly, but this only matters for optional accounts resolved to programId strategy. The check is broader than necessary.
Areas for subsequent reviewers
- The
validators.tsfile (514 lines) is large — consider splitting into per-type-node validators in a future refactor - The
generate-client-types.tsstring-concatenation approach works but could benefit from a proper AST builder if the generated output grows - The
mapTypeNodeincodamaTypeToTSalways emitsRecord<string, V>regardless of the actual key type — this is a known simplification but worth documenting - Verify the Solana version bump from v2 to v3 doesn't affect other packages in the monorepo
- The
.sodumps intest/programs/dumps/are binary files committed to git — consider documenting how to regenerate them
packages/dynamic-client/src/cli/commands/generate-client-types/generate-client-types.ts
Show resolved
Hide resolved
packages/dynamic-client/src/instruction-encoding/arguments/encode-instruction-arguments.ts
Show resolved
Hide resolved
packages/dynamic-client/src/instruction-encoding/accounts/create-account-meta.ts
Outdated
Show resolved
Hide resolved
| } | ||
| case 'mapTypeNode': { | ||
| const v = codamaTypeToTS(type.value, definedTypes); | ||
| return `Record<string, ${v}>`; |
There was a problem hiding this comment.
Map keys are always typed as string regardless of the actual node.key type. This works for most IDLs (where map keys are strings), but if a program uses publicKeyTypeNode or numberTypeNode keys, the generated type won't reflect that. Worth a comment noting this is a known simplification.
| if (!Number.isInteger(node.number) || node.number < 0 || node.number > 0xff) { | ||
| throw new CodamaError(CODAMA_ERROR__DYNAMIC_INSTRUCTIONS__INVARIANT_VIOLATION, { | ||
| message: `NumberValueNode PDA seed is out of range: must be a valid u8 (0–255), got ${node.number}`, | ||
| }); |
There was a problem hiding this comment.
This assumes NumberValueNode in PDA seeds is always a single u8 byte. That's currently true for all known IDLs (PDA seeds are raw bytes), but the assertion message says "must be a valid u8" without citing why. Consider adding a brief comment explaining that PDA constant seeds using numberValueNode are always single-byte values per the Codama spec, so future readers understand this isn't an arbitrary limitation.
* fix: anchor paths in setup-anchor.yml * fix: codec type * fix: improve downgrading to readonly role of optional acc * fix: cleanup
lorisleiva
left a comment
There was a problem hiding this comment.
As discussed offline, reviewing this is extremely challenging so let's get this merge first to unblock the explorer integration and we can make incremental improvements later on.
Summary
Adds
@codama/dynamic-clientruntime 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: