Skip to content

fix(wallet): accept numeric chainId in balance and token responses - #87

Merged
suisuss merged 1 commit into
KeeperHub:mainfrom
Makabeez:fix/flexible-chainid
Aug 9, 2026
Merged

fix(wallet): accept numeric chainId in balance and token responses#87
suisuss merged 1 commit into
KeeperHub:mainfrom
Makabeez:fix/flexible-chainid

Conversation

@Makabeez

@Makabeez Makabeez commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

kh w balance and kh w tokens crash on v0.13.1 against the current API:

json: cannot unmarshal number into Go struct field ChainBalance.balances.chainId of type string

The API now returns chainId as a JSON number; ChainBalance and Token both declare it as string. Go's encoding/json hard-errors on this mismatch. Reproducible on every invocation, with and without --json or --chain.

Fix

Adds FlexibleString — a type alias for string with a custom UnmarshalJSON that accepts both a quoted string and a bare number, normalising both to string. No behaviour change for callers that already receive a string chainId.

Changed files:

  • cmd/wallet/balance.goFlexibleString definition + ChainBalance.ChainID type change
  • cmd/wallet/tokens.goToken.ChainID type change (reuses FlexibleString)
  • cmd/wallet/flexstring_test.go — new: failing-before / passing-after test covering both JSON shapes
  • balance_test.go, tokens_test.gogofmt alignment fixes (widened type name shifted column)

Test

go test ./cmd/wallet/...   # ok
gofmt -l cmd/wallet/       # (empty — clean)

Discovered during a fresh Linux/WSL2 onboarding run on v0.13.1 (2026-08-05).

@Makabeez
Makabeez force-pushed the fix/flexible-chainid branch from 479190e to 31f8ca2 Compare August 5, 2026 21:58
@Makabeez

Makabeez commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Context for reviewers — this was found during a fresh Linux/WSL2 onboarding run on v0.13.1 (2026-08-05). Full teardown with repro steps and all findings:

https://github.com/Makabeez/carrydesk/blob/main/onboarding-teardown.md

Five other friction points hit during the same session (not all PRs, some just doc gaps):

  1. No Linux install path in quickstart — had to find the binary manually from GitHub Releases
  2. kh --version → "unknown flag"; correct form is kh version
  3. kh auth login docs say "opens a browser window" — it's actually a device-code flow; codes expired repeatedly on a headless box before the URL could be opened
  4. kh wallet info shells out to npx @keeperhub/wallet — undocumented Node dependency, fails with "could not determine executable to run"
  5. This PRkh w balance crashes with the numeric chainId type mismatch

Items 1, 3, and 4 are covered in the companion doc PR: #88. SSH marketplace blocker: KeeperHub/claude-plugins#5.

Also amended since the initial push: dropped balance_test.go and tokens_test.go from the commit — they had pre-existing gofmt whitespace issues unrelated to this fix. Expanded the test to cover both ChainBalance and Token since both structs were changed.

@Makabeez
Makabeez force-pushed the fix/flexible-chainid branch from 31f8ca2 to 024b16a Compare August 5, 2026 22:01

@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.

Welcome - first PR here. Briefly how we review: the diff gets read independently of the description first, then the two are compared. Conventions are in the repo's contributing guide.

What this changes

Three files. cmd/wallet/balance.go adds type FlexibleString string with a pointer-receiver UnmarshalJSON: if the first byte is " it delegates to json.Unmarshal into a string, otherwise it assigns the raw bytes verbatim and returns nil. ChainBalance.ChainID and Token.ChainID change from string to FlexibleString. cmd/wallet/flexstring_test.go is new and table-tests the two chainId shapes for both structs.

Does it match the description

Matches. The stated problem is real - on main, kh w balance and kh w tokens fail with json: cannot unmarshal number into Go struct field ... of type string when the API returns chainId as a number, and this fixes that.

Blocking

  • cmd/wallet/balance.go:36 - the else branch accepts every non-string JSON value, not just numbers, because encoding/json hands UnmarshalJSON the complete next value. JSON null has first byte n, so ChainID becomes the four-character string "null". -> The API returns {"chainId":null,...} and kh w tokens prints null in the CHAIN column, while --json emits "chainId": "null". On main the same input leaves the field as "", because encoding/json skips null for a plain string. That is a regression, and it is also contrary to the Unmarshaler contract, which asks that UnmarshalJSON([]byte("null")) be a no-op. -> Return early without assigning when string(b) == "null".

  • cmd/wallet/balance.go:35-37 - the same branch swallows objects, arrays and booleans. {"chainId":{"x":1}} returns nil with ChainID set to {"x":1}; [1,2] and true likewise. -> A version-skewed or malformed response that previously produced a clear decode error now prints {"x":1} in the CHAIN column and hands it to jq consumers as a chain id. -> Accept only a JSON number: check b[0] is - or a digit, or unmarshal into json.Number and take its String(). That keeps the fix and restores the error for everything else.

