Repository navigation
Release retained wallet material on reset - #953
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens wallet reset/logout semantics by ensuring retained treasury state is released and by preventing late-arriving async initialization/synchronization work from republishing state after a reset.
Changes:
- Add
WalletTreasury.reset()and a generation counter to invalidate in-flight updates and clear retained profiles/derived account state. - Introduce a
walletSessionidentifier inwalletStoreto ignore async init/sync work that completes after a reset. - Add/extend lifecycle tests covering treasury reset behavior, init reset races, and aborted sync settling after reset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/wallet-treasury/index.js | Add reset/generation invalidation and guard treasury sync work against stale sessions. |
| src/lib/wallet-treasury/tests/index.spec.js | Add coverage asserting treasury reset discards profiles and derived state. |
| src/lib/stores/walletStore.js | Add wallet-session gating and improved sync/reset coordination to avoid post-reset republishing. |
| src/lib/stores/tests/walletStore.spec.js | Add tests for init-reset race and aborted-sync settling after reset; adjust timer expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
25221fe to
eec901b
Compare
eec901b to
b76ee98
Compare
There was a problem hiding this comment.
🟡 Changes recommended
WalletTreasury.update() currently reads unspent nullifiers where spent nullifiers are required, which can break the spent/unspend reconciliation logic during sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| // gather all spent nullifiers in the cache | ||
| const currentSpentNullifiers = | ||
| await walletCache.getUnspentNotesNullifiers(); | ||
| ensureCurrent(); |
There was a problem hiding this comment.
Confirmed with a cache-state regression: after the network reports a previously spent note as unspent, it remains incorrectly cached as spent. The same wrong lookup predates this PR. Opened the narrowly scoped fix separately as #963, using getSpentNotesNullifiers() for the second check and verifying both cache sets after reconciliation. The regression fails before the fix and passes afterward.
542a393 to
8f67b99
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cross-tab invalidation reload currently won’t happen if logout(true) rejects, leaving the tab potentially in a stale state after a detected reset.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
| // Reload after cancellation to discard cached login info and preferences | ||
| // on welcome routes as well as authenticated routes. | ||
| logout(true).then(() => location.reload()); | ||
| } |
8f67b99 to
23f79d4
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core wallet lifecycle and synchronization invalidation logic across multiple layers (routing, unlock, store, treasury), so it needs final human review despite strong test additions.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
Propagate sync failures and cancellation to transaction callers while handling background failures explicitly through syncStatus. Coordinate login removal, replacement and conditional migration writes with native Web Locks. Keep password encryption inside its write lock so reset cannot be undone by that pending producer. Invalidate each root layout only once for repeated cross-tab notifications. Add regressions for preflight failures, stale transfers, delayed reset notifications, queued credential writes and failed password encryption. All 677 tests and 13 local browser fault-injection scenarios pass; the old-head controls reproduce the reported failures. No live transactions were broadcast. Refs #953.
Summary
Why
walletStore.reset()cleared the public Svelte store, but the long-lived treasury retained its profile array and derived account state. Asynchronous initialization or synchronization could also settle after reset and mutate the cleared store.The shared reset path releases those references and advances the existing wallet-session identifier. Clear-and-init reserves its session before clearing the cache so logout or a newer initialization invalidates the pending continuation. Shared balance/stake refreshes check session/profile ownership before caching and publishing results; treasury sync reuses its generation check around the final asynchronous cache-update steps.
Cross-tab invalidation now lives in the root layout. It uses the existing logout path, then reloads to discard cached login information and preferences. Unlock uses an AbortController to stop late initialization/navigation/migration after leaving the page, and rejects duplicate submissions while an attempt is pending.
This releases JavaScript references to wallet material; it does not claim guaranteed memory zeroization by the browser runtime or cancellation of already-issued browser/network operations. No new dependencies or separate lifecycle framework.
Validation
main, preserving its removal of Lamb.npm run checks— passed: formatting, lint, typecheck, 126 test files / 650 tests.npm run build— passed.One full-suite run hit the untouched receive-page QR snapshot before its image rendered. The isolated test and a complete checks rerun passed; no snapshots or unrelated components were changed.