walletdk: browser (js/wasm) wallet bindings - #803
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces browser WASM support for the walletdk client by adding a syscall/js adapter, browser-compatible SQLite and migration drivers backed by OPFS, and WASM stubs for native packages. It also replaces the neutrino LRU cache with a custom implementation to avoid native dependencies. The review feedback identifies critical issues that must be addressed: a potential nil pointer dereference in readWriteBucket.Get, thread-safety issues in the new ancestryTreeLRU cache under concurrent access, and a memory leak in the WASM promise wrapper due to unreleased js.Func callbacks.
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.
| // When an empty byte array is stored as the value, Sqlite will decode | ||
| // that into nil whereas postgres will decode that as an empty byte | ||
| // array. Since returning nil is taken to mean that no value has ever | ||
| // been written, we ensure here that we at least return an empty array | ||
| // so that nil checks will fail. | ||
| if len(*value) == 0 { | ||
| return []byte{} | ||
| } | ||
|
|
||
| return *value |
There was a problem hiding this comment.
If the database value is NULL (which is the case for bucket keys or if a NULL value is somehow stored), row.Scan(&value) will set value to nil. Attempting to dereference value via len(*value) will then cause a guaranteed nil pointer dereference panic. We must check if value is nil before dereferencing it.
| // When an empty byte array is stored as the value, Sqlite will decode | |
| // that into nil whereas postgres will decode that as an empty byte | |
| // array. Since returning nil is taken to mean that no value has ever | |
| // been written, we ensure here that we at least return an empty array | |
| // so that nil checks will fail. | |
| if len(*value) == 0 { | |
| return []byte{} | |
| } | |
| return *value | |
| if value == nil { | |
| return nil | |
| } | |
| // When an empty byte array is stored as the value, Sqlite will decode | |
| // that into nil whereas postgres will decode that as an empty byte | |
| // array. Since returning nil is taken to mean that no value has ever | |
| // been written, we ensure here that we at least return an empty array | |
| // so that nil checks will fail. | |
| if len(*value) == 0 { | |
| return []byte{} | |
| } | |
| return *value |
| "container/list" | ||
| "context" | ||
| "crypto/sha256" | ||
| "errors" |
There was a problem hiding this comment.
To support concurrent access to the ancestry tree cache, we need to import the sync package so we can add a mutex to ancestryTreeLRU.
| "container/list" | |
| "context" | |
| "crypto/sha256" | |
| "errors" | |
| "container/list" | |
| "context" | |
| "crypto/sha256" | |
| "errors" | |
| "sync" |
| type ancestryTreeLRU struct { | ||
| maxEntries int | ||
| order *list.List | ||
| items map[[sha256.Size]byte]*list.Element | ||
| } |
There was a problem hiding this comment.
The custom ancestryTreeLRU cache is not thread-safe. Since the database store can be accessed concurrently by multiple goroutines (e.g., concurrent gRPC requests or background workers), concurrent reads and writes to the items map and order list will cause a panic. We should add a sync.Mutex to protect these fields.
| type ancestryTreeLRU struct { | |
| maxEntries int | |
| order *list.List | |
| items map[[sha256.Size]byte]*list.Element | |
| } | |
| type ancestryTreeLRU struct { | |
| mu sync.Mutex | |
| maxEntries int | |
| order *list.List | |
| items map[[sha256.Size]byte]*list.Element | |
| } |
| func (c *ancestryTreeLRU) Get(key [sha256.Size]byte) (*ancestryTreeCacheValue, | ||
| error) { | ||
|
|
||
| entry, ok := c.items[key] | ||
| if !ok { | ||
| return nil, errAncestryTreeCacheMiss | ||
| } | ||
|
|
||
| c.order.MoveToFront(entry) | ||
| cacheEntry, ok := entry.Value.(*ancestryTreeCacheEntry) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid ancestry tree cache entry") | ||
| } | ||
|
|
||
| return cacheEntry.value, nil | ||
| } |
There was a problem hiding this comment.
Acquire the mutex lock in Get to ensure thread-safe access to the LRU cache.
func (c *ancestryTreeLRU) Get(key [sha256.Size]byte) (*ancestryTreeCacheValue,
error) {
c.mu.Lock()
defer c.mu.Unlock()
entry, ok := c.items[key]
if !ok {
return nil, errAncestryTreeCacheMiss
}
c.order.MoveToFront(entry)
cacheEntry, ok := entry.Value.(*ancestryTreeCacheEntry)
if !ok {
return nil, fmt.Errorf("invalid ancestry tree cache entry")
}
return cacheEntry.value, nil
}| func (c *ancestryTreeLRU) Put(key [sha256.Size]byte, | ||
| value *ancestryTreeCacheValue) (bool, error) { | ||
|
|
||
| if entry, ok := c.items[key]; ok { | ||
| cacheEntry, ok := entry.Value.(*ancestryTreeCacheEntry) | ||
| if !ok { | ||
| return false, fmt.Errorf("invalid ancestry tree " + | ||
| "cache entry") | ||
| } | ||
|
|
||
| cacheEntry.value = value | ||
| c.order.MoveToFront(entry) | ||
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| entry := c.order.PushFront(&ancestryTreeCacheEntry{ | ||
| key: key, | ||
| value: value, | ||
| }) | ||
| c.items[key] = entry | ||
|
|
||
| if c.order.Len() > c.maxEntries { | ||
| c.removeOldest() | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
There was a problem hiding this comment.
Acquire the mutex lock in Put to ensure thread-safe access to the LRU cache.
func (c *ancestryTreeLRU) Put(key [sha256.Size]byte,
value *ancestryTreeCacheValue) (bool, error) {
c.mu.Lock()
defer c.mu.Unlock()
if entry, ok := c.items[key]; ok {
cacheEntry, ok := entry.Value.(*ancestryTreeCacheEntry)
if !ok {
return false, fmt.Errorf("invalid ancestry tree " +
"cache entry")
}
cacheEntry.value = value
c.order.MoveToFront(entry)
return false, nil
}
entry := c.order.PushFront(&ancestryTreeCacheEntry{
key: key,
value: value,
})
c.items[key] = entry
if c.order.Len() > c.maxEntries {
c.removeOldest()
}
return true, nil
}| func promise(fn func() (any, error)) any { | ||
| executor := js.FuncOf(func(_ js.Value, args []js.Value) any { | ||
| resolve, reject := args[0], args[1] | ||
|
|
||
| go func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| reject.Invoke( | ||
| jsError( | ||
| errors.New( | ||
| "walletdk panic"), | ||
| ), | ||
| ) | ||
| } | ||
| }() | ||
|
|
||
| res, err := fn() | ||
| if err != nil { | ||
| reject.Invoke(jsError(err)) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| resolve.Invoke(res) | ||
| }() | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| return js.Global().Get("Promise").New(executor) | ||
| } |
There was a problem hiding this comment.
In Go's syscall/js, every js.Func created via js.FuncOf allocates resources in both the Go runtime and the browser that are only freed when Release() is called. Since promise is called for almost every API interaction, failing to release the executor callback will cause a severe memory leak. Because the JS Promise constructor invokes the executor synchronously, we can safely release the executor callback immediately after New(executor) returns.
func promise(fn func() (any, error)) any {
var executor js.Func
executor = js.FuncOf(func(_ js.Value, args []js.Value) any {
resolve, reject := args[0], args[1]
go func() {
defer func() {
if r := recover(); r != nil {
reject.Invoke(
jsError(
errors.New(
"walletdk panic"),
),
)
}
}()
res, err := fn()
if err != nil {
reject.Invoke(jsError(err))
return
}
resolve.Invoke(res)
}()
return nil
})
p := js.Global().Get("Promise").New(executor)
executor.Release()
return p
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7187cfa4a7
ℹ️ 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".
|
|
||
| // browserSQLiteFileName maps native paths to stable origin-local OPFS names. | ||
| func browserSQLiteFileName(name string) string { | ||
| base := filepath.Base(name) |
There was a problem hiding this comment.
In WASM this reduces every configured SQLite path to only its basename, so defaults like .../data/regtest/client.db and .../data/signet/client.db both open the same OPFS file /client.db (and the swap store similarly collides on /swaps.db). In a single browser origin, switching networks or data dirs would silently reuse/mix the prior network's ledger/VTXO/swap state. Encode or hash the normalized full path rather than dropping it to filepath.Base.
Useful? React with 👍 / 👎.
| cfg := stringify(req) | ||
|
|
||
| return promise(func() (any, error) { | ||
| return js.Null(), mobile.Start(cfg) |
There was a problem hiding this comment.
When walletdkCall("start") is invoked without a request or with {}, this passes no data_dir override through mobile.Start, leaving walletdk.DefaultConfig() with DataDir == "~/.darepod". In a browser wasm_exec.js starts with an empty Go environment, so darepod.Validate() expands ~ via os.UserHomeDir() and fails with $HOME is not defined before the wallet can start. Set a WASM-safe default data dir before calling mobile.Start or make tilde expansion browser-aware.
Useful? React with 👍 / 👎.
| return nil | ||
| }) | ||
|
|
||
| return js.Global().Get("Promise").New(executor) |
There was a problem hiding this comment.
Every wallet method call creates a new js.Func executor and returns the Promise without ever calling Release. Promise executors are invoked synchronously by the constructor and are not needed afterward, so repeated UI polling of balance, status, next(), etc. leaks Go callback handles for the lifetime of the page. Release the executor after constructing the Promise.
Useful? React with 👍 / 👎.
|
@claude review this |
|
Claude finished @Roasbeef's task in 2m 59s —— View job Claude Code ReviewI reviewed the WASM bindings against 🔴 1.
|
ac9c418 to
74c8033
Compare
20027e7 to
5b66749
Compare
Verified end-to-end in a real browserThe build passing didn't prove the OPFS/worker runtime path actually works, so I drove the
The two non-green steps aren't binding bugs: the harness points the light wallet at a dead Esplora ( So the whole thing genuinely works: the blob boots, OPFS SQLite persists across reloads, the full embedded daemon comes up in one browser VM, and the bridge round-trips real RPCs. One fix from this: |
5b66749 to
6b83af4
Compare
74c8033 to
3faf8be
Compare
|
@claude review this |
|
Claude finished @Roasbeef's task in 3m 38s —— View job Claude Code Review (re-review)I re-reviewed the current tree (
So the only carryover bug is #3. 🟠 1.
|
d9b8d7d to
cf31b4a
Compare
|
Addressed the last open item: Validated with the |
cf31b4a to
e976f01
Compare
Add wasm-safe SQLite open, migrate, and error handling through go-wasmsqlite. Browser builds use OPFS while native builds keep the existing database/sql path. Move lwwallet walletdb and seed storage behind native and wasm files so browser wallets can persist state in OPFS.
The embedded daemon path was walled off from js/wasm builds: embedded.go, embedded_config.go, errmap.go, and the swapruntime / walletdkrpc tag pairs all carried a !js constraint, so walletdk.Start, the wallet RPC subserver, and the error-reconstruct interceptor did not exist in a browser build at all. Drop those constraints so the in-process bufconn runtime compiles under GOOS=js, and disable the public HTTP gateway on the embedded path, since a browser has no TCP listener to bind. sdk/ark splits its embedded boot into a native file (embedded.go, now tagged !js || !wasm) and a browser stub (embedded_wasm.go) whose StartEmbedded reports that the native daemon runtime is unavailable, with the shared grpc readiness helper lifted into transport.go. The browser wallet boots the daemon through walletdk.Start rather than sdk/ark, so the stub keeps the package compiling for js consumers that only need the remote Ark types.
Expose the embedded walletdk runtime to browser JavaScript as a thin syscall/js adapter over the sdk/walletdk/mobile JSON facade. Every verb takes a JS request object and resolves a Promise with the decoded JSON response, so the daemon, swap, and OOR machinery all run in the single browser VM with no separate gateway process. Routing through the shared mobile facade keeps one source of truth with the gomobile bindings, so the bridge cannot drift from the walletdk.Client API the way a bespoke per-method dispatch would. A native stub keeps go build ./... green off-target, where the js/wasm main has no entry point, and a wasm-wallet make target assembles the stripped blob alongside wasm_exec.js and the go-wasmsqlite OPFS runtime assets so the browser bundle is buildable in one step.
e976f01 to
b91300a
Compare
2 participants
In this PR, we add the WebAssembly wallet bindings for
walletdk, so thefull embedded daemon (wallet + swaps + OOR) runs inside a single browser VM
with no separate gateway process. This supersedes #431, which had drifted out
of sync with
mainover the past few months: the OOR actor set was rewritten(the old global signing-effect actor is gone), the
walletdk.Clientsend pathmoved to a prepare/dispatch split, and
sdk/walletdkitself was reorganizedaround the
walletdkrpcrename. Rather than untangle that by hand, we rebuiltthe wasm portions from scratch against current
main.This work stacks on top of #713 (the
sdk/walletdk/mobilegomobile facade), sothe base branch here is
walletdk-mobile-gomobile, notmain. The browserbridge drives that same JSON facade, which means the wasm and gomobile bindings
now share one source of truth, and the bridge can't drift from the
ClientAPIthe way the old hand-written
syscall/jsswitch did. That drift is exactly whatbroke #431 (
client.Sendno longer exists).What's in here
We pulled in the OPFS-backed SQLite storage layer from the original branch (the
db,internal/sqlbase, and per-subsystem*_wasm.gostubs), then did twothings differently from #431.
First, there's no inline OOR actor. #431 hand-rolled an
inlineOORActorReftorun the legacy global OOR actor synchronously in the browser, but
maindeletedthat actor entirely in favor of the durable registry + per-session actors. It
turns out those new durable actors compile and run under
js/wasmas-is on topof the OPFS SQLite mailbox, so the whole inline-actor concept just goes away.
Second, the browser bridge in
cmd/walletdk-wasmis a thinsyscall/jsadapterover the
sdk/walletdk/mobileJSON facade. Every verb takes a JS request objectand resolves a
Promisewith the decoded JSON response. It's ~280 lines insteadof the old ~700, and nothing in it reaches into
walletdk.Clientdirectly.On the build side, we drop the
!jswalls that fenced the embedded daemon(
embedded.go,errmap.go, theswapruntime/walletdkrpctag pairs) offfrom
js/wasm, disable the public HTTP gateway on the embedded path (a browserhas no TCP listener to bind), and split
sdk/arkinto native + wasm halves.make wasm-walletthen builds the stripped blob and drops it next towasm_exec.jsand the go-wasmsqlite OPFS runtime assets. None of theapp-specific bits from #431 came along: no React demo, no Playwright harness, no
Pages workflow.
See each commit message for the incremental detail.
Validation
cc @jamaljsr