Mechanical - actionable as-is

  • cmd/wallet/flexstring_test.go:9 - the comment // current API - panics before this fix is wrong. On main the decode returns a *json.UnmarshalTypeError; nothing panics. Worth correcting since it is the only description of the bug in the test.
  • cmd/wallet/flexstring_test.go - the table covers the two shapes that work and none of the ones above. Cases for null, an object and a bool would have caught both blockers.
  • cmd/wallet/balance.go:24 - there is no MarshalJSON, so --json now always emits chainId as a string even when the input was a number. Not a regression against main for numeric input, since that errored out entirely, but it locks in a string-only output contract that disagrees with kh chain list --json, which emits a number - a divergence docs/kh_chain_list.md:10-13 already calls out as a footgun. kh w tokens --json | jq 'select(.chainId == 11155111)' will match nothing.
  • cmd/wallet/balance.go:36 - strings.TrimSpace is dead on this path; encoding/json passes the literal with no surrounding whitespace.
  • cmd/wallet/flexstring_test.go:1 - declared package wallet while every other test in the directory is package wallet_test. Both compile; matching the neighbours is the smaller surprise.

Needs a decision

  • Should FlexibleString also marshal back as a number, or is string-out the intended CLI contract? (a) Add MarshalJSON emitting a bare number when the value is numeric, costs a little complexity and makes --json output shape depend on input; (b) keep string-out and align kh chain list --json to match, costs a breaking change on that command; (c) accept the divergence and document it, costs the jq footgun staying. This is the fix-selection question behind the third mechanical item.

Verdict

Changes requested - the numeric case is fixed correctly, but the else branch is wider than the problem and turns null and malformed values into printable chain ids.

The auth label on this PR is an automated file-path heuristic firing on "tokens"; nothing credential-related is touched here.

@suisuss suisuss added changes-requested Triage: reviewed, changes needed from the contributor decision-needed Blocked on a maintainer decision, not on the contributor labels Aug 6, 2026
kh w balance and kh w tokens fail against the current API with:
  json: cannot unmarshal number into Go struct field ChainBalance.balances.chainId of type string

The API returns chainId as a JSON number; the structs expect a string.
Adds FlexibleString, which unmarshals either shape, so older responses
keep working. Reproducible on v0.13.1 with and without --json/--chain.
@Makabeez

Makabeez commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Full onboarding teardown now published: https://github.com/Makabeez/carrydesk/blob/main/onboarding-teardown.md

@Makabeez

Makabeez commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed still reproducible on v0.14.0 (just released): kh w balance returns json: cannot unmarshal number into Go struct field ChainBalance.balances.chainId of type string on every invocation.

@suisuss

suisuss commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What changed since the last review

  • null handling in UnmarshalJSON - addressed: cmd/wallet/balance.go now returns early with string(b) == "null", leaving the zero value, and flexstring_test.go:12 asserts {"chainId":null} -> "".
  • Non-string/non-number values (object, array, bool) swallowed silently - addressed: the else branch now decodes into json.Number, which errors on those shapes, and flexstring_test.go:33-45 asserts an error for {"x":1}, true, and [1,2].
  • Test comment "panics before this fix" - addressed: now reads "returned *json.UnmarshalTypeError before this fix".
  • Table missing null/object/bool coverage - addressed: all three now covered.
  • Dead strings.TrimSpace in the unmarshal path - addressed: removed.
  • flexstring_test.go package mismatch (wallet vs wallet_test) - addressed: now package wallet_test.
  • No MarshalJSON, so --json always emits chainId as a string even for numeric input, diverging from kh chain list --json - not addressed, still true on this diff. This is the open decision below.

Also dropped from this push per the contributor's comment: the gofmt alignment fixes in balance_test.go/tokens_test.go that were unrelated to the fix. Confirmed - the diff no longer touches either file.

Blocking

None remaining.

Mechanical - actionable as-is

None remaining.

Needs a decision

  • Should FlexibleString also marshal back as a number, or is string-out the intended CLI contract? (a) add MarshalJSON emitting a bare number when the value is numeric, costs extra complexity and makes --json output shape depend on input; (b) keep string-out and align kh chain list --json to match, costs a breaking change on that command; (c) accept the divergence and document it, costs the jq footgun already called out in docs/kh_chain_list.md staying. Not addressed in the comments since the last review.

Verdict

Both blockers from the prior review are fixed with tests covering the failure cases; the only open item is the string-vs-number output contract, which is a maintainer call, not a defect.

@suisuss suisuss added approve Triage: reviewed and good - not a GitHub approval and removed changes-requested Triage: reviewed, changes needed from the contributor labels Aug 9, 2026
@suisuss

suisuss commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Decision: add MarshalJSON to this field so --json output emits it as a number, matching kh chain list --json. Keep the two output contracts consistent rather than diverging or documenting the split.

@suisuss suisuss removed the decision-needed Blocked on a maintainer decision, not on the contributor label Aug 9, 2026
@suisuss
suisuss merged commit 1c6be81 into KeeperHub:main Aug 9, 2026
4 checks passed
@eskp eskp mentioned this pull request Aug 9, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants