fix(wallet): marshal FlexibleString as a number when numeric - #101
fix(wallet): marshal FlexibleString as a number when numeric#101Makabeez wants to merge 1 commit into
Conversation
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
left a comment
There was a problem hiding this comment.
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
- No accepted issue - the
check-issue-linkCI check is currently failing..github/workflows/pr-issue-link.ymlrequires the PR title to carry a#<n>resolving to a GitHub Issue (not a PR) labeledaccepted; this repo's policy (ISSUES.md) explicitly puts "output format" changes in the required-issue category, which this is. No such issue exists (searchedchainId,MarshalJSON,FlexibleStringacross open/closed issues - nothing). Note that citing#87in 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 itaccepted, retitle to reference it.
Mechanical - actionable as-is
wallet.Token.ChainIDshares the exactFlexibleStringtype and is fixed by this same change, but onlywallet.ChainBalanceis 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
isJSONIntegerdoc 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.networkexpects a string;chainIdnow prints as a number). This PR extends the same output shape tokh wallet balance/kh wallet tokens, but neitherdocs/kh_wallet_balance.mdnordocs/kh_wallet_tokens.mdcarries 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.
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 against11155111(number).What changed
Added
MarshalJSONtoFlexibleString. 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 thanstrconv.ParseFloat, which acceptsNaNandInfand would emit invalid JSON, and which would cap chain IDs at int64 precision.Tests
Round-trip tests cover:
"")One open question
null currently marshals as
""rather thannull, since the zero value ofFlexibleStringis an empty string. Emittingnullwould 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.