Repository navigation
[2.x] Add formatters for decoded nodes - #1215
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds formatInteger, formatFloat, formatFixedPoint, formatDateTime, formatDuration and formatString to @codama/dynamic-codecs. Each takes the decoded node of its kind and renders it using the presentation metadata of the node that decoded it: unit, display (amount / unit / string display nodes), scale/base, and ticksPerSecond. Scaled numbers go through Kit's @solana/fixed-points (rawDecimalFixedPoint / rawBinaryFixedPoint) so 128-bit and Q-format values are exact; date-times use bigint calendar maths (Hinnant's civil_from_days) so any i64 timestamp formats correctly. Options cover unit placement, Intl.NumberFormat and a resolveInjectedValue callback for display-node inputs.
The core logic is solid. I walked through getCivilDate against the reference algorithm (the floorDiv for era and truncating division for the rest is correct since dayOfEra is always non-negative), floorDiv's sign handling, the nanosecond rounding path (half signed to round away from zero, then truncating division — correct), and the unit precedence (display wins, type unit is the fallback when the display unit is absent, injected-and-unresolved, or resolves to ''), which matches the spec text on amountNumberDisplayNode / unitNumberDisplayNode. Tests are thorough on the number and date paths, including the i64 extremes and a cross-check against Date.prototype.toISOString within JS's range.
Things to address / consider
- Missing changeset.
CONTRIBUTING.mdasks for one for any user-facing change (npx changeset add --empty), and this adds new public exports to@codama/dynamic-codecs. No.changeset/*.mdis in the diff. formatStringslices UTF-16 code units while the spec says indices apply to the "decoded character sequence". See inline — if that's meant to be code points, non-BMP characters (emoji, some CJK) will be split.- Unbounded
decimalsfrom the resolver — see inline onresolveIntegerInput. Low risk, but a cheap guard. - Fractional
ticksPerSecond. BothformatDateTimeandformatDurationreturnnullunlessticksPerSecondis a positive integer. If the spec allows e.g.1/60for minute-resolution timestamps, that's a legit value we'd refuse to format. If the validators already restrict it to positive integers, ignore this — just want to make sure thenullisn't hiding a supported case.
Notes for subsequent reviewers
@solana/codecs@^8.4.0already re-exports@solana/fixed-points, so the new imports (rawDecimalFixedPoint,formatDecimalFixedPoint,Signedness, …) don't need a dependency bump.- The
resolveInjectedValueoption is intentionally a plain callback rather than reusing the package'sProvidedScopemachinery fromgetValueNodeVisitor— display-node inputs likedecimalstypically come from external providers (e.g. a mint account), not from the instruction scope. Worth keeping in mind if someone later wants a default resolver built onscope. - Test coverage gaps that are minor but would be nice:
formatIntegerwith an amount display onshortU16(exercises the special-case ingetIntegerLayout), a 128-bit binary fixed point, a resolved (not just unresolved) injected unit onformatFixedPoint, andformatDurationwith a non-power-of-10ticksPerSecond.
77f82ec to
068e49e
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the diff. Everything actionable from my first pass has been addressed:
formatStringnow slices by Unicode code point (Array.from(...).slice(...).join('')), with tests for both asliceEndand asliceStartlanding after an emoji. README and JSDoc document the semantics.DecimalFixedPoint/BinaryFixedPointare imported directly instead ofParameters<typeof …>[0].numberFormatdocs (JSDoc + README) now say its options decide the digits shown, with the 3-fraction-digit default called out, and that only amounts and fixed points bypass JS floats. There's amaximumFractionDigits: 2test on an amount pinning this.- README documents the
ticksPerSecond→nullbehaviour. - Coverage gaps filled:
shortU16amount (checkeddc0b→ 1500), 128-bit binary fixed point (verified the(2^128 − 1) / 2^64expansion by hand — correct), resolved injected unit onformatFixedPoint, and a locale test on fixed points.
Still open (non-blocking)
- Changeset. There's still no
.changeset/*.mdin the diff. If it's being added at the top of the Graphite stack along with the rest of thedynamic-codecswork, ignore this — just flagging so it doesn't fall through the cracks before the stack merges. decimalsupper bound inresolveIntegerInputis stillMAX_SAFE_INTEGER. Purely defensive; happy to leave it as-is if you'd rather trust the resolver.
Approving — the implementation is correct and well tested.
(Apologies for the delay — GitHub's API was returning 500s on my earlier attempts to post this.)
2e1a5ca to
ce304e7
Compare
068e49e to
7256a9f
Compare
ce304e7 to
361b8ef
Compare
46c664f to
cbd7116
Compare
9b0fbca to
f143ca4
Compare
cbd7116 to
62035f2
Compare
f143ca4 to
fa8eac0
Compare
ebd5a98 to
06c0d0a
Compare
12782e9 to
1248d65
Compare
06c0d0a to
9dbd31f
Compare
1248d65 to
fd9f4ec
Compare
4a978e5 to
1d26fb1
Compare
0ad1db3 to
fcc31ac
Compare
8093254 to
239b195
Compare
fcc31ac to
123c3f8
Compare
239b195 to
e128735
Compare

This PR adds formatters that turn decoded nodes into human-readable strings, using the presentation metadata of the nodes that decoded them: units, display nodes, scales and ticks.
formatInteger,formatFloat,formatFixedPoint,formatDateTime,formatDurationandformatStringeach take the decoded node of their kind.@solana/codecs. Date-times are exact ISO 8601 UTC strings for any year, computed withbigintcalendar maths.%,‰and°.decimals:formatIntegerreturnsnullinstead of a wrongly scaled amount, so callers can show the raw value.formatUnitplaces units, e.g."USD 40.5";numberFormatformats for a locale, its options deciding the digits shown, without ever going through a JavaScript float;resolveInjectedValueresolves injecteddecimalsand units from their path through the decoded node.