feat(dynamic-client): extract dynamic-instructions#999
Conversation
* feat: extract dynamic-instructions package * feat: add dynamic-instructions to dynamic-client - Cleaned dynamic-client from dynamic-instructions and import dynamic-instructions * fix: cleanup * fix: cleanup * fix: cleanup * fix: add note to readme * chore: add note to dynamic-instruction typesgen readme * fix: dynamic-client index re-exports
🦋 Changeset detectedLatest commit: 91a79f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
This PR extracts the instruction-building logic out of @codama/dynamic-client into a new @codama/dynamic-instructions package, mirroring the split done for dynamic-address-resolution in #996. dynamic-client now consumes the new package via MethodsBuilder and re-exports a smaller surface; everything that used to live under dynamic-client/src/instruction-encoding, plus the relevant shared helpers and tests, is moved over with the imports rewritten.
Alongside the move, the public API picks up generics (TArgs, TAccounts, TSigners, TResolvers) so generated types can flow through, createIxBuilder is renamed to createInstructionsBuilder, and BuildIxFn becomes InstructionsBuilderFn. Tests are rebuilt against in-memory IDL nodes instead of fixture JSON, which is a nice cleanup — the new package has no fixture/SVM dependency.
Overall the split is clean and the diff is mostly mechanical. A few things worth a closer look below, none of them blocking.
Things to watch out for
- Behavior change in
encodeInstructionArguments:mergeBytes(chunks as Uint8Array[])is nowmergeBytes(chunks.map(chunk => Uint8Array.from(chunk))). That's a real per-chunk copy on every encode call (notably hot forremainderOptionTypeNode(bytesTypeNode())payloads likewrite). Inline below — would be good to confirm this was intentional and, if so, add a comment. - Lost memoization:
getMemoizedUtf8Encoder()was removed fromdynamic-client/src/shared/codecs.tsand the one remaining caller invalidators.tsnow callsgetUtf8Encoder()directly inside a validator closure. Probably negligible, but the memoization was deliberate. Inline below. - Changeset bump levels:
dynamic-instructionsminor /dynamic-clientpatch looks right — the new package is 0.1.0, and the only user-visible change indynamic-clientis that it now (transitively) depends ondynamic-instructions. The renames (createIxBuilder,BuildIxFn) were internal todynamic-clientand not re-exported from its index, so no breaking external surface there. - README cross-link:
packages/dynamic-client/README.mdnow points to../dynamic-address-resolution/README.md#automatic-resolution-rules. Worth double-checking that anchor exists onmain(or is added in the #996 stack) so the link doesn't 404 on npm.
Notes for subsequent reviewers
- The deleted
dynamic-client/src/shared/{bytes-encoding,codecs}.tshad a handful of memoized codec/encoder helpers and agetCodecFromBytesEncodingutility. OnlygetMemoizedUtf8Encoderhad a remaining caller (invalidators.ts); the rest were only used by deleted tests. Worth a quick search to confirm nothing else in the repo (outside these two packages) imported fromdynamic-client/src/shared/*— I didn't see anything but it's easy to miss. InstructionsBuilderFnnow has four generic parameters;TSigners extends EitherSigners = EitherSignersdoesn't add any narrowing power (EitherSignersis juststring[]). Harmless but worth a thought as to whether it earns its place in the public type signature.- The new tests construct IDL nodes inline via
codama's node helpers — this is a good direction and removes the fixture coupling, but worth scanning for whether any meaningful coverage that depended on real fixtures (e.g. thepmp-idl.jsoninitialize roundtrip viagetInitializeInstructionDataDecoder) has been dropped without replacement. Looks like thedefinedTypeLinkNode → enumTypeNodecase is still covered with a synthetic enum, which is the main thing.
| }); | ||
|
|
||
| return mergeBytes(chunks as Uint8Array[]); | ||
| return mergeBytes(chunks.map(chunk => Uint8Array.from(chunk))); |
There was a problem hiding this comment.
Switching from chunks as Uint8Array[] to chunks.map(chunk => Uint8Array.from(chunk)) changes this from a free type assertion to a full per-chunk copy on every encode. For typical instruction args this is fine, but remainderOptionTypeNode(bytesTypeNode()) payloads (the write-instruction shape exercised by the tests) can be large, and this runs in the instruction builder hot path.
What was the motivation? If chunks can legitimately come back as something mergeBytes rejects (e.g. a Node Buffer, a ReadonlyUint8Array-typed value, or a subclass), a short comment explaining that would help future readers — otherwise the previous as Uint8Array[] cast was cheaper. If the chunks are guaranteed to already be Uint8Array-shaped, chunks as Uint8Array[] would be enough.
| return `Expected a string, received: ${formatValueType(value)}`; | ||
| } | ||
| const encoder = getMemoizedUtf8Encoder(); | ||
| const encoder = getUtf8Encoder(); |
There was a problem hiding this comment.
Minor: the old code went through getMemoizedUtf8Encoder() so the encoder was instantiated once per process. This validator closure can run many times per builder, so calling getUtf8Encoder() here means a fresh encoder per validation.
In practice @solana/codecs's encoders are cheap to build, so this is almost certainly negligible — but if you'd like to keep the original behavior without re-introducing the deleted memoization module, hoisting const utf8Encoder = getUtf8Encoder(); to module scope (or just outside the returned validator) would do it.
| resolvedArgumentsInput[argumentNode.name] = await resolverFn(resolvedArgumentsInput, accountsInput); | ||
| resolvedArgumentsInput[argumentNode.name] = await resolverFn( | ||
| resolvedArgumentsInput as TArgs, | ||
| (accountsInput ?? {}) as TAccounts, |
There was a problem hiding this comment.
Small inconsistency worth a sanity check: resolvedArgumentsInput is cast to TArgs even when the caller passed no argumentsInput (so it starts as {}), and accountsInput is defaulted to {} here before being cast to TAccounts. With the new optional-parameter signature elsewhere in the file, undefined arguments/accounts are otherwise preserved as undefined. Defaulting to {} for the resolver call is almost certainly what you want (resolvers reasonably expect an object), but the cast quietly lies to the resolver when TArgs/TAccounts have required keys. Probably fine for the current generics setup — flagging in case you want to document that resolvers should treat their inputs as Partial<…>.
| import type { Address } from '@solana/addresses'; | ||
| import { generateKeyPairSigner } from '@solana/kit'; | ||
|
|
||
| export async function generateAddress(): Promise<Address> { |
There was a problem hiding this comment.
generateAddress doesn't seem to be referenced by any of the new test files in this PR. If it's slated for an upcoming test in this stack, ignore — otherwise this whole file (and the @solana/kit devDependency it pulls in) can be dropped.
Description
This PR [2] extracts
dynamic-instructionsinto a separate package with minimal changes. Packagedynamic-clientwas cleaned again and now usesdynamic-instructions.Previous PR#996 contained split for dynamic-address-resolution
Change
dynamic-instructionspackage with initial structure (LICENSE, README, configs).dynamic-instructionsincluding args encoding, validation.dynamic-address-resolutionfor Account address auto-resolution.dynamic-clientwith@codama/dynamic-instructions.Public API