Skip to content

Harden wallet lifecycle: remove implicit auto-init, enforce explicit identity boundary - #77

Merged
jonathunne merged 12 commits into
tetherto:mainfrom
nulllpc:npc/remove-auto-init-flag
Aug 18, 2026
Merged

Harden wallet lifecycle: remove implicit auto-init, enforce explicit identity boundary#77
jonathunne merged 12 commits into
tetherto:mainfrom
nulllpc:npc/remove-auto-init-flag

Conversation

@nulllpc

@nulllpc nulllpc commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR removes the reactive, implicit wallet auto-initialization model and replaces it with an explicit, race-free lifecycle centered entirely in useWalletManager

@nulllpc nulllpc self-assigned this Jul 16, 2026
Comment thread src/hooks/internal/useWalletOrchestrator.ts
@nulllpc nulllpc changed the title Remove deprecated unused flags Harden wallet lifecycle: remove implicit auto-init, enforce explicit identity boundary Aug 5, 2026
@nulllpc

nulllpc commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

It started as cleanup of a deprecated prop and turned up a real identity-boundary bug along the way. So I did a deeper refactoring and the result was pretty satisfying. Here's a detailed report of what I found:

Motivation

WdkAppProvider defaulted enableAutoInitialization to true, which drove a reactive effect in useWalletOrchestrator that auto-created/unlocked wallets whenever currentUserId and the worklet were ready - bypassing whatever biometric/auth gate the consuming app built. Auditing that path surfaced a confirmed, reproducible bug: a persisted activeWalletId could get silently unlocked after the app moved to a different user, and switching users could report READY without the new wallet ever being initialized.

Approach

  • Removed the implicit lifecycle entirely. Deleted enableAutoInitialization, currentUserId, retry, and clearSensitiveDataOnBackground/useAppLifecycle — all either dead, unused, or actively fighting the "consumer owns auth" model. useWalletOrchestrator is now a pure state-derivation hook with no side effects.
  • Closed the identity-boundary bug at the root, not with a patch:
    • activeWalletId is no longer persisted to disk — identity is caller-owned, so nothing should silently outlive a session/user change. Existing installs get it force-reset on every rehydrate too, not just future writes.
    • unlock(walletId) now requires an explicit id; no implicit fallback to whatever the store happens to hold.
    • READY status now requires the loaded wallet's identifier to actually match activeWalletId, not just "something is initialized."
    • Removed setActiveWalletId, an unguarded setter with no real callers that was the most direct way to desync identity from what's actually loaded.
  • Extended the same discipline everywhere it was missing. createWallet and restoreWallet had the exact same "can silently overwrite an active session" gap as the original unlock bug — both now reject unless the caller explicitly lock()s first. lock() itself now shares the same operation mutex as every other lifecycle call, closing a race where it could be silently undone by an in-flight unlock/createWallet/etc. completing afterward. Added switchWallet(walletId) as the one deliberate convenience (atomic lock() + unlock()) for the genuinely frequent case of switching between existing accounts.
  • Removed dead weight found along the way: an unexported, unused WalletSwitchingService implementing a second, less-safe wallet-switching path with none of the above guards.
  • Rebalanced test coverage — dropped ~20 tests that were coverage padding on trivial pass-throughs, replaced with tests that actually pin the identity-boundary invariant, mutex atomicity, and manual lock→unlock composition.

Net result: all wallet identity mutation lives in one place (useWalletManager), behind one real mutex, with no path left that can move activeWalletId without doing the corresponding work.

Breaking changes

  • WdkAppProvider: removed enableAutoInitialization, currentUserId, clearSensitiveDataOnBackground props.
  • useWdkApp/WdkAppContextValue: removed retry.
  • useWalletManager: removed setActiveWalletId; added switchWallet; unlock(walletId) now requires an explicit argument; lock() now returns Promise<void> (callers should await it before chaining another lifecycle call); createWallet/restoreWallet now throw if a wallet is already active - callers must lock() first.

@nulllpc

nulllpc commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Breaking Changes

Each item below only applies if the removed/changed API was actually used. If a given prop or method was never called, no action is needed for that item.

  • enableAutoInitialization, currentUserId, and clearSensitiveDataOnBackground have been removed from WdkAppProvider. Call unlock(walletId) / createWallet(walletId) explicitly instead of relying on auto-init; for background locking, listen to AppState directly and call lock().

  • retry() has been removed from useWdkApp. Re-call whichever lifecycle method failed (unlock/createWallet/restoreWallet) instead.

  • state.walletId is now optional when status === 'LOCKED' (previously always a string). The undefined case needs to be handled.

  • setActiveWalletId has been removed. unlock/createWallet/restoreWallet/switchWallet manage identity correctly as part of doing real work and should be used instead.

  • unlock() now requires an explicit walletId argument (previously optional).

  • lock() now returns Promise<void> (previously void) and shares a mutex with unlock/createWallet/restoreWallet/switchWallet. It must be awaited before calling another lifecycle method - await switchWallet(id) handles switching wallets in one call.

  • createWallet/restoreWallet now throw if a wallet is already active, instead of silently overwriting the session. lock() must be awaited first.

  • activeWalletId is no longer persisted across app restarts. Identity must be re-established on every launch (e.g. calling unlock(userId) from the app's own session state).

Also Fixed

  • NO_WALLET no longer misreports right after lock() when a wallet is actually known to exist - it now only fires when confirmed empty.

@NirmalPatidar

Copy link
Copy Markdown

Please rebase onto main before merge. This branch is 8 commits behind main (missing PR #78 swidge protocol support, the beta.14 release/version bump, and a dependabot postcss bump). As-is, this diff shows package.json going from 1.0.0-beta.15 → 1.0.0-beta.13, @tetherto/pear-wrk-wdk downgrading from beta.10 → beta.8, and 'swidge' being silently dropped from useProtocol.ts's protocolType union. None of that is intentional — it's just staleness — but merging as-is would revert those changes.

Comment thread src/hooks/useWalletManager.ts Outdated
Comment thread README.md
nulllpc added 11 commits August 13, 2026 22:20
- unlock(walletId) now requires an explicit walletId; no more implicit
  fallback to the store's activeWalletId
- Add switchWallet(walletId) as an atomic lock() + unlock() convenience,
  guaranteeing the previous wallet's state is cleared before the next
  one loads
- Remove setActiveWalletId: an unguarded setter with no internal callers
- Stop persisting activeWalletId - identity is caller-owned, so no
  wallet id should silently survive a session/user change
- READY status now requires walletLoadingState.identifier to match
  activeWalletId, not just isWorkletInitialized, so a mismatched
  identity reports LOCKED instead of a false READY
- Refactor tests for `useWalletManager` to remove dependency on
  `useWdkApp` and its associated context wrapper, simplifying the test
  setup
- Remove redundant and filler tests that do not add much value
- `lock` now shares the same operation mutex as
  unlock/switchWallet/createWallet/restoreWallet instead of running
  unprotected.
- `createWallet` and restoreWallet now throw if a wallet is already
  ready, matching the guard already enforced by unlock.
- Guard `createWallet` and `restoreWallet` with mutex
orchestrator test

- useWalletManager: drop dead/padding tests (deleted setActiveWalletId,
  thin pass-through delegation/error tests); add coverage for
  switchWallet atomicity, lock/unlock/createWallet/restoreWallet mutex
  races, and the "must lock before switching identity" guards
- useWalletOrchestrator: add identity-mismatch READY-vs-LOCKED test; fix
  an existing test that silently broke when READY started requiring a
  matching walletLoadingState identifier
- Set activeWalletId to null in walletStore during rehydration
- Adjust tests accordingly
- Unexported, unused by any hook/component - a leftover parallel
  wallet-switching path from an earlier architecture
- Missing the reset-before-switch discipline enforced everywhere else in
  useWalletManager
- Strips its two tests out of raceConditions.test.ts (they only used it
  as a vehicle to exercise the generic mutex, not because the service
  itself needed dedicated coverage)
- Fixes a stale JSDoc example in operationMutex.ts that referenced it
- Improve state orchestration to distinguish between "no wallets exist" and "wallets exist but are locked"
- Update documentation and type definitions to clarify that LOCKED status may contain an optional walletId
- README: fix stale useWdkApp/useWalletManager snippets (isReady,
  loadWallet, hasWallet() never existed as shown), add a Wallet
  Lifecycle section covering caller-owned identity, the
  lock-before-switching rule, and status meanings
- docs/quick-start.md: rewrite the example to use state.status and
  explicit unlock(userId)/createWallet(userId)
- docs/architecture.md: remove the deleted WalletSwitchingService and
  "consolidated effect" description, replace with the actual
  mutex-serialized useWalletManager model; drop dead
  WALLET_STATE_MACHINE.md link; fix useWallet -> useWalletManager
- docs/troubleshooting.md: remove reference to the removed retry()
  method
…ometric doc claims

- unlock() now throws "A wallet is already active. Call lock() before
  unlocking a different wallet." when a *different* wallet is ready,
  matching createWallet/restoreWallet's existing behavior. It still
  silently no-ops when the *same* wallet is already ready.
- Update tests: split the old test into one asserting the throw and one
  asserting the same-wallet no-op is preserved.
- Fix JSDoc on unlock/createWallet/restoreWallet/switchWallet/getMnemonic/getEncryptionKey
  that claimed the library triggers or requires biometric authentication
  - it doesn't (WalletSetupService always reads secure storage with
  requireBiometrics: false). All now say the app is responsible for any
  biometric/security check before calling.
@nulllpc
nulllpc force-pushed the npc/remove-auto-init-flag branch from 72b3793 to 4a77c04 Compare August 14, 2026 10:58
NirmalPatidar
NirmalPatidar previously approved these changes Aug 17, 2026

@NirmalPatidar NirmalPatidar 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.

LGTM

Comment thread src/hooks/useWalletManager.ts
state

- unlock() now requires activeWalletId to match walletLoadingState
  before treating a wallet as already ready, instead of trusting
  walletLoadingState alone.
- createTemporaryWallet() now refuses to run while a different wallet is
  ready, and updates walletLoadingState through loading -> ready/error
  so it can't diverge from activeWalletId.
- Add tests for the divergence, the new guard, and the ready state being
  set on temp wallet creation.

@jonathunne jonathunne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work @nulllpc , and thanks @NirmalPatidar for the review 👌

@jonathunne
jonathunne merged commit 983ed00 into tetherto:main Aug 18, 2026
4 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.

3 participants