Skip to content

walletdk: expose the wallet SDK to mobile via gomobile - #801

Merged
Roasbeef merged 5 commits into
mainfrom
walletdk-mobile-gomobile
Jun 26, 2026
Merged

walletdk: expose the wallet SDK to mobile via gomobile#801
Roasbeef merged 5 commits into
mainfrom
walletdk-mobile-gomobile

Conversation

@Roasbeef

Copy link
Copy Markdown
Member

This series exposes sdk/walletdk to iOS and Android through gomobile bind, so a React Native, Swift, or Kotlin host can drive an embedded
darepod wallet in-process, with no separate daemon binary and no open
socket.

We already do the hard part in sdk/walletdk: Start boots the daemon and
dials it over a private bufconn gRPC transport. So rather than a
falafel-style protoc plugin, we add a thin hand-written facade,
sdk/walletdk/mobile, that translates at the boundary and respects
gomobile's type restrictions.

The facade is callback-free. gomobile carries only a narrow set of types, so
the rich walletdk.Client surface (context, channels, maps, unsigned ints,
time.Time, slices of structs) cannot cross directly. Instead:

  • RPC verbs take and return JSON bytes; a few hot paths return scalars.
  • Start is synchronous and singleton-guarded. It returns once gRPC is
    serving, which walletdk.Start already guarantees, unlike lnd.Main,
    which blocks forever and forces lnd-mobile to use a callback.
  • The one streaming verb hands back a pull-based Subscription instead of a
    channel or a host callback.
  • Every entry point recovers panics into errors, since a panic does not
    cross the gomobile boundary and would otherwise kill the host process.

The package builds only under mobile && walletdkrpc && swapruntime, so
default builds are untouched. make mobile-android / mobile-ios drive
gen_bindings.sh.

What is here

  • sdk/walletdk/mobile: the facade plus its boundary tests.
  • make mobile-* targets and gen_bindings.sh.
  • docs/walletdk_mobile.md.
  • A Mobile Bindings CI workflow: a fast host-tagged compile on PRs and the
    merge queue, with the heavy .aar / .xcframework builds gated to
    mobile-v* tags so routine merges do not trigger them.

What is not here

The sample apps and the idiomatic Kotlin (coroutines / Flow) and Swift
(async / AsyncThrowingStream) wrappers live in
lightninglabs/damobile. Both
were validated end to end against signet: the embedded wallet boots
in-process, connects to the operator mailbox, syncs from Esplora, and runs
on the Android emulator and the iOS simulator.

Closes #713.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a gomobile-safe facade (sdk/walletdk/mobile) over the wallet SDK, enabling Android and iOS hosts to run an embedded darepod wallet in-process. It includes flat translation layers for configuration, lifecycle management (Start/Stop), scalar convenience methods, and JSON-based RPC/streaming interfaces, along with a build script (gen_bindings.sh) and documentation. Feedback on the changes highlights critical race conditions in the Start and Stop lifecycle methods that could lead to leaked daemons or database lock conflicts, as well as a potential script crash in gen_bindings.sh when locating NDK directories under set -euo pipefail.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread sdk/walletdk/mobile/mobile.go Outdated
Comment thread sdk/walletdk/mobile/gen_bindings.sh
@Roasbeef

Copy link
Copy Markdown
Member Author

Follow-up: #802 tracks the mobile mailbox wake-and-drain lifecycle (push-driven wake instead of the always-on ingress loop), which was open question #2 from #713. This PR ships the bindings with the existing always-on loop; #802 scopes the processOnce factoring and the push integration on top.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa18d6fde4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/walletdk/mobile/mobile.go Outdated
Comment thread sdk/walletdk/mobile/config.go
Comment thread docs/walletdk_mobile.md Outdated
@Roasbeef
Roasbeef requested a review from sputn1ck June 26, 2026 01:07
@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 2m 8s —— View job


Review: walletdk mobile gomobile bindings

I read the full facade (mobile.go, config.go, wallet.go, convenience.go), the tests, gen_bindings.sh, and the doc, and cross-checked against sdk/walletdk/types.go and lwwallet/tip_poller.go. Overall this is a clean, well-documented boundary layer — callback-free, panic-recovering, singleton-guarded, build-tag gated. Nice work. I do agree with the bot reviewers on several points and have a few additions; my independent assessment below.


🔴 1. The Start/Stop lifecycle race is real and worth fixing now

Both Gemini and Codex flagged this, and I confirmed it against the code. The window is genuine:

  • Start CASes started 0→1 (mobile.go:55), then blocks in walletdk.Start for up to startTimeout (90s).
  • If Stop runs during that window, it CASes started 1→0 (mobile.go:102), sees state.client == nil, and returns nila no-op that stops nothing.
  • When walletdk.Start finally returns, Start publishes a live client with started == 0 (mobile.go:87-91). The daemon is now running but unreachable by Stop, and the next Start CAS 0→1 succeeds and boots a second daemon on the same data dir → bolt lock / port conflict, and the first daemon leaks.

This breaks the exact singleton guarantee the package advertises, and an app-lifecycle Stop racing a background Start is the normal mobile case (user backgrounds the app mid-boot), not a corner case. The atomic.Int32 CAS can't express "starting" — it conflates starting and started.

The fix both bots converged on is right: replace the two-state atomic with an explicit state machine (stopped/starting/started/stopping) under state.mu, and store the startCancel in the state so Stop during starting cancels the in-flight walletdk.Start (it already takes the startCtx, so cancellation unwinds the dial), then have Start's success path re-check status == starting under the lock and client.Stop() if it changed. The stopping state also closes Gemini's second window (a new Start arriving before client.Stop() finishes). I'd lean toward the state-machine approach over patching the CAS.

Fix this →


🟠 2. Negative / out-of-range config values can crash the host (Codex P2 — confirmed, high impact)

config.go:113-117 passes wallet_poll_interval_seconds straight into a time.Duration. I traced where it lands: lwwallet/tip_poller.go:267 does time.NewTicker(t.pollInterval) inside pollLoop, a background goroutine spawned after startup. A negative interval makes NewTicker panic — and because it's a separate goroutine, the recover() in Start (mobile.go:60) cannot catch it. That panic crosses no boundary; it takes down the host process. This defeats the whole "recover panics into errors so they never kill the host" design goal.

Same class of bug for the unchecked signed→unsigned conversion at config.go:119: WalletRecoveryWindow int64uint32(mc.WalletRecoveryWindow) silently wraps a negative or >2³² value (e.g. -14294967295). buffer_size and max_operator_fee_sat deserve a non-negative check too.

Since the gomobile boundary forces signed ints, applyMobileConfig (or parseConfig) is the right place to validate and return a startup error for negative durations/windows/sizes rather than letting malformed JSON detonate later.

Fix this →


🟠 3. The doc's JSON field-name claim is wrong (Codex P2 — confirmed)

docs/walletdk_mobile.md:39-42 states the JSON field names "are the json:"…" tags of the walletdk DTOs." I checked sdk/walletdk/types.go: the DTOs carry no struct tags. encoding/json therefore emits and accepts the Go field names verbatim — Version, ConfirmedSat, PendingInSat, AmountSat, IdentityPubKey, etc. (PascalCase). A mobile host modeling kotlinx.serialization/Codable from snake_case names following this doc will silently decode nothing and send zero-valued requests.

Two options: (a) fix the doc to say fields are the Go field names (PascalCase) and show a real example payload, or (b) add json tags to the walletdk DTOs and key the doc/config off those — more invasive but gives a stable, idiomatic wire contract. Given this is the public mobile contract, I'd weakly prefer (b) long-term, but (a) is the correct minimal fix for this PR. Either way, TestEntryRoundTripsAsJSON only proves Go↔Go round-trip; it does not pin the wire names a foreign decoder sees — consider an assertion on the actual JSON key strings.

Fix this →


🟡 4. gen_bindings.sh NDK glob under set -euo pipefail (Gemini — minor, valid)

gen_bindings.sh:42 — the -d "${ANDROID_HOME}/ndk" guard at line 41 only proves the directory exists, not that it has children. If ndk/ is empty, ls -d .../ndk/* fails to glob and exits non-zero; under set -e the assignment-from-command-substitution aborts the script with a confusing error. Gemini's find -mindepth 1 -maxdepth 1 -type d rewrite is the clean fix. Edge case, but a five-line hardening.


Smaller observations (non-blocking)

  • IsRunning() during boot (convenience.go:58): returns false for the entire starting window because activeClient requires a non-nil client. A host polling it after kicking off Start on a background thread sees "not running" for up to 90s. With the state machine from lib: start adding lib helpers #1, consider reporting starting as running (or document the gap). The doc says "started and not yet stopped," which doesn't quite match.
  • Subscription.Next clean-cancel signal (wallet.go:345-356): on Close/Stop, the derived ctx cancels and walletdk.Subscribe likely pushes context.Canceled onto errs. Next would then surface context.Canceled rather than io.EOF, so a host loop distinguishing "clean end" from "error" by EOF will treat a normal close as a failure. Worth confirming what the upstream stream emits on cancel and documenting it.
  • Marshal consistency: verbs use the marshal helper (wrapped error) but Next calls json.Marshal directly (wallet.go:358). Harmless, just slightly inconsistent error wrapping on the stream path.
  • Tests: the lifecycle race in lib: start adding lib helpers #1 is exactly the kind of thing a small concurrent Start/Stop test (with a fake/slow walletdk.Start) would lock down once the state machine lands. The current tests cover the happy/not-started/bad-config paths well but not the concurrency contract that's the heart of the singleton guarantee.

Verdict

The architecture (bytes-out + pull-based subscription + synchronous singleton Start) is the right shape for gomobile and reads well. The lifecycle race (#1) and config validation (#2) are the two I'd want addressed before merge — both can crash or leak in the ordinary mobile suspend/resume flow, which is the package's whole reason to exist. The doc fix (#3) prevents silent integration breakage for the very first consumer. #4 and the smaller notes are polish.
· branch walletdk-mobile-gomobile

@Roasbeef
Roasbeef force-pushed the walletdk-mobile-gomobile branch from aa18d6f to ac9c418 Compare June 26, 2026 21:41
@Roasbeef

Copy link
Copy Markdown
Member Author

Addressed the review feedback and force-pushed (branch rebased onto latest
main, fixups autosquashed into their target commits, so the history stays at
five clean commits).

  • Lifecycle race in Start/Stop (gemini + codex, P1): replaced the
    single started/stopped CAS with a four-state machine (stopped / starting /
    started / stopping) plus a stored startCancel and a generation counter. A
    Stop that races an in-progress Start now cancels the boot; the Start
    then tears down any client it produced instead of publishing it under a reset
    guard, and Stop only returns after client.Stop() completes. (in
    walletdk: add gomobile-safe mobile binding package)
  • gen_bindings.sh empty-NDK glob (gemini): the NDK lookup now uses find
    instead of a * glob, so an empty ndk/ directory no longer aborts the
    script under set -euo pipefail. (same commit)
  • Negative signed config (codex, P2): parseConfig now rejects a negative
    wallet_poll_interval_seconds (which would otherwise panic the tip poller's
    ticker), recovery window, fee, and buffer size, with a test covering them.
    (config in the package commit, test in walletdk: test the mobile binding boundary)
  • Doc JSON field names (codex, P2): corrected docs/walletdk_mobile.md to
    state that the verb DTOs have no json tags, so the wire keys are the Go
    field names (PascalCase: Version, ConfirmedSat, …), with the snake_case
    Start config called out as the one tagged exception. (in docs: add walletdk mobile bindings guide)

The host wrappers in lightninglabs/damobile already map the PascalCase response
keys, so that side is consistent.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 2m 20s —— View job


Re-review: walletdk mobile bindings (post-fixup)

I re-read the whole facade against the force-pushed branch and traced the lifecycle state machine and the subscription path through walletdk/client.go. The four blocking/major items from the last round are all addressed correctly. One previously-noted minor item is still open, plus a couple of small new notes.

  • Lifecycle state machine in mobile.go
  • Config validation in config.go
  • gen_bindings.sh NDK fix
  • Doc JSON field-name fix
  • Subscription EOF / IsRunning follow-ups
  • Post review

✅ Verified fixed

1. Start/Stop lifecycle race — the four-state machine + generation counter is correct. I traced the scenarios that were broken before:

  • Stop during starting (mobile.go:165-176) cancels the boot and bumps gen; when the in-flight walletdk.Start returns, the success path re-checks status == statusStarting && gen == gen (mobile.go:132) and tears the client down instead of publishing it. No stranded daemon.
  • Start during stopping (mobile.go:78) now returns already started rather than booting a second daemon over a half-finished shutdown — Stop only flips to statusStopped after client.Stop() returns (mobile.go:198-200).
  • Stop-then-Start before the first boot returns is handled by the gen mismatch — the stale Start stops its own client and the new one keeps its starting state. The defer's status == statusStarting && gen == gen ownership guard (mobile.go:109) prevents it clobbering the new boot. Nicely done.

2. Negative config (config.go:86-107) — validate() rejects negative wallet_poll_interval_seconds / wallet_recovery_window / max_operator_fee_sat / buffer_size, with TestParseConfigRejectsNegativeScalars covering all four. This closes the background-goroutine NewTicker panic that Start's recover couldn't catch.

3. Doc JSON field names (docs/walletdk_mobile.md:42-49) — now correctly states the DTOs carry no json tags so the wire keys are the Go PascalCase names, and calls out the Start config as the one snake_case tagged exception. Matches sdk/walletdk/types.go.

4. gen_bindings.sh NDK glob (gen_bindings.sh:41-51) — replaced with find -mindepth 1 -maxdepth 1 -type d | sort -V | tail -1, guarded by a non-empty check. No longer aborts on an empty ndk/ under set -euo pipefail.


🟡 Still open — Subscription.Next reports a clean host-side close as an error, not io.EOF

This was a smaller note last round and isn't in the changelog. I confirmed it end-to-end. Next's comment and the doc (wallet.go:333-336, walletdk_mobile.md:83) both promise io.EOF "at clean end," and Next reports io.EOF only when errs reads nil (wallet.go:351-355). But Close()/Stop() cancel the derived ctx, and the upstream goroutine in walletdk.Subscribe pushes a non-nil error onto errs on cancellation — either from stream.Recv() returning a non-EOF status, or the explicit errs <- "wallet subscription closed: %w", ctx.Err() branch (client.go:599 and client.go:608). So a host that closes its own subscription gets a wrapped context.Canceled out of Next, not io.EOF.

For the doc's finally { sub.close() } pattern this is benign (the loop is already unwinding), but a Stop() that fires while a Flow/AsyncStream is actively collecting surfaces that cancellation as a thrown error to the collector rather than clean completion — exactly the app-suspend path. Recommend Next translate a self-initiated cancellation to io.EOF: store the subscription's ctx and, when streamErr != nil && ctx.Err() != nil (or errors.Is(streamErr, context.Canceled)), return io.EOF. Either fix the behavior or soften the "io.EOF at clean end" wording to cover the cancel case.

Fix this →


Smaller notes (non-blocking)

  • IsRunning() during boot (convenience.go:58): still returns false for the whole starting window, since activeClient needs a non-nil client. The doc's "started and not yet stopped" doesn't quite cover the up-to-90s starting gap. With the state machine in place this is a one-liner if you want it to report statusStarting as running — or just document the gap.
  • WalletRecoveryWindow upper bound (config.go:154): negatives are now rejected, but a positive > math.MaxUint32 still silently wraps in uint32(...). Absurd value, so optional — an upper-bound check in validate() would make the conversion total.
  • Wire-name test gap: TestEntryRoundTripsAsJSON still only proves Go↔Go round-trip; it doesn't pin the literal PascalCase keys a foreign decoder sees. Since that's now the documented public contract, a strings.Contains(string(b), "\"AmountSat\"")-style assertion would lock it down cheaply.
  • Next marshal consistency (wallet.go:358): still calls json.Marshal directly instead of the marshal helper the verbs use. Harmless, just inconsistent error wrapping on the stream path.

Verdict

The three blocking concerns (lifecycle race, config validation, doc contract) are resolved cleanly, and the state-machine reasoning holds up under the race scenarios I traced. The remaining Subscription.Next cancel-vs-EOF mismatch is the only behavioral item I'd still call out — it touches the app-suspend path the package exists to serve — but it's minor and the rest are polish. Looks good to merge once that's either fixed or the doc wording is softened.
· branch walletdk-mobile-gomobile

@Roasbeef
Roasbeef force-pushed the walletdk-mobile-gomobile branch from ac9c418 to 74c8033 Compare June 26, 2026 21:59
Roasbeef added 5 commits June 26, 2026 15:22
In this commit, we add sdk/walletdk/mobile, a thin facade that exposes
the embedded walletdk wallet to iOS and Android through gomobile bind.
gomobile carries only a narrow set of types across the language
boundary, so the existing walletdk.Client surface (context.Context,
channels, maps, unsigned integers, time.Time, and slices of structs)
cannot be bound directly.

The facade translates at the edge. RPC verbs take and return JSON
bytes, a handful of hot paths return plain scalars, and the one
streaming verb hands back a pull-based Subscription rather than a
channel or a host-implemented callback. Start is synchronous and
singleton-guarded via a CAS; it owns an internal context that Stop
cancels, so the boundary never has to express a context, and in-flight
work unwinds on shutdown. Every entry point recovers panics into
errors, since an unrecovered panic does not cross the gomobile boundary
and would otherwise take down the host process.

Unlike falafel and lnd-mobile we keep the bytes-out shape but drop the
callback interfaces: walletdk.Start returns once gRPC is serving,
whereas lnd.Main blocks forever, so a synchronous Start is both
possible and simpler.

The package builds only under the mobile, walletdkrpc, and swapruntime
tags together, matching walletdk's embedded wallet runtime requirement,
so default builds are unaffected. gen_bindings.sh drives gomobile bind
for the Android .aar and the iOS .xcframework.
In this commit, we cover the mobile facade against a bufconn-embedded
daemon: the config JSON decodes onto the right walletdk.Config fields,
verbs fail cleanly before Start, Start rejects a malformed config and
resets the singleton so a retry can succeed, Stop is idempotent, and an
Entry survives a JSON round trip with its optional progress and request
unions intact. The suite is tagged like the package (mobile,
walletdkrpc, swapruntime) and runs on the host toolchain, so it does
not need gomobile.
In this commit, we add mobile, mobile-android, and mobile-ios make
targets that drive sdk/walletdk/mobile/gen_bindings.sh. The targets
give a stable entry point for building the Android .aar and iOS
.xcframework, and are what the bindings CI and the damobile fetch
scripts invoke.
In this commit, we document the mobile binding design: the gomobile
type restrictions the facade works around, the JSON bytes-out
convention, the callback-free lifecycle (synchronous Start, pull-based
Subscribe), and how to build the .aar / .xcframework. The sample apps
and host wrappers live in the separate lightninglabs/damobile repo,
which this guide points to.
In this commit, we add a Mobile Bindings workflow. On pull requests and
the merge queue it runs a fast host-tagged compile and the boundary
tests, so a change that breaks the facade is caught cheaply. The
expensive gomobile builds cross compile the whole embedded daemon for
every ABI, so they run only when a mobile-v* tag is pushed, producing
the Android .aar and iOS .xcframework as artifacts rather than on every
merge.
@Roasbeef
Roasbeef force-pushed the walletdk-mobile-gomobile branch from 74c8033 to 3faf8be Compare June 26, 2026 22:23
@Roasbeef

Copy link
Copy Markdown
Member Author

Thanks for the thorough re-review. Addressed the open item and the polish
notes, and fixed the fmt-changed failure (had run only gofmt locally, not
llformat). Force-pushed; still five clean commits.

  • Subscription.Next cancel-vs-EOF (the behavioral one): Next now stores
    the subscription's ctx and, when updates closes with a non-nil errs,
    returns io.EOF if s.ctx.Err() != nil (a self-initiated Close/Stop)
    rather than the wrapped context.Canceled. So a Stop that fires while a
    Flow/AsyncStream is collecting (the app-suspend path) now completes the
    host loop cleanly, matching the doc. A genuine upstream error still
    propagates. (in the package commit)
  • IsRunning() boot window: now backed by the lifecycle status, so it
    reports true across the whole starting window (not only once gRPC is
    serving), with the doc noting a true during boot does not yet imply RPCs
    succeed (use WalletReady).
  • Recovery-window upper bound: validate() now also rejects
    wallet_recovery_window > math.MaxUint32, so the uint32 narrowing is total;
    covered by a new test.
  • Wire-name test: TestEntryRoundTripsAsJSON now asserts the literal
    PascalCase keys ("ID", "Kind", "AmountSat") so the documented public
    contract is pinned, not just a Go-to-Go round trip.
  • Next marshal consistency: switched from json.Marshal to the shared
    marshal helper.

Verified locally: make fmt-changed-check clean, mobile test gate green
(including the new cases), all five commits pass commitmsg-lint.

The unit failures in the previous run were a flaky db/actordelivery
worker-pool test (TestDurableActorPerKeyFIFOUnderWorkerPool) unrelated to
this branch, which touches no non-mobile code.

@Roasbeef

Copy link
Copy Markdown
Member Author

@claude review this

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @Roasbeef's task in 2m 27s —— View job


Re-review: walletdk mobile bindings (third pass)

I re-read the full facade on the current branch and traced the two things the latest fixups touch most: the Stop-during-starting cancellation path (does it actually tear the daemon down?) and the Subscription.Next EOF translation. The previously-flagged items all hold up. I found one genuinely new edge in the config validation, plus one residual lifecycle gap worth a sentence.

  • Re-read mobile.go / config.go / wallet.go / convenience.go
  • Confirm the four prior fixes landed
  • Trace Stop-during-startingwalletdk.Start teardown
  • Trace Next cancel-vs-EOF
  • Post review

✅ Confirmed resolved

  • Lifecycle race — the four-state machine + gen counter is correct, and I verified the one assumption it leans on: when Stop cancels startCtx mid-boot, walletdk.Start's waitForReady failure path (embedded.go:182-194) cancel()s runCtx and waitForRunExits before returning the error, so the daemon is fully torn down — no leak. The narrow window where walletdk.Start returns a live client despite a racing Stop is caught by the status != statusStarting || gen != gen re-check (mobile.go:132) calling client.Stop(). Sound.
  • Subscription.Next cancel→EOF (wallet.go:351-368) — streamErr != nil && s.ctx.Err() == nil returns the real error; a self-initiated Close/Stop (ctx cancelled) falls through to io.EOF. Matches the doc and the app-suspend path. marshal helper now used (wallet.go:371).
  • Config validation + IsRunning boot window + recovery-window upper bound + wire-name test — all present and correct (config.go:87-125, convenience.go:55-62, mobile_test.go:105-113,193-197).

🟡 New — wallet_poll_interval_seconds still overflows into the negative duration the validation was meant to prevent

validate() rejects a negative poll interval (good — that's the NewTicker panic from the last round), but the conversion at config.go:166-170 is

cfg.WalletPollInterval = time.Duration(mc.WalletPollIntervalSeconds) * time.Second

time.Second is 1e9, so any wallet_poll_interval_seconds greater than ~9.2e9 (math.MaxInt64 / 1e9) overflows the int64 multiplication and wraps to a negative time.Duration — which lands in lwwallet's time.NewTicker(t.pollInterval) in the post-startup background goroutine and panics the host process, exactly the failure mode fix #2 closed for the negative case. It's the same overflow class you already handled for wallet_recovery_window (config.go:119), just missed for the duration field — and it's the higher-impact one, because the panic fires in a goroutine Start's recover can't catch.

An upper-bound check in validate() (e.g. reject WalletPollIntervalSeconds > math.MaxInt64/int64(time.Second)) makes the conversion total. Absurd input, but it's host-supplied JSON and the consequence is a process kill, so it's worth the two lines for parity with the recovery-window guard.

Fix this →


Smaller note (non-blocking)

  • Stop during starting returns before the in-flight boot's teardown completes. The statusStarting branch (mobile.go:165-176) cancels startCancel, flips to statusStopped, and returns nil immediately — but the racing Start goroutine is still inside walletdk.Start, synchronously running waitForRunExit (bolt close, listener close) before it observes the cancellation. The statusStarted path is careful to wait for client.Stop() before reporting stopped; the starting path is not. So a host that does Stop() → immediate Start() (the suspend-then-resume race) can have the new walletdk.Start collide with the old daemon's still-in-progress shutdown on the same data dir. Unlike the original race this can't strand or double-run a daemon — it surfaces as a transient bolt-lock / listener startup error from the second Start, which the host can retry — so it's minor. If you want Stop to be a true barrier in this branch too, have the starting Stop wait on a done signal the boot goroutine closes after walletdk.Start returns, rather than returning eagerly. Worth at least a doc sentence noting Stop is not a teardown barrier when it races a boot.

Verdict

Architecture and the three blocking concerns from the prior rounds are resolved and hold up under tracing. The only actionable item this pass is the poll-interval overflow — a small, in-character addition to the validation you already wrote — and it closes the last way malformed host JSON can kill the process via the background-ticker path. The Stop-races-boot teardown note is genuinely minor. Looks good to merge once the overflow guard is in (or consciously waived as absurd input).
· branch walletdk-mobile-gomobile

@Roasbeef
Roasbeef merged commit 408cba3 into main Jun 26, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sdk/walletdk: expose the wallet SDK to mobile via gomobile

1 participant