Add offline display dictionary to dynamic-instructions - #1028
Conversation
🦋 Changeset detectedLatest commit: bda4447 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
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:
- Static planner —
getRequiredAccountsForDisplaywalks the inject graph statically (no fetching) to compute which account addresses the display would read. The graph walk previously living inresolve-consumed-members.tsis extracted tocollect-injected-keys.tsso the consumed-member computation and the planner agree on what "injected" means. - Online filler —
getDisplayAccountMapbatch-fetches those addresses through aFetchAccountsFnand drops non-existent accounts (a missing key means "no data"). - Codecs —
getDisplayDictionaryCodecand per-map codecs, each split into…Encoder/…Decoderand combined viacombineCodec. All exposed as factory functions to stay tree-shakable (which the package'stest:treeshakabilityscript 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
collectInjectedKeysincollect-injected-keys.tsis a faithful move of the previous inline logic — same amount/flatten-struct semantics, same one-level recursion, sameinjectedValueNodenarrowing. The rewrite from imperativeforEach+ mutableSettoflatMap+new Set(...)at the boundary is a style improvement and preserves behaviour.resolveConsumedMemberNamesnow consumes the shared helper, so the two callers cannot drift. - Planner correctness.
resolveFetchedAccountNamesrecurses through provider chains that re-inject other keys, terminates ataccountFieldValueNode(the only node that actually requires account state — a bareaccountValueNodejust names an existing instruction account), and carries aseenset 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'saddressis lifted out as the map key (nice size win, and it matches how the online side keys the map).datauses au32size prefix;lamportsandspaceareu64. ThegetU64Decoder() as Decoder<EncodedAccount['lamports']>cast is the standard workaround for Kit'sLamportsbrand — no way around that today. ThetransformEncoder(..., map => new Map(map))on the map encoders is a defensive copy that lets the sized-map encoder iterate aReadonlyMap— 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: falseand theagadootreeshakability 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 throughgetDisplayAccountMap. - README + changeset.
minoris 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.
Mapiterates 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 → accountFieldValueNodechain (there's a test for one hop), andseenbreaks 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. accountValueNodetermination. Worth double-checking that no display path today reads bytes off a provider whose terminal isaccountValueNode(rather thanaccountFieldValueNode). Reading the current display resolvers, only field reads decode account data —accountValueNodesurfaces the address itself and needs no fetch — so terminating the walk ataccountFieldValueNodelooks 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.
6e5a1fa to
f2e6668
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
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
fallbackwhen no provider supplies its key, matching whatresolveInjectedValuedoes at runtime. Previously the consumed-member walk only consultedprovides.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:
resolveInjectionTargetselects the provider even when it later resolves tonull, never falling back — exactly mirroringresolveInjectedValue's selection order. Theit prefers a provider over the injection fallbacktests 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
seenguard 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
providesmap, two slots injecting the same key share a target by reference and resolve (and fetch) once — verified by thetoHaveBeenCalledOnce()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.tsis a good call — the typed display slots genuinely can't hold anaccountFieldValueNode, 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
left a comment
There was a problem hiding this comment.
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.
f2e6668 to
bda4447
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
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 —resolveInjectionTargetapplies the same selection order at each hop, just without evaluating. The only behavioural change is the intended one: a cyclic provider graph now resolves tonullinstead of overflowing the stack. - No double-selection: the walk's terminal is never an
injectedValueNode, so the re-entrantresolveInjectedValue(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 throughformatAmountValue(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.

This PR adds a serialisable display dictionary to
@codama/dynamic-instructionsso an offline renderer (typically a hardware wallet) can resolve an instruction's clear-signing display without reaching the network.DisplayDictionarybundles two maps:DisplayAccountMap(address toEncodedAccount, the offline counterpart of the display layer'sfetchAccount; only existing accounts are stored, so a missing key means "no data") and a genericDisplayNamedMap(address to name — a.soldomain, token symbol, program label, alias).getRequiredAccountsForDisplaycomputes 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 intocollect-injected-keys.ts;buildDisplayContextsplit to expose a synchronous base context).getDisplayAccountMapbatch-fetches those accounts through aFetchAccountsFnin a singlegetMultipleAccountsround-trip. Byte codecs (getDisplayDictionaryCodecand per-map codecs, each split into encoder/decoder and combined viacombineCodec) 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.