Skip to content

Add offline display dictionary to dynamic-instructions - #1028

Merged
lorisleiva merged 1 commit into
mainfrom
07-20-add_offline_display_dictionary_to_dynamic-instructions
Aug 6, 2026
Merged

Add offline display dictionary to dynamic-instructions#1028
lorisleiva merged 1 commit into
mainfrom
07-20-add_offline_display_dictionary_to_dynamic-instructions

Conversation

@lorisleiva

Copy link
Copy Markdown
Member

This PR adds a serialisable display dictionary to @codama/dynamic-instructions so an offline renderer (typically a hardware wallet) can resolve an instruction's clear-signing display without reaching the network. DisplayDictionary bundles two maps: DisplayAccountMap (address to EncodedAccount, the offline counterpart of the display layer's fetchAccount; only existing accounts are stored, so a missing key means "no data") and a generic DisplayNamedMap (address to name — a .sol domain, token symbol, program label, alias). getRequiredAccountsForDisplay computes the accounts a display would read, statically from the IDL and parsed instruction, sharing the inject-graph walk with the existing consumed-member resolution (extracted into collect-injected-keys.ts; buildDisplayContext split to expose a synchronous base context). getDisplayAccountMap batch-fetches those accounts through a FetchAccountsFn in a single getMultipleAccounts round-trip. Byte codecs (getDisplayDictionaryCodec and per-map codecs, each split into encoder/decoder and combined via combineCodec) serialise the bundle, built lazily to stay tree-shakable. A named-map filler is intentionally not shipped: its data comes from sources Codama has no opinion on.

@changeset-bot

changeset-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bda4447

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

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@lorisleiva
lorisleiva marked this pull request as ready for review July 27, 2026 14:32
@lorisleiva
lorisleiva requested a review from mikhd July 27, 2026 14:37
@lorisleiva

Copy link
Copy Markdown
Member Author

@trevor-cortex

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Summary

Adds an offline DisplayDictionary (accounts + names maps) to @codama/dynamic-instructions so a renderer without network access — typically a hardware wallet — can resolve an instruction's clear-signing display. Three moving parts:

  1. Static plannergetRequiredAccountsForDisplay walks the inject graph statically (no fetching) to compute which account addresses the display would read. The graph walk previously living in resolve-consumed-members.ts is extracted to collect-injected-keys.ts so the consumed-member computation and the planner agree on what "injected" means.
  2. Online fillergetDisplayAccountMap batch-fetches those addresses through a FetchAccountsFn and drops non-existent accounts (a missing key means "no data").
  3. CodecsgetDisplayDictionaryCodec and per-map codecs, each split into …Encoder / …Decoder and combined via combineCodec. All exposed as factory functions to stay tree-shakable (which the package's test:treeshakability script enforces).

Also extracts buildBaseDisplayContext from buildDisplayContext so the planner can reuse the same synchronous context assembly without the async consumedMemberNames resolution.

What I looked at closely

  • Refactor equivalence. The extracted collectInjectedKeys in collect-injected-keys.ts is a faithful move of the previous inline logic — same amount/flatten-struct semantics, same one-level recursion, same injectedValueNode narrowing. The rewrite from imperative forEach + mutable Set to flatMap + new Set(...) at the boundary is a style improvement and preserves behaviour. resolveConsumedMemberNames now consumes the shared helper, so the two callers cannot drift.
  • Planner correctness. resolveFetchedAccountNames recurses through provider chains that re-inject other keys, terminates at accountFieldValueNode (the only node that actually requires account state — a bare accountValueNode just names an existing instruction account), and carries a seen set to break cycles where a provider transitively references its own key. The final dedup by address (not by name) is the right key: two different account names that happen to point at the same address collapse to one fetch.
  • Codec shape. EncodedAccount's address is lifted out as the map key (nice size win, and it matches how the online side keys the map). data uses a u32 size prefix; lamports and space are u64. The getU64Decoder() as Decoder<EncodedAccount['lamports']> cast is the standard workaround for Kit's Lamports brand — no way around that today. The transformEncoder(..., map => new Map(map)) on the map encoders is a defensive copy that lets the sized-map encoder iterate a ReadonlyMap — worth a one-line comment but not blocking.
  • Codecs stay tree-shakable. Every codec is a factory function; nothing is a top-level constant. sideEffects: false and the agadoo treeshakability test cover this.
  • Tests. Genuinely thorough: the planner suite covers the direct field case, the literal-provider case, the missing-provider case, the chained-provider case, address dedup, key dedup across display slots, and the "account declared but no concrete address" edge. The codec suite round-trips empty maps, multi-byte UTF-8 (USD₮), split encoder/decoder interop, and non-existent accounts through getDisplayAccountMap.
  • README + changeset. minor is right for a new export surface, and the README section clearly separates the online (building) and offline (consuming) sides.

Notes for subsequent reviewers

  • Determinism of encoded output. Map iterates in insertion order, so byte-for-byte output depends on the order accounts/names were inserted. That is fine for the current use cases (encode → hand to device → decode), but if downstream ever wants to hash/attest the bundle they'll need to sort before encoding. Not a change I'd ask for here — just something to be aware of.
  • Provider-chain coverage. The planner already handles the injectedValueNode → injectedValueNode → accountFieldValueNode chain (there's a test for one hop), and seen breaks cycles. An explicit cycle test (provider A → B → A) would be cheap insurance if you feel like adding one, but it's genuinely covered by construction.
  • accountValueNode termination. Worth double-checking that no display path today reads bytes off a provider whose terminal is accountValueNode (rather than accountFieldValueNode). Reading the current display resolvers, only field reads decode account data — accountValueNode surfaces the address itself and needs no fetch — so terminating the walk at accountFieldValueNode looks correct. Flagging it because if that ever changes on the display side, this planner will silently under-fetch.

Nice piece of work — the split of buildBaseDisplayContext and the shared collectInjectedKeys helper are exactly the kind of refactor that keeps the two sides honest.

Comment thread packages/dynamic-instructions/src/display/collect-injected-nodes.ts
Comment thread packages/dynamic-instructions/src/display/dictionary.ts Outdated
Comment thread packages/dynamic-instructions/src/display/dictionary.ts Outdated
Comment thread packages/dynamic-instructions/src/display/dictionary.ts
@lorisleiva
lorisleiva force-pushed the 07-20-add_offline_display_dictionary_to_dynamic-instructions branch from 6e5a1fa to f2e6668 Compare August 6, 2026 09:40

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review after the restructuring since my last pass. The PR still ships the offline display dictionary (DisplayDictionary, getRequiredAccountsForDisplay, getDisplayAccountMap, and the lazily-built codecs), but the shared graph walk has been reworked: collect-injected-keys.ts is now collect-injected-nodes.ts (returning nodes so fallbacks are reachable), and a new resolveInjectionTarget collapses provider/fallback chains to their terminal node, shared by both the offline planner and the whenInjected consumed-member computation.

What changed since my last review, verified:

  • Fallback support is a real behavioural fix, not just a refactor: both the planner and consumed-member resolution now follow an injection's fallback when no provider supplies its key, matching what resolveInjectedValue does at runtime. Previously the consumed-member walk only consulted provides.get(key), so a member reachable only through a fallback stayed visible even when its value was displayed. The new tests (it marks an account consumed through an injection fallback, it follows an injection fallback that resolves to an account field) pin this down.
  • Provider-over-fallback precedence matches runtime: resolveInjectionTarget selects the provider even when it later resolves to null, never falling back — exactly mirroring resolveInjectedValue's selection order. The it prefers a provider over the injection fallback tests confirm the planner won't over-fetch through an ignored fallback.
  • Cycle safety (which I suggested testing last time) is now both implemented via the seen guard and explicitly tested — self-referential and mutual cycles, in all three test suites.
  • Dedup semantics are sound: consumed-members now dedups by resolved target rather than injection key. Since chains collapse to the provider node held in the provides map, two slots injecting the same key share a target by reference and resolve (and fetch) once — verified by the toHaveBeenCalledOnce() test. Two slots sharing a key but diverging through different fallbacks are correctly kept distinct.
  • The "returns the terminal instead of splicing it back" design note in resolve-injection-target.ts is a good call — the typed display slots genuinely can't hold an accountFieldValueNode, and it keeps the walk evaluation-free.

One note for a possible follow-up (non-blocking, outside this diff): the runtime resolver resolveInjectedValue still recurses through provides/fallback with no cycle guard, so a cyclic provider graph is only safe on the two paths that pre-collapse through resolveInjectionTarget. The actual display rendering path would still overflow on a malicious/malformed IDL. The changeset wording is correctly scoped to the planner and skip rule, but porting the seen guard into resolveInjectedValue would make cycle safety uniform. Happy to see that land separately.

Points from my previous review that still stand and don't need re-raising: Map insertion-order determinism of the encoded bytes (relevant only if the bundle is ever hashed/attested), and the accountFieldValueNode-only fetch trigger (now well documented inline).

Tests are thorough and the changeset reads well. LGTM.

This PR adds a serialisable display dictionary to `@codama/dynamic-instructions` so an offline renderer (typically a hardware wallet) can resolve an instruction's clear-signing display without reaching the network. `DisplayDictionary` bundles two maps: `DisplayAccountMap` (address to `EncodedAccount`, the offline counterpart of the display layer's `fetchAccount`; only existing accounts are stored, so a missing key means "no data") and a generic `DisplayNamedMap` (address to name — a `.sol` domain, token symbol, program label, alias). `getRequiredAccountsForDisplay` computes the accounts a display would read, statically from the IDL and parsed instruction, sharing the inject-graph walk with the existing consumed-member resolution (extracted into `collect-injected-keys.ts`; `buildDisplayContext` split to expose a synchronous base context). `getDisplayAccountMap` batch-fetches those accounts through a `FetchAccountsFn` in a single `getMultipleAccounts` round-trip. Byte codecs (`getDisplayDictionaryCodec` and per-map codecs, each split into encoder/decoder and combined via `combineCodec`) serialise the bundle, built lazily to stay tree-shakable. A named-map filler is intentionally not shipped: its data comes from sources Codama has no opinion on.

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-checked the full diff against my approval from earlier this morning (09:43 UTC): the change set is identical — same 11 files, same hunks — and I re-read resolve-injected-value.ts on the current head to confirm nothing changed there either. My previous review stands in full, so re-approving without repeating it.

Still outstanding as a non-blocking follow-up (outside this diff): porting the seen cycle guard from resolveInjectionTarget into the runtime resolveInjectedValue, so cycle safety is uniform across the rendering path as well as the planner/skip-rule paths. Happy for that to land separately.

LGTM.

@lorisleiva
lorisleiva force-pushed the 07-20-add_offline_display_dictionary_to_dynamic-instructions branch from f2e6668 to bda4447 Compare August 6, 2026 10:06

@trevor-cortex trevor-cortex left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review — the delta since my approval an hour ago is exactly the non-blocking follow-up I'd suggested: resolveInjectedValue (the display-rendering path) now performs injection selection through the shared resolveInjectionTarget walk, making cycle safety uniform across all three consumers of the provide/inject graph. Verified in detail:

  • Semantics preserved for acyclic graphs: the old inline recursion (provider → recurse, else fallback → recurse, else null) and the new collapse-then-evaluate are observably identical — resolveInjectionTarget applies the same selection order at each hop, just without evaluating. The only behavioural change is the intended one: a cyclic provider graph now resolves to null instead of overflowing the stack.
  • No double-selection: the walk's terminal is never an injectedValueNode, so the re-entrant resolveInjectedValue(target, context) call cannot hit the injection branch again. The inline comment says exactly this — good.
  • Test coverage at both levels: the cycle case is pinned on the raw resolver (resolve-injected-value.test.ts) and through formatAmountValue (format-value.test.ts), so the actual rendering path is covered, not just the unit.
  • Changeset updated accurately — the new final sentence scopes the runtime cycle-safety claim correctly.

With this, my one outstanding follow-up is resolved and nothing new is flagged. Prior notes that still stand without needing action: Map insertion-order determinism of the encoded bytes (only relevant if the bundle is ever hashed/attested), and the accountFieldValueNode-only fetch trigger (well documented inline).

LGTM — good to merge.

@lorisleiva
lorisleiva merged commit a6a9135 into main Aug 6, 2026
5 checks passed
@lorisleiva
lorisleiva deleted the 07-20-add_offline_display_dictionary_to_dynamic-instructions branch August 6, 2026 10:17
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