Skip to content

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

Open
Makabeez wants to merge 1 commit into
KeeperHub:mainfrom
Makabeez:fix/flexible-chainid-marshal
Open

fix(wallet): marshal FlexibleString as a number when numeric#101
Makabeez wants to merge 1 commit 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes-requested Triage: reviewed, changes needed from the contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants