Skip to content

feat: SDK hardening integration + MidenClient singleton unification - #189

Closed
WiktorStarczewski wants to merge 29 commits into
mainfrom
wiktor-rltest
Closed

feat: SDK hardening integration + MidenClient singleton unification#189
WiktorStarczewski wants to merge 29 commits into
mainfrom
wiktor-rltest

Conversation

@WiktorStarczewski

@WiktorStarczewski WiktorStarczewski commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Integrates the SDK hardening improvements into the wallet and unifies the MidenClientSingleton from two instances (dispose-and-recreate per-tx) to one long-lived singleton with a late-binding keystore bridge. Net result: cleaner architecture, 2.5× faster sends, and robust error handling for partial-failure scenarios.

SDK hardening integration

Wallet-side integration for the SDK improvements (each has its own SDK PR):

  1. Typed sign-callback failure recovery — when the wallet gets locked mid-transaction, the sign callback throws with { reason: 'locked' }. The TransactionProcessor reads midenClient.lastAuthError() and leaves the tx Queued for retry after unlock, instead of marking it Failed (which previously caused note loss).

  2. ApplyTransactionAfterSubmitFailed handling — when a tx submits on-chain but the local apply step fails, the wallet marks it Completed (not Failed) using the SDK's new errorCode dispatch. The user sees "Transaction sent" instead of a confusing "Transaction failed" for a tx that IS on chain.

  3. WASM call serialization + waitForIdle — the wallet's lock() action uses waitForIdle() to drain in-flight WASM operations before clearing the vault key, preventing the sign-callback race that caused 7/7 executeTransaction errors in stress testing.

Note: an earlier revision of this PR added wallet-side machinery for private-note transport retry (transportPending flag + retryPendingTransports background loop calling resendPrivateNoteById). That has been dropped — superseded by 0xMiden/miden-client#2127's durable NTL outbox at the SDK layer. Original PRs (0xMiden/miden-client#2061, 0xMiden/web-sdk#26) are closed.

MidenClient singleton unification

Collapses MidenClientSingleton from two instances (instance + instanceWithOptions) to one long-lived singleton:

  • New keystore-bridge.ts — pure callback-slot module (activeInsertKey + activeSignCallback). The SDK's permanent keystore callbacks delegate to these mutable slots.
  • New keystore-wiring.ts — subscribes to Effector unlocked/locked events and re-points the bridge's insert-key slot to the active vault's encryptKeystoreEntry method. Called from all 3 entry points (SW main.ts, mobile-adapter.ts, desktop-adapter.ts).
  • Vault.encryptKeystoreEntry() — new method that encrypts a keypair under the vault's KEK without exposing the key outside the Vault class.
  • getMidenClient() no longer accepts options — tests that need a specific seed call MidenClientInterface.create({seed}) directly.
  • Wallet mutex stays active as belt-and-suspenders (pending SDK-side exploration in #2057).
  • withProverFallback uses defaultProver path — relies on the SDK's proveTransactionWithProver(&prover) by-reference fix (#2062) to keep the JS prover handle alive across calls.

Performance

The singleton eliminates the per-tx client dispose-and-recreate cycle (~1-3s overhead per send). Combined with the by-reference prover fix:

Metric Before (dispose-recreate) After (singleton)
Public send ~3.2s ~1.4s
Private send ~8.5s ~2.2s
Conservation

…lock

Depends on miden-client wiktor-test-followups branch via file: ref until
the SDK change is merged and a new version is published.
Transport failures on private-note sends now mark the tx Completed (the
on-chain commit is durable) with a transportPending flag + attempt
counter. A background retry loop in the TransactionProcessor finds
these, backs off exponentially, and calls the SDK's resendPrivateById
until the recipient receives the note blob.

Without this, a transport-side failure left the sender's asset
effectively lost — the recipient has no way to discover private notes
from on-chain data alone. The retry runs on SW startup + every
TransactionProcessor tick; after 20 attempts the tx is flagged Failed
so the user sees a clear terminal state.

Depends on the SDK helper landed on miden-client wiktor-test-followups.
Adds STRESS_TRANSPORT_FAIL_PROB env var. When set (e.g. 0.1), each
private-note send has that probability of having its SendNote gRPC
call intercepted and aborted via Playwright page.route — forcing the
wallet into its transport-pending retry path.

The retry loop inside generateTransactionsLoop should then deliver the
note on the next tick via the SDK's resendPrivateNoteById. Final
balance conservation should still hold; failure to deliver = note loss
= test fail.

Default is 0 so regular stress runs match historical behavior;
passing STRESS_TRANSPORT_FAIL_PROB=0.1 validates the transport retry
end-to-end. Skipped for concurrent ops to keep the signal clean.
…re bridge

Collapse MidenClientSingleton from two instances (instance + instanceWithOptions)
to one. Keystore callbacks are wired permanently at MidenClient.create time via
a late-binding bridge module; Effector unlocked/locked events re-point the
insert-key slot to the active vault. Per-tx sign callbacks go through the
bridge's activeSignCallback slot with a concurrent-set guard.

Key changes:
- New keystore-bridge.ts: pure callback-slot module (insertKey + sign + getKey)
- New keystore-wiring.ts: Effector event subscriptions, called from all 3 entry
  points (SW main.ts, mobile-adapter, desktop-adapter)
- Vault gains encryptKeystoreEntry() method; KEK never leaves the Vault class
- getMidenClient() no longer accepts options; tests use MidenClientInterface.create directly
- withProverFallback builds a fresh TransactionProver.newRemoteProver per-call
  instead of relying on client.defaultProver (which silently falls back to local
  after a single failure and never recovers)
- Wallet mutex stays active as belt-and-suspenders

Validated via stress suite: 5-op run with explicit remote prover shows all ops
completing in ~1.3-2.2s (vs 3.2-8.5s baseline with dispose-recreate per-tx).
Balance conservation held across all test configurations.
The SDK's proveTransactionWithProver now takes &TransactionProver
(by reference), so the JS handle is preserved across calls. Remove
the wallet-side newRemoteProver-per-call workaround and rely on
MidenClient's defaultProver (set from proverUrl at create time).
…pass-through)

The SDK's internal _serializeWasmCall chain + single-instance MidenClient
design handles WASM call serialization. The wallet's AsyncMutex was
belt-and-suspenders from before the singleton unification — needed when
two client instances had independent SDK chains.

Validated by 2 consecutive 5-op stress runs (seeds 301, 302) with mixed
private/public sends, 0 failures, conservation held on both. A third run
failed during faucet deployment (devnet RPC flake, unrelated to wallet code).

Also restores @miden-sdk/miden-sdk to file:../miden-client/crates/web-client.
@0xnullifier

0xnullifier commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

The singleton eliminates the per-tx client dispose-and-recreate cycle (~1-3s overhead per send). Combined with the by-reference prover fix:

Does it ? I mean surely not doing a simple console.time around like this

export async function getMidenClient(options?: MidenClientCreateOptions): Promise<MidenClientInterface> {
  if (options) {
    console.time('Creating MidenClient with options');
    const client = await midenClientSingleton.getInstanceWithOptions(options);
    console.timeEnd('Creating MidenClient with options');
    return client;
  }
  console.time('Getting MidenClient instance');
  const client = await midenClientSingleton.getInstance();
  console.timeEnd('Getting MidenClient instance');
  return client;
}

it consistently takes less than 50ms so idk if this unification does anything in that regards and if it does take 1-2s just to create the client and destroy instance then there is bigger fish to fry

WiktorStarczewski and others added 2 commits April 20, 2026 00:03
Write a stage marker at each observable phase boundary (syncing,
sending, confirming, delivering) during tx processing. The progress
modal reads the active tx's stage and shows a per-stage title +
description instead of a single opaque "Generating Transaction" for
the whole 3-8s spinner. Send-type sub-label varies by tx type
(claim / execute / send). Batch subtitle surfaces remaining-count
when more than one tx is in flight.
…2127) handles it

PR 0xMiden/rust-sdk#2061 (resendPrivateNoteById) is being closed in
favor of #2127's durable NTL relay outbox at the SDK layer. With #2127,
`send_private_note` persists the relay payload to the store before
invoking transport, and `sync_state` flushes the outbox first thing.
Callers no longer have to track which sends failed or schedule retries
— the SDK guarantees eventual delivery.

That makes the wallet's transportPending + retryPendingTransports
machinery duplicate work. Dropped:

- `transportPending`, `transportAttempts`, `transportLastAttemptAt`
  fields on ITransaction (db/types)
- `retryPendingTransports()` + `retryPendingTransportsImpl()` + the
  exponential-backoff constants in transactions.ts (~90 lines)
- The retry loop call from TransactionProcessor (both per-iteration and
  startup-resume)
- The `describe('retryPendingTransports')` test block (~140 lines, 6
  cases) and the 2 transportPending tests in completeSendTransaction
- The CHANGELOG entry for the wallet-side retry feature

The transport-failure handling in `completeSendTransaction` collapses
to: try sendPrivateNote, log any failure, fall through to the normal
Completed path. The on-chain commit is durable; the SDK's outbox will
re-drive the blob to the recipient on the next sync. Two replacement
tests verify Completed-without-transportPending semantics for both the
sendPrivateNote-throws and withWasmClientLock-rejects branches.

The stress-driver's transportFailProb perturbation stays — it now
exercises the SDK's outbox path end-to-end (final balance conservation
should still hold regardless of which layer drives the retry).
@WiktorStarczewski

Copy link
Copy Markdown
Collaborator Author

Cleanup landed: 665a2b697 drops the wallet-side transport-retry machinery now that miden-client#2127's durable NTL outbox handles delivery at the SDK layer. Removed: transportPending/transportAttempts/transportLastAttemptAt fields, retryPendingTransports() + retryPendingTransportsImpl() + backoff constants (~90 LOC in transactions.ts), the TransactionProcessor calls, the describe('retryPendingTransports') test block + 2 transportPending tests in completeSendTransaction (~140 LOC in transactions.branches.test.ts), and the CHANGELOG entry. Two replacement tests cover the Completed-without-transportPending semantics on both sendPrivateNote-throws and lock-rejects branches.

Net: 7 files changed, 45 insertions / 347 deletions. All 740 src/lib/miden/** tests pass, lint clean, ts clean. Description updated to drop the section-3 narrative.

Related closures: miden-client#2061 (PR), web-sdk#26 (companion PR), web-sdk#124 (source issue) — all closed with reasoning that points at #2127.

@WiktorStarczewski

Copy link
Copy Markdown
Collaborator Author

Superseded by #268, a thinned-down version containing only the SDK-hardening improvements backed by external PRs (sign-callback recovery → miden-client#2058; ApplyTransactionAfterSubmitFailed → #2059/#2060; tx-stage UI; stress transport perturbation → #2127). The MidenClient singleton unification and item 3 (waitForIdle, draft #2057) are dropped, and the client singleton is left untouched. Closing in favor of #268.

WiktorStarczewski added a commit that referenced this pull request Jun 9, 2026
…-submit) (#268)

* feat: SDK hardening integration (sign-callback recovery + apply-after-submit)

Thinned from the original #189 to contain only the improvements backed by
outside SDK PRs, dropping the MidenClient singleton unification entirely:

- Typed sign-callback failure recovery: a sign callback that fails because the
  wallet locked mid-tx is recovered via the SDK's lastAuthError(); the tx is
  left Queued for retry after unlock instead of marked Failed
  (miden-client#2058).
- ApplyTransactionAfterSubmitFailed / errorCode dispatch: a tx that submits
  on-chain but fails to apply locally is marked Completed, not Failed
  (miden-client#2059, #2060). InputNoteAlreadyConsumedOnChain is cancelled.
- Transaction-stage progress UI (syncing/sending/confirming/delivering) and
  the stress suite's transport-failure perturbation for the SDK relay outbox
  (miden-client#2127).

Dropped vs #189: the singleton unification (keystore-bridge/keystore-wiring,
Vault.encryptKeystoreEntry, two-instance->one collapse) and item 3
(waitForIdle in lock(), backed by draft #2057). The sign-callback is wired
through the existing getMidenClient(options) path; the client singleton is
untouched.

* test(tx): cover sign-callback recovery branches to clear 95% coverage gate

Export buildSignCallbackError and readLastAuthReason and add branch-coverage
tests for the SDK-hardening error recovery: reason classification, all four
lastAuthError reasons + invalid/throw, the locked->leave-Queued path, and the
wrapped sign callback's success/failure paths. Lifts global branch coverage
back over 95% (the new error-recovery code had introduced uncovered branches).
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.

2 participants