Skip to content

fix(wallet): marshal FlexibleString as a number when numeric - #101

Merged
suisuss merged 2 commits into
KeeperHub:mainfrom
Makabeez:fix/flexible-chainid-marshal
Aug 17, 2026
Merged

fix(wallet): marshal FlexibleString as a number when numeric#101
suisuss merged 2 commits into
KeeperHub:mainfrom
Makabeez:fix/flexible-chainid-marshal

Conversation

@Makabeez

Copy link
Copy Markdown
Contributor

Follow-up to #87, implementing the decision left on that thread — option (a), MarshalJSON emitting a bare number when the value is numeric.

kh w tokens --json | jq 'select(.chainId == 11155111)' now matches; before this it returned nothing because the field serialised as "11155111" (string) while the filter compared against 11155111 (number).

What changed

Added MarshalJSON to FlexibleString. When the stored value is a bare integer literal, it emits it unquoted; otherwise it falls back to standard string marshalling. The integer check uses digit iteration rather than strconv.ParseFloat, which accepts NaN and Inf and would emit invalid JSON, and which would cap chain IDs at int64 precision.

Tests

Round-trip tests cover:

  • numeric input → numeric output
  • legacy string input → numeric output (UnmarshalJSON normalises to string, MarshalJSON re-emits as number)
  • non-numeric value → stays a string
  • null → zero value ("")

One open question

null currently marshals as "" rather than null, since the zero value of FlexibleString is an empty string. Emitting null would require either a sentinel value or changing the field to a pointer. Happy to do either if you'd prefer that shape — flagging it rather than picking silently.

Implements the decision on KeeperHub#87: --json now emits chainId as a bare number,
matching kh chain list --json and closing the jq footgun documented in
docs/kh_chain_list.md. Non-numeric values still marshal as strings.

Uses a digit check rather than strconv.ParseFloat, which accepts NaN and Inf
and would emit invalid JSON, and which would cap chain ids at int64.

@suisuss suisuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What this changes

Adds MarshalJSON to FlexibleString (cmd/wallet/balance.go) so that a value holding a bare integer literal serializes as an unquoted JSON number instead of always as a string. Non-integer values (including anything with a decimal point, exponent, non-digit character, or a disallowed leading zero) continue to serialize as quoted strings. The check is a hand-rolled digit scan (isJSONInteger) rather than strconv.ParseFloat, specifically to avoid accepting "NaN"/"Inf" as numeric and to avoid capping chain ids at int64. Because FlexibleString is the type of both ChainBalance.ChainID and Token.ChainID, this affects kh wallet balance --json and kh wallet tokens --json identically, bringing both in line with kh chain list --json's existing numeric output.

Does it match the description

Yes. Title and body describe exactly this change; nothing unrelated is touched.

Blocking

  1. No accepted issue - the check-issue-link CI check is currently failing. .github/workflows/pr-issue-link.yml requires the PR title to carry a #<n> resolving to a GitHub Issue (not a PR) labeled accepted; this repo's policy (ISSUES.md) explicitly puts "output format" changes in the required-issue category, which this is. No such issue exists (searched chainId, MarshalJSON, FlexibleString across open/closed issues - nothing). Note that citing #87 in the title would not satisfy the gate either, since #87 is a pull request and the workflow explicitly rejects PR references. That said, the underlying decision to do exactly this isn't in question - it's on record in #87's review thread from the maintainer ("Decision: add MarshalJSON to this field so --json output emits it as a number, matching kh chain list --json"). Closing this is a formality: file an issue capturing that decision, get it accepted, retitle to reference it.

Mechanical - actionable as-is

  • wallet.Token.ChainID shares the exact FlexibleString type and is fixed by this same change, but only wallet.ChainBalance is exercised by the new test. #87's own test suite tested both structs symmetrically for the unmarshal side; matching that pattern here (one extra loop iteration, cheap) would keep coverage symmetric with the sibling struct.
  • The new test covers the four cases named in the PR body (numeric-in, legacy-string-in, non-numeric, null) but not the specific edge cases the isJSONInteger doc comment argues for: a "NaN"/"Inf"-style string (the exact case cited as the reason ParseFloat was rejected), a leading-zero string like "007", and something hex-looking like "0x1a". I traced all three by hand and the current logic handles them correctly (all fall through to quoted-string output), but right now that correctness is provable only by reading the code, not by the test suite - a regression here would ship invisibly. Worth adding a couple of table rows.
  • kh chain list's Long help text already documents this exact string-vs-number footgun for users copying a chain id into a workflow node config (config.network expects a string; chainId now prints as a number). This PR extends the same output shape to kh wallet balance/kh wallet tokens, but neither docs/kh_wallet_balance.md nor docs/kh_wallet_tokens.md carries any equivalent note. Optional, but it's the same trap in two more places.

Verdict

Code is correct and precision-safe (verified by hand-tracing every branch of isJSONInteger against live main, including leading zeros, hex-like strings, scientific notation, and arbitrary-precision digit strings - the raw-byte passthrough on the numeric path avoids any float64 conversion). The only reason this isn't approve-as-is is the process gate: no accepted issue exists yet and check-issue-link is red. Once that's filed/accepted/retitled, I'd approve without further changes; the mechanical suggestions above are nice-to-haves, not blockers.

@suisuss suisuss added the changes-requested Triage: reviewed, changes needed from the contributor label Aug 13, 2026
@suisuss

suisuss commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

On the open question in your description: keep the current "".

Emitting null changes the output shape for every consumer of kh w tokens --json and kh w balance --json, and it is a change to the output contract rather than part of restoring a numeric chainId. If we want a real null there it should land once across the whole output surface, against its own issue, not on one field inside a fix. Nothing to change on your side.

You flagged it rather than picking silently, which was the right call - I should have answered it in the review instead of leaving it in your description.

Assert the marshal round-trip on wallet.Token.ChainID alongside
ChainBalance.ChainID, mirroring TestFlexibleStringAcceptsBothShapes.
Token is the field in the reported reproduction and was untested.

Add rows for "NaN", "Inf", "007" and "0x1a", each asserting the emitted
JSON. These pin the cases that motivated iterating digits instead of
calling strconv.ParseFloat: with ParseFloat, "NaN" and "Inf" marshal to
bare NaN and Inf, and without the leading-zero guard "007" marshals to
bare 007 - all invalid JSON.
@suisuss

suisuss commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Pushed the two test additions rather than sending you round again for them: wallet.Token.ChainID is now asserted alongside ChainBalance.ChainID, and there are rows for NaN, Inf, 007 and 0x1a pinning the cases that motivated iterating digits instead of calling strconv.ParseFloat. Your implementation needed no changes - balance.go is untouched.

Worth recording what those rows now prove, because it is the strongest argument for the approach you chose: values past int64 and past float64's 53-bit mantissa round-trip exactly, including a 78-digit one. ParseFloat would have rounded them silently.

no-issue-required is on this PR. The change is a behaviour change and post-dates the gate, so it would normally need an accepted issue, but the decision it implements was already taken on #87's thread before this PR existed. Making you file an issue to re-decide something we had already settled is a formality, and ISSUES.md:113 provides for exempting exactly that.

Handing it to a second maintainer for the final look. CI on forks needs a run approval from us, which is why your checks have been sitting unrun - that is on our side, not yours.

@suisuss suisuss added no-issue-required PR exempt from the issue-first gate approve Triage: reviewed and good - not a GitHub approval and removed changes-requested Triage: reviewed, changes needed from the contributor labels Aug 17, 2026
@suisuss
suisuss merged commit d5b2be9 into KeeperHub:main Aug 17, 2026
7 checks passed
@eskp eskp mentioned this pull request Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approve Triage: reviewed and good - not a GitHub approval no-issue-required PR exempt from the issue-first gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants