Skip to content

feat(dynamic-client): extract dynamic-instructions#999

Open
mikhd wants to merge 2 commits into
codama-idl:mainfrom
hoodieshq:feat/pr2-dynamic-instructions
Open

feat(dynamic-client): extract dynamic-instructions#999
mikhd wants to merge 2 commits into
codama-idl:mainfrom
hoodieshq:feat/pr2-dynamic-instructions

Conversation

@mikhd
Copy link
Copy Markdown
Contributor

@mikhd mikhd commented May 19, 2026

Description

This PR [2] extracts dynamic-instructions into a separate package with minimal changes. Package dynamic-client was cleaned again and now uses dynamic-instructions.

Previous PR#996 contained split for dynamic-address-resolution

Change

  • Created dynamic-instructions package with initial structure (LICENSE, README, configs).
  • Moved AccountMeta and Instruction building into dynamic-instructions including args encoding, validation.
    • Added generics to exported functions, so they can consume generated types.
    • Uses dynamic-address-resolution for Account address auto-resolution.
  • Updated dynamic-client with @codama/dynamic-instructions.

Public API

export { createAccountMeta } from './accounts';
export { encodeInstructionArguments } from './arguments';
export { createInstructionsBuilder } from './instructions-builder';
export type { InstructionsBuilderFn, EitherSigners } from './shared/types';

// Re-exports
export {
    type AccountsInput,
    type AddressInput,
    type ArgumentsInput,
    isPublicKeyLike,
    type PublicKeyLike,
    type ResolverFn,
    type ResolverFnInput,
    type ResolversInput,
    toAddress,
} from '@codama/dynamic-address-resolution';

mikhd added 2 commits May 19, 2026 09:49
* 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-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 19, 2026

🦋 Changeset detected

Latest commit: 91a79f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@codama/dynamic-instructions Minor
@codama/dynamic-client Patch

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
Copy link
Copy Markdown
Member

@trevor-cortex

Copy link
Copy Markdown

@trevor-cortex trevor-cortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 now mergeBytes(chunks.map(chunk => Uint8Array.from(chunk))). That's a real per-chunk copy on every encode call (notably hot for remainderOptionTypeNode(bytesTypeNode()) payloads like write). Inline below — would be good to confirm this was intentional and, if so, add a comment.
  • Lost memoization: getMemoizedUtf8Encoder() was removed from dynamic-client/src/shared/codecs.ts and the one remaining caller in validators.ts now calls getUtf8Encoder() directly inside a validator closure. Probably negligible, but the memoization was deliberate. Inline below.
  • Changeset bump levels: dynamic-instructions minor / dynamic-client patch looks right — the new package is 0.1.0, and the only user-visible change in dynamic-client is that it now (transitively) depends on dynamic-instructions. The renames (createIxBuilder, BuildIxFn) were internal to dynamic-client and not re-exported from its index, so no breaking external surface there.
  • README cross-link: packages/dynamic-client/README.md now points to ../dynamic-address-resolution/README.md#automatic-resolution-rules. Worth double-checking that anchor exists on main (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}.ts had a handful of memoized codec/encoder helpers and a getCodecFromBytesEncoding utility. Only getMemoizedUtf8Encoder had a remaining caller (in validators.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 from dynamic-client/src/shared/* — I didn't see anything but it's easy to miss.
  • InstructionsBuilderFn now has four generic parameters; TSigners extends EitherSigners = EitherSigners doesn't add any narrowing power (EitherSigners is just string[]). 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. the pmp-idl.json initialize roundtrip via getInitializeInstructionDataDecoder) has been dropped without replacement. Looks like the definedTypeLinkNode → enumTypeNode case 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)));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants