Add display value formatting and provide/inject resolution#1014
Open
lorisleiva wants to merge 1 commit into
Open
Conversation
This PR begins the clear-signing display-text feature (sRFC 39) with its value layer. It adds resolveInjectedValue, which resolves the provide/inject graph — literal values, injected keys (with fallback), account references, and account fields read via an optional fetchAccountData hook returning Kit's decoded Account — within an explicit DisplayResolutionContext. On top of it, it adds the per-display-node value formatters: scaled token amounts (treating decimals as correctness, so an unresolvable scale yields no output rather than a misleading value, while an unresolvable unit merely drops the suffix), ISO 8601 date-times, HH:mm:ss durations, and sliced strings. These are internal building blocks and are not yet exported from the package; the resolution context is kept explicit so the value layer can be exercised in isolation ahead of the interpolation, fallback-list, and orchestration PRs. No changeset is included, consistent with the rest of the 1.7.0 stack.
|
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Jun 25, 2026
mikhd
reviewed
Jun 26, 2026
| * | ||
| * Returns `null` when the value cannot be resolved so callers can fall back safely. | ||
| */ | ||
| export async function resolveInjectedValue( |
Contributor
There was a problem hiding this comment.
question: is argument-sourced injection intentionally scoped out?
What made me unsure was that collectReferencedMembers does handle argumentValueNode (added in PR#1016) but resolveInjectedValue does not. The fix would look like just if (isNode(node, 'argumentValueNode')) return toResolvedValue(context.data[node.name]);, if i'm not mistaken.
Structure example:
instructionNode({
arguments: [
instructionArgumentNode({ name: 'amount', type: numberTypeNode('u64', 'le', {
display: amountNumberDisplayNode({ decimals: injectedValueNode({ key: 'decimals' }) }),
})}),
instructionArgumentNode({ name: 'decimals', type: numberTypeNode('u8') }),
],
provides: [ providedNode('decimals', argumentValueNode('decimals')) ], // <-- argument-sourced provide
})And something like these:
test('it resolves an injected value provided by an argumentValueNode', async () => {
// Given an injected key provided by an argumentValueNode.
const node = injectedValueNode({ key: 'decimals' });
const provides = providesMap(providedNode('decimals', argumentValueNode('decimals')));
// When we resolve it (the decoded argument is present in context.data).
const result = await resolveInjectedValue(node, context({ data: { decimals: 6 }, provides }));
// Then we expect the argument value.
expect(result).toBe(6);
});
test('it resolves an argumentValueNode', async () => {
// Given an argument value node resolved directly.
const node = argumentValueNode('decimals');
// When we resolve it.
const result = await resolveInjectedValue(node, context({ data: { decimals: 6 } }));
// Then we expect the argument value.
expect(result).toBe(6);
});
There was a problem hiding this comment.
Pull request overview
Introduces the initial “display value” layer for dynamic instructions by adding a context-driven provide/inject resolver and a set of value formatters (amount/date-time/duration/string) that can be exercised independently of later interpolation/orchestration work.
Changes:
- Added
DisplayResolutionContextandresolveInjectedValueto resolve literal, injected, and account-derived scalar values. - Added formatter helpers for token-amount scaling, ISO8601 date-times,
HH:mm:ssdurations, and sliced strings. - Added Vitest coverage for injected value resolution and formatter behavior; updated dependencies to include
@solana/accounts.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks new dependency resolution for @solana/accounts. |
| packages/dynamic-instructions/package.json | Adds @solana/accounts dependency for decoded account typing. |
| packages/dynamic-instructions/src/display/types.ts | Defines DisplayResolutionContext and FetchAccountDataFn. |
| packages/dynamic-instructions/src/display/resolve-injected-value.ts | Implements provide/inject + account field resolution into scalar display values. |
| packages/dynamic-instructions/src/display/format-value.ts | Adds amount/date-time/duration/string formatting helpers. |
| packages/dynamic-instructions/src/display/index.ts | Barrels the new display-layer modules. |
| packages/dynamic-instructions/test/display/resolve-injected-value.test.ts | Tests provide/inject and account field resolution behavior. |
| packages/dynamic-instructions/test/display/format-value.test.ts | Tests formatter behavior for scaling/units, timestamps, durations, and slicing. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+51
| export async function resolveInjectedValue( | ||
| node: Node, | ||
| context: DisplayResolutionContext, | ||
| ): Promise<ResolvedDisplayValue> { | ||
| if (isNode(node, 'numberValueNode')) { | ||
| return node.number; | ||
| } | ||
|
|
||
| if (isNode(node, 'stringValueNode')) { | ||
| return node.string; | ||
| } | ||
|
|
||
| if (isNode(node, 'injectedValueNode')) { | ||
| const provided = context.provides.get(node.key); | ||
| if (provided) { | ||
| return await resolveInjectedValue(provided.node, context); | ||
| } | ||
| if (node.fallback) { | ||
| return await resolveInjectedValue(node.fallback, context); | ||
| } | ||
| return null; | ||
| } |
Comment on lines
+89
to
+95
| function toSeconds(value: bigint | number, ticksPerSecond: number | undefined): number | null { | ||
| const ticks = Number(value); | ||
| if (!Number.isFinite(ticks)) return null; | ||
| const divisor = ticksPerSecond ?? 1; | ||
| if (divisor <= 0) return null; | ||
| return ticks / divisor; | ||
| } |
Comment on lines
+76
to
+86
| function scaleByDecimals(value: bigint | number, decimals: number): string | null { | ||
| if (!Number.isInteger(decimals) || decimals < 0) return null; | ||
| if (decimals === 0) return value.toString(); | ||
|
|
||
| const negative = value < 0; | ||
| const digits = (negative ? -BigInt(value) : BigInt(value)).toString().padStart(decimals + 1, '0'); | ||
| const integerPart = digits.slice(0, digits.length - decimals); | ||
| const fractionPart = digits.slice(digits.length - decimals).replace(/0+$/, ''); | ||
| const sign = negative ? '-' : ''; | ||
| return fractionPart ? `${sign}${integerPart}.${fractionPart}` : `${sign}${integerPart}`; | ||
| } |
Comment on lines
+63
to
+66
| /** | ||
| * Presents a string by slicing it to the `[sliceStart, sliceEnd)` range of decoded characters. | ||
| * Both bounds are optional and default to the start and end of the string respectively. | ||
| */ |
mikhd
reviewed
Jun 26, 2026
| if (decimals === 0) return value.toString(); | ||
|
|
||
| const negative = value < 0; | ||
| const digits = (negative ? -BigInt(value) : BigInt(value)).toString().padStart(decimals + 1, '0'); |
Contributor
There was a problem hiding this comment.
Should we also double-guard that value: bigint | number number is not float? Otherwise conversion to BigInt will throw RangeError.
mikhd
approved these changes
Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR begins the clear-signing display-text feature (sRFC 39) with its value layer. It adds resolveInjectedValue, which resolves the provide/inject graph — literal values, injected keys (with fallback), account references, and account fields read via an optional fetchAccountData hook returning Kit's decoded Account — within an explicit DisplayResolutionContext. On top of it, it adds the per-display-node value formatters: scaled token amounts (treating decimals as correctness, so an unresolvable scale yields no output rather than a misleading value, while an unresolvable unit merely drops the suffix), ISO 8601 date-times, HH:mm:ss durations, and sliced strings. These are internal building blocks and are not yet exported from the package; the resolution context is kept explicit so the value layer can be exercised in isolation ahead of the interpolation, fallback-list, and orchestration PRs. No changeset is included, consistent with the rest of the 1.7.0 stack.