fix(send): hydrate numPubkeys from server-authoritative num_sends - #127
Merged
Merged
Conversation
The wallet tracks the BIP-32 child-index counter (`numPubkeys`) purely in local state. After a seed restore the counter is reset to 0 — but the server may still hold `account.proof = Some(...)` from a previous session for the same address. The next /send then either omits `prev_commitment_pubkey` (server 400 `"prev_commitment_pubkey required for account update"`, mapped client-side to `Interner Fehler: Vorheriger Public Key fehlt.`) or reuses pubkey[0] and collides on the same SMT slot at commit time. The E2E `07-send.spec.ts::send-success` test exposed this on every retry after the first global-setup minted balance was spent. This is the app side of zk-coins/node PR #129, which adds the authoritative `Account.num_sends` counter and emits it on `/api/balance`. The app: * extends `BalanceResponseSchema` with `num_sends: z.number().default(0)`; * adds `syncNumPubkeys(n)` to the wallet store — a no-op fast-path when the counter is already correct, replace semantics otherwise; * drives a fresh `api.balance` BEFORE every send (SendPage) so the counter is hydrated against the server's source of truth even if the WalletScreen 5-s poll hasn't ticked yet; * hydrates `numPubkeys` on every seed/passkey restore flow; * re-syncs on every WalletScreen balance tick + after the post-send /api/balance refresh. Tests: * `wallet.test.ts` — `syncNumPubkeys` replace semantics + no-op shortcut + multi-account safety. * `send-pipeline.test.tsx` — pre-send balance hydration is mocked in every send-confirm path; the existing `prev_commitment_pubkey` + `numPubkeys` assertions still hold. App-PR depends on node-PR being merged + DEV-deployed first — otherwise the wallet reads `undefined` for `num_sends` against the old server. The schema's `.default(0)` keeps the fallback safe (local counter behaviour preserved).
Contributor
[OK] Button-Inventory-Audit — all clearChecked 71 testid(s) in |
2 tasks
4 tasks
TaprootFreak
added a commit
that referenced
this pull request
May 29, 2026
…atency The send-flow now does an extra `/api/balance` round-trip before signing (`num_sends` hydration per the thin-client rule in CONTRIBUTING.md), and the matching server-side writes from zk-coins/node #129 / #132 add a few seconds of legitimate per-send latency for the atomic `Account.num_sends` + `commitment_public_key` upsert. Observed wall-time on the bundle PR's CI runs is 180-220 s, which the previous 180 s test cap was treating as a deadlock. Lift the waitFor cap from 150 s to 250 s and the overall test cap from 180 s to 300 s. The race against the inline error banner stays in place so a genuine server-side failure still surfaces with its real message instead of timing out.
TaprootFreak
added a commit
that referenced
this pull request
May 29, 2026
…es + fresh Goldens (#129) * ci(regen): add spec_glob + tolerate_failures inputs to baseline regen Currently the workflow runs every spec under e2e/0*.spec.ts e2e/1*.spec.ts unconditionally and the whole job aborts (set -e) the moment any spec fails. When the only thing that needs regenerating is the wallet header — and an unrelated spec (e.g. 07-send) is flaky on DEV — the regen can't even reach its commit step. The fix that needs the baselines gets stuck behind a problem it has nothing to do with. Two new workflow_dispatch inputs: - `spec_glob` (default unchanged) — narrows the run to specific files. e.g. `e2e/02-create-seed.spec.ts e2e/03-restore-seed.spec.ts e2e/04-unlock-password.spec.ts` only refreshes the wallet headers. - `tolerate_failures` (default false, opt-in) — appends `|| true` to the playwright invocation. `--update-snapshots=all` still writes new PNGs for the specs that ran; the commit step inspects `git status` and commits only files that actually changed. Partial regens are safe. Defaults preserve current behaviour — full suite, fail-fast — so existing routine calls keep working. * fix(errorMessages): family pattern matching for diagnostic node error strings The node API emits diagnostic, field-naming error strings (e.g. "account_address is not valid hex", "recipient must be 32 bytes (64 hex chars)", "Failed to broadcast commitment inscription on-chain") that did not match the exact-string lookup table on the app side, so users saw the raw "Serverfehler <status>: ..." fallback instead of the translated message. Adds a second lookup layer SERVER_ERROR_PATTERNS (regex) that runs only when the existing exact-match pass misses. KNOWN_SERVER_ERRORS stays unchanged so the lockstep test with api_remote.rs::error_strings_match_known_app_mapping keeps holding. Patterns are ordered specific-before-generic and intentionally do not overlap with any KNOWN_SERVER_ERRORS string — exact-match always wins. Three new test cases in error-mapping.test.ts cover one variant per pattern, including an address-length variant with a different field name to prove the family (not a specific field) is matched. * fix(wallet): case-insensitive network check for faucet gating The node reports `network` as a display string ("Mainnet", "Mutinynet", …) — see `node::router::info_handler`. The existing check used a case-sensitive comparison, so `"Mainnet" !== "mainnet"` evaluated true and the faucet button rendered on Production. Lowercase the network name before the comparison so a casing decision on the server can't accidentally re-enable mint on Mainnet. Add a WalletScreen unit test for both branches (Mainnet hidden, Mutinynet visible). Update the `06-balance-zero-empty-banner` e2e spec to reflect testnet reality (faucet button is the primary CTA in the empty-wallet banner on Mutinynet); the Mainnet-hidden case is covered at the unit-test layer since E2E always runs against the testnet API. * test(e2e): regenerate visual baselines * test(e2e): raise send-success timeout to account for post-#127/#129 latency The send-flow now does an extra `/api/balance` round-trip before signing (`num_sends` hydration per the thin-client rule in CONTRIBUTING.md), and the matching server-side writes from zk-coins/node #129 / #132 add a few seconds of legitimate per-send latency for the atomic `Account.num_sends` + `commitment_public_key` upsert. Observed wall-time on the bundle PR's CI runs is 180-220 s, which the previous 180 s test cap was treating as a deadlock. Lift the waitFor cap from 150 s to 250 s and the overall test cap from 180 s to 300 s. The race against the inline error banner stays in place so a genuine server-side failure still surfaces with its real message instead of timing out. * test(e2e): raise send-success cap to 480 s for Mutinynet jitter + parallel-load The 300 s cap from the previous commit was still insufficient on the bundle PR's CI. Empirically the send-success spec lands between 180 s and ~5 min on DEV. The variance is driven by: * Publisher track-tx WS jitter — 30 s timeout per attempt, up to three attempts under load when Mutinynet block-event delivery stalls (see `node/src/publisher.rs::TRACK_TX_TIMEOUT_SECS`). * Playwright `fullyParallel: true` saturates the shared DEV node with mint + balance traffic from neighbour specs while 07-send-success is its only real-send test, but everything else keeps hammering /api/balance, /api/info, and (via globalSetup) /api/mint — node mutex contention pushes the lone send past its happy path. Raise the spec cap to 480 s and the waitFor cap to 420 s. Deadlock still surfaces; well-behaved-but-slow runs no longer mask as a regression. Follow-ups (out of scope for this PR): (a) tighten the publisher's track-tx budget, (b) consider serializing the mutation-heavy specs against the DEV node. * test(e2e): harden globalSetup against cold-start navigation timeouts The CI runner's first page.goto('/') after boot occasionally exceeds Playwright's default 30 s navigation timeout — Cloudflare cold path, Next.js standalone first-paint, and the WASM bundle handshake all stack on the very first request. Subsequent navigations in the same context are fast. - Raise the context-wide navigation timeout to 90 s for both wallet- bootstrap contexts. Scoped to globalSetup only so test specs keep the 30 s default and a genuine app-side regression still surfaces as a timeout. - Wrap each wallet bootstrap (clearWalletState + createSeedWallet) in a single retry on a fresh page so a transient first-paint timeout doesn't fail the whole CI run. - Fix a TS strictness regression in 07-send.spec.ts where textContent() returns string | null and was passed directly to expect's message slot (string | undefined). * test(e2e): serialize 07-send + raise send-success cap to 12 min The 480 s cap introduced in 3abecfa was still consistently hit by the real Send through DEV under the suite's parallel load. Both attempts (test + retry) timed out, with the waitFor(420 s) draining before the test cap. Empirically the DEV Send takes 180 s to ~7 min depending on Mutinynet block-time jitter plus neighbouring specs hitting the node. Raising the cap is one half of the calibration; the other half is reducing load: - on the file keeps the 13 in-file tests in a single worker so two 07-send tests can no longer fight each other for DEV bandwidth. - (12 min) and the waitFor budgets raised to 660 s (11 min). Setup overhead (login + balance hydration) lives in the remaining 60 s of headroom. A genuine deadlock still surfaces in 11 min instead of 7. The follow-up to tighten publisher track-tx is unchanged. * ci(e2e): give send-success exclusive DEV-node bandwidth via two-step job Three consecutive E2E runs failed on `07-send-success` at progressively larger test-timeout caps (180 s -> 300 s -> 480 s -> 720 s). Every time the DEV node's single proof-gen pipeline was starved by parallel mint and balance traffic from the rest of the suite (09 / 10 / 11), pushing the real Send wall-clock above the cap. Increasing the cap further is the wrong direction. Structural fix: split the existing E2E step in two. Step 1: `npx playwright test --grep-invert "send-success"` -- the parallel-safe bulk of the suite (75 specs) running with the default fullyParallel + 2 workers. Step 2: `npx playwright test --grep "send-success" --workers=1` -- the one mutating Send test with exclusive DEV-node bandwidth, no competing proof-gen / balance traffic from neighbouring specs. Side fixes: - Drop `test.describe.configure({ mode: 'serial' })` from the spec. Serial mode caused a cascade-retry failure where send-no-funds-banner re-ran on the group retry and failed for an unrelated reason. - Lower send-success cap back to 6 min (test) + 5 min (waitFor) since exclusive bandwidth drops the realistic upper bound to ~3-4 min. globalSetup runs once per playwright invocation, so each step gets a clean fresh-Alice + fresh-Bob fixture set independent of the other. * fix(e2e): guard error.textContent() behind race-winner check ROOT CAUSE of all `07-send-success` failures since PR #104. The test races the success heading against the inline error banner via `Promise.race`, then resolves both: await error.textContent().catch(() => null) The catch handles a *rejected* promise, but Playwright's `Locator.textContent()` does not reject when no element matches — it WAITS for the element to appear, blocking up to the test-timeout. On the success page (the happy path) `send-error` is not in the DOM, so textContent never resolves and the assertion at line 216 sat spinning until the 360 s test cap fired, even though the success heading had rendered minutes earlier. Failure-screenshot evidence from run 26651174055: the page clearly shows "Sent privately" with proof #183 / #184 in both attempt 1 and the retry. The Send succeeded server-side; only the post-race assertion blocked. Fix: tag which branch of the race won, then act only on that branch. The error branch resolves textContent (which is safe because the locator just became visible). The heading branch skips the error check entirely. This was the intent of the race-against-error pattern from the start — the implementation just resolved the error too eagerly. Side-effect: with this fix the previous timeout-budget escalations (180 s -> 300 s -> 480 s -> 720 s -> 360 s) were all chasing the wrong cause. The 360 s cap stays — it's a reasonable upper bound for a real DEV-node Send, but the test now resolves as soon as the success heading appears. * test(e2e): refresh 07-send goldens for post-textContent-fix flow The 07-send-success golden was generated against the broken textContent hang (test never reached the snap() call, so no PNG was ever captured). With the race-winner guard in a15cc24 the test now actually takes a fresh screenshot of the success page and compares it to the on-branch golden, which had drifted with the post-2026-05-23 header/balance layout pass. Includes three other 07-send goldens that the same regen run picked up (confirm-dialog desktop/mobile + recipient-valid-hex) — the maxDiffPixelRatio: 0.01 tolerance had been masking the same drift under the threshold; refreshing them now keeps the diffs at 0 px for any future regression that crosses the threshold. Regen run: 26652625153 against chore/all-app-fixes-bundle. * fix: address subagent review findings MAJOR (security): - regenerate-visual-baselines.yml — command injection via `${{ inputs.spec_glob }}` interpolated into a shell string and `eval`'d. Replaced with env-var passthrough + array-form CMD, no eval. Glob expansion still works ($SPEC_GLOB unquoted), but user-controlled metachars are now data, not parser tokens. CWE-78. MINOR: - errorMessages.ts — doc claim "patterns must NOT overlap with KNOWN_SERVER_ERRORS" was wrong (overlap exists today, exact-first shadows it harmlessly). Rewrote the comment to reflect reality and added the test that the comment now references: `every KNOWN_SERVER_ERRORS string resolves via the exact-match table, not via a pattern`. Catches a future refactor that deletes an exact entry and lets a pattern silently swap the German copy. Also exported SERVER_ERROR_PATTERNS + SERVER_ERROR_TO_USER_MESSAGE so the test can introspect them. - _global-setup.ts::withPageRetry — page leaked on the success path (no close after `return await step(page)`) and on the terminal throw. Moved close into a `finally` block. - ci.yaml::e2e-tests — added `timeout-minutes: 30` so a hung browser process can't sit on the default 6 h budget. * fix(errorMessages): trim dead-code assertion + revert unused export Subagent re-review NITs: - error-mapping.test.ts had a tautological inner assertion (userMessageFor always returns the exact entry first, so the conditional check could never fire). Line 76's .toBeDefined() already enforces the real lockstep invariant — trim the rest. - SERVER_ERROR_PATTERNS no longer needs export now that the test doesn't consult it. Reverted to module-private. - SERVER_ERROR_TO_USER_MESSAGE stays exported (the test needs it), marked @internal so production callers stick to userMessageFor(). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-opens PR #125 (closed in error). Resolves the persistent App-side Bug #2 (
07-send-successE2E failure with"Beweisgenerierung fehlgeschlagen"= server 500"prove failed").Why PR #125 was closed
A prior closing comment claimed PR #125 was superseded by zk-coins/node#132 (server-side commitment_public_key store). That reasoning was wrong:
prev_commitment_pubkeyrequest fieldRoot cause (verified via DEV request_log)
The deployed App's bundle reads `numPubkeys` from the local Zustand wallet store, which resets to `0` on every fresh page load and Playwright retry. After Alice's first successful send (server-side `num_sends → 1`), the next E2E retry re-signs with `pk(0)` instead of `pk(1)`. DEV `request_log` row 17952 shows the exact wire signature — `public_key=pk(0)`, `next_public_key=pk(1)` — which is precisely backwards from what the in-circuit constraint requires.
The api_remote test wallet (`TestWallet::sign_send_at(... idx)`) threads the index explicitly, so it never hits this fault — that's why `second_send_succeeds_without_prev_commitment_pubkey_field` is green on DEV but `07-send.spec.ts::send-success` is red.
What this PR does
Hydrate the BIP-32 child-index counter from the server's authoritative `num_sends` immediately before signing each send:
```ts
const preSend = await api.balance(account.address);
setBalance(preSend.balance);
syncNumPubkeys(preSend.num_sends);
const effectiveNumPubkeys = preSend.num_sends;
const keys = wasm.derivePublicKeys(account.xpriv, effectiveNumPubkeys);
const prevPk = effectiveNumPubkeys > 0
? wasm.derivePublicKeys(account.xpriv, effectiveNumPubkeys - 1).publicKey
: undefined;
const res = await api.sendSigned({...}, account.xpriv, effectiveNumPubkeys);
```
Also: WalletScreen's 5 s balance poll syncs `num_sends` so subsequent ticks keep the local store correct; Onboarding's seed/passkey restore syncs immediately after the first balance fetch.
Files changed
Test plan