Fix/external wallet login hardening by Ryjen1 · Pull Request #78 · GoodDollar/GoodWallet

Ryjen1 · 2026-11-05T13:42:46Z

Summary

Completes and hardens the external-wallet login flow (PR #72), closing all four scope areas defined in #76:

Scope 1 — Account & Session Isolation: Isolates activity history and token balances per account with session-token guards; validates dApp-supplied sender address with case-insensitive EIP-55 comparison; ensures dApp sessions disconnect on all logout paths; prunes incompatible restored sessions before signer usage.
Scope 2 — Network Switching Coordination: Serializes chain switching, transaction submission, typed-data signing, and message signing through a shared mutex (chainOpLock); unblocks on failure/rejection; separates external disconnect from account change.
Scope 3 — Mempool Tracking & Accurate Outcomes: Tracks external wallet submissions through replaceableTransaction; captures startBlock before submit; rethrows on cancellation and replacement so canceled claims never report false success; preserves Divvi referral tracking on mined and repriced transactions.
Scope 4 — Connection Feedback & Compatibility: Rejects unsupported required chains with UNSUPPORTED_CHAINS error code; advertises only executable methods in approved namespaces; prunes incompatible restored sessions on startup; provides distinct account change and disconnect feedback.

Closes #76
Base branch: feat-connect-wallet-reown

Detailed Changes by Scope

Scope 1 — Keep each account's permissions and data separate

Key activity history cache by active account and purge on account change/logout (transactionHistoryStore).
Guard activity history and token balance async responses against late arrival across account switches (amountsStore, transactionHistoryStore).
Use case-insensitive address comparison (isSameAddress) in WalletWrapper so EIP-55 casing variants from dApps are accepted while mismatched senders are rejected.
Call disconnectDappSessions() on all logout paths (social, private key, external disconnect, account change) in storage.ts.
Prune incompatible restored WalletConnect sessions in initializeWalletConnect before signer usage (walletConnectStore).

Scope 2 — Avoid competing network switches

Serialize ensureChain, submitTransaction, signTypedData, signMessage, and getReownEip1193Provider through chainOpLock in reown.ts.
Release mutex in finally blocks so rejected or failed switches do not leave the interface blocked.
Split watchReownAccount into onAccountChanged and onDisconnected callbacks for distinct UI messaging.

Scope 3 — Finish transaction tracking

Enable mempool replacement tracking via replaceableTransaction(hash, startBlock) in utils.ts.
Query getBlockNumber prior to calling submitTransaction so replacement window is captured.
Branch on err.reason: use replacement receipt and proceed with Divvi tracking for "repriced"; rethrow for "cancelled" and "replaced" to prevent false success.
Retry transient RPC getTransaction failures up to 40 attempts at 1.5s intervals.

Scope 4 — Connection feedback and compatibility checks

Reject session proposals for unsupported required namespaces with UNSUPPORTED_CHAINS error.
Advertise only supported methods in session approval namespaces.
Prune incompatible restored sessions on initialization.
Provide distinct messages for account changes vs. explicit disconnections.

Verification

yarn install --immutable ✅
yarn biome check ✅ (363 files, 0 errors)
yarn tsc --noEmit ✅ (0 type errors)
yarn build ✅ (Turbopack static build clean, 11/11 pages)
yarn test ✅ (19 files, 151 tests passed)
yarn test:e2e ✅ (10/10 Playwright tests passed)
Interactive QA: Live-tested on Brave/MetaMask against testnet contracts covering external login, account switching auto-logout, account isolation, hard reload session restore, and logout.

Conscious Deferrals

N1-R3: disconnectDappSessions fire-and-forget race on rapid re-login (pre-existing pattern, not introduced by this PR).
N2-R3: No timeout on chainOpLock for indefinitely pending wallet popups (requires product decision on mid-operation abort UX).
N3-R2: LiFi swap sequence locking in lifiStore (manages own retries, out of scope for wallet-level queue).

…dresses - Key activity history cache by active account and purge on account change/logout - Guard activity history and token balances against late async responses - Preserve dApp-supplied from address in WalletWrapper and validate against connected account

… from disconnect - Serialize ensureChain and transaction/typed-data signing via wallet-operation queue - Release operation lock on failed or rejected switch so subsequent actions are unblocked - Separate watchReownAccount into onAccountChanged and onDisconnected callbacks - Ensure all logout paths disconnect active dApp sessions and clear activity history

…wallets - Capture submission start block and enable ethers replacement tracking - Resolve replacement receipts for speedup and cancel without throwing unhandled errors - Preserve Divvi referral tracking on replacement transaction hash - Avoid duplicate transaction submissions on transient RPC getTransaction failures

…le restored sessions - Reject session proposals for unsupported required namespaces with UNSUPPORTED_CHAINS - Disconnect restored sessions incompatible with the active signer before signer is usable - Prevent restored sessions from silently rebinding across accounts or unsupported namespaces

…Wrapper - Use isSameAddress helper to avoid rejecting dApp transactions with EIP-55 casing differences - Add regression test verifying case-insensitive sender acceptance

- Acquire chainOpLock in signMessage to prevent interleaving with switch-and-submit - Add regression test verifying signMessage waits for in-flight transaction submission

- Query getBlockNumber prior to submitTransaction in submitWithWallet - Pass startBlock into getSubmittedTransaction and assert call ordering in test

- Restore import { config } from "@/config" in reown.ts and walletConnectStore.ts matching base branch - Update reown.test.ts mock target to match @/config

… accounts

… shim

…ializeWalletConnect

…esolving as success Ethers TRANSACTION_REPLACED distinguishes three replacement reasons: - repriced: same calldata/to/value at higher gas; replacement carries the original intent so the receipt is used and Divvi tracking proceeds. - cancelled: zero-value self-transfer with empty calldata. - replaced: different calldata/to/value (different intent). Previously all replacement reasons resolved as success with the replacement receipt. This caused cancelled transactions (e.g. user cancels a claim in their wallet popup) to report success in ClaimView, fire false success analytics, and trigger Divvi referral tracking with a cancel tx hash. Branch on err.reason: rethrow for 'cancelled' and 'replaced' so caller error paths surface the failure correctly and Divvi tracking is not reached. Keep current behavior for 'repriced' speed-ups.

Copilot

🟡 Changes recommended

Unresolved critical issues release the chain lock while signing is pending and allow reverted replacement transactions to be reported as successful.

9 open issues

What changed in this PR

This PR builds on #72’s external-wallet login flow, targeting #76’s account isolation, network coordination, transaction tracking, and connection compatibility requirements.

Changes:

Adds account-scoped history and balance guards, sender validation, and dApp session cleanup.
Introduces shared wallet-operation locking and separate account-change/disconnect callbacks.
Adds replacement-aware transaction tracking, unsupported-chain rejection, and regression tests.

File	Description
`vitest.config.ts`	Prioritizes specific configuration aliases.
`src/stores/transactionHistoryStore.ts`	Resets history by account and guards stale responses.
`src/stores/transactionHistoryStore.test.ts`	Tests history isolation and delayed responses.
`src/sections/WalletConnect/utils/WalletWrapper.ts`	Validates dApp-supplied sender addresses.
`src/sections/WalletConnect/utils/WalletWrapper.test.ts`	Tests sender matching and address casing.
`src/sections/WalletConnect/store/walletConnectStore.ts`	Checks restored sessions against the active signer.
`src/sections/WalletConnect/store/walletConnectStore.test.ts`	Tests session compatibility and pruning.
`src/sections/WalletConnect/requests/RequestHandler.ts`	Rejects unsupported-chain proposals explicitly.
`src/sections/WalletConnect/requests/RequestHandler.test.ts`	Tests unsupported-chain rejection.
`src/login/reown.ts`	Adds operation locking and distinct account callbacks.
`src/login/reown.test.ts`	Tests locking, failure recovery, and account callbacks.
`src/login/context/SessionContext/storage.ts`	Clears history and disconnects dApp sessions on logout.
`src/hooks/useTokenBalances/stores/amountsStore.ts`	Guards against stale balance responses.
`src/hooks/useTokenBalances/stores/amountsStore.test.ts`	Tests delayed balance responses and clearing.
`src/ethers-utils/utils.ts`	Tracks replacements and distinguishes cancellation outcomes.
`src/ethers-utils/utils.test.ts`	Tests replacements, delayed visibility, and start-block capture.

_{Review effort: Balanced}

…ation - Reject reverted speed-up replacements instead of treating as success (H1) - Await signTypedData inside chainOpLock to prevent premature lock release (H2) - Retry transient RPC failures in getSubmittedTransaction poll loop (M3) - Return replacement transaction and hash from sendTxWaitForMining (M4) - Scope balance refresh invalidation token per chain in amountsStore (M5) - Provide disconnected reconnect reason and notification for external disconnects (M6) - Reject WalletConnect proposals requiring unsupported methods (M7) - Validate restored WalletConnect sessions against signer capabilities (M8) - Match network CAIP-2 chain when selecting Bitcoin and Solana addresses (M9)

Copilot

🟡 Changes recommended

Unresolved session-isolation flaws can expose or overwrite another account’s balances and allow WalletConnect requests to use the wrong account’s signer.

7 open issues

2 previously missed issues

In code that hasn't changed since last review

Validate restored session chains and signers

src/sections/WalletConnect/store/walletConnectStore.ts:46

A restored session can pass with the current address on an unsupported chain, or with a Solana/Bitcoin account whose network-specific signer is missing. Such sessions remain connected even though their requests cannot execute. Validate declared chains and full chain-qualified accounts against the supported namespace, preserving case-insensitive comparison for EVM addresses.

Reject stale-session propagating transactions

src/stores/transactionHistoryStore.ts:29

Advancing this token guards fetched history but not delayed addPropagatingTx calls: a claim started by A can mine after login as B, and ClaimView then inserts A's transaction into B's history. Capture the originating session token when starting send, claim, and swap operations, and reject propagating inserts when that token no longer matches.

7 resolved since last review

_{Review effort: Balanced}

- Guard refreshAmounts against stale callbacks adopting inactive session addresses (B7) - Add session token guard to addPropagatingTx to prevent cross-account history leak (B8) - Defer exposing activeSigner in WalletConnect until incompatible sessions are pruned (B9) - Guard inFlightRequests counter decrement in amountsStore against stale requests (B10) - Normalize chain-scoped namespace keys in WalletConnect session compatibility (B11) - Clear cached amounts on account change in amountsStore (B12) - Validate EVM account chain against supported chains in restored sessions (B13)

Copilot

🟡 Changes recommended

Unresolved account-isolation flaws can expose a new signer to incompatible WalletConnect sessions and insert a prior account’s transactions into the current account’s history.

4 open issues

6 previously missed issues

In code that hasn't changed since last review

Capture the account token before awaiting the SDK import

src/sections/Swap/components/RouteSlider/RouteSlider.tsx:424

The account token is captured after await import("@lifi/sdk"), so a logout/login during that wait tags account A's retained swap operation with account B's token. Its completed transaction can then pass the guard and enter B's history. Capture the token at the start of executeSelectedRoute, before any await, and stop before executing the route if the token has changed.

Require session tokens for all asynchronous transaction inserts

src/stores/transactionHistoryStore.ts:51

The undefined-token bypass leaves the non-EVM send paths unprotected: BTCDialogDetails, SVMDialogDetails, and XRPDialogDetails still call addPropagatingTx(tx) after awaiting broadcast. If account A logs out and B logs in while that broadcast is pending, A's completion inserts its transaction into B's newly cleared history. Require a captured session token for every asynchronous insertion and update those callers as well.

Reconcile propagating sends with mined transaction outcomes

src/sections/Send/hooks/evmTransaction.ts:295

The Send path inserts the original hash but never reconciles it with the mining outcome: a speed-up leaves a stale entry alongside the fetched replacement, while cancellation leaves a transfer that never executed. Replace the propagating entry with the mined receipt's hash on success and remove or mark it failed on cancellation or replacement with different intent. Guard these updates with the captured account token.

Collect chains from scoped namespace keys

src/sections/WalletConnect/requests/RequestHandler.ts:104

A valid chain-scoped proposal such as requiredNamespaces: { "eip155:1": { methods: ["personal_sign"], events: [] } } receives UNSUPPORTED_CHAINS because chain collection ignores the namespace key. With no explicit chains array, supportedChains is empty despite Ethereum being supported. Extract chains from scoped keys in both required and optional namespaces before deciding to reject.

Stop advertising unsupported raw transaction signing

src/sections/WalletConnect/utils/supportedNamespaces.ts:25

For social/private-key signers, this still advertises eth_sendRawTransaction as supported. approveEIP155Request has no handler for that method and falls through to INVALID_METHOD, so an approved request cannot execute; the new restored-session check also incorrectly considers that permission compatible. Exclude it for all signers until a raw-broadcast handler exists.

Initialize the account token when the session starts

src/stores/transactionHistoryStore.ts:80

On a fresh login, activeAccount remains undefined until the first history refresh. Home initially mounts only the Crypto tab, so a claim or swap can capture a token and insert a propagating transaction before Activity is opened. This first refresh then clears that same account's transaction and increments its token, also rejecting operations still in flight. Initialize the account/token when the session starts so opening Activity does not invalidate valid same-session operations.

4 resolved since last review

_{Review effort: Balanced}

…ty reconciliation - Guard WalletKit initialization and signer publication with shared init promise and generation counter - Validate session topic compatibility before opening request dialog and before signing - Extract chains from scoped namespace keys in session proposals - Exclude unsupported eth_sendRawTransaction from EIP155 supported methods for all signers - Initialize activity account token on session start to avoid invalidating same-session operations - Capture account token before awaiting SDK import in swap route execution - Require session tokens for all asynchronous transaction insertions across all chain dialogs - Reconcile propagating transactions with mined outcomes (hash replacement on speed-up, removal on failure) - Ignore balance refresh callbacks from ended or non-matching sessions

Copilot

🟡 Changes recommended

A critical initialization race can remove valid WalletConnect sessions, while transaction tracking can lose Activity entries for uncertain outcomes or duplicate speed-up transfers.

5 open issues

2 resolved since last review

_{Review effort: Balanced}

… and deduplicate speed-ups - Check generation before pruning restored sessions during WalletKit init to prevent superseded signers from pruning active connections - Keep pending activity entries when transaction outcome is uncertain; remove only on confirmed cancellation, different-intent replacement, or on-chain revert - Deduplicate speed-up replacements in activity history by dropping original propagating entry when replacement hash is already present

…duplication - Return immediately from initializeWalletConnect when superseded before pruning or publishing (F1) - Add dedicated test suite for handleTrxMinedOutcome covering unknown tracking errors vs cancellations (F2) - Match duplicate replacement check per list excluding the hash being renamed in replacePropagatingTx (F3)

Copilot

🟡 Changes recommended

Logout during WalletKit initialization can leave restored sessions connected, allowing a later login to retain permissions that logout should have revoked.

3 open issues

3 resolved since last review

_{Review effort: Balanced}

…itialization - Record pending logout cleanup when disconnectAllWalletConnectSessions is called while WalletKit is initializing - Execute disconnect on all active sessions when initialization resolves with pending logout cleanup - Add regression test verifying restored sessions are disconnected and signer remains unpublished upon logout during init

Copilot

🔵 Needs a closer look

Human assessment is needed for critical session-lifecycle races that can allow signing after connection revocation or execute swaps using another account’s signer.

2 open issues

1 resolved since last review

Complete session cleanup when logout races initialization

_{Review effort: Balanced}

- Add regression test verifying executeSelectedRoute captures initialAccountToken before dynamic LiFi import and aborts if token changes in-flight - Assert executeRoute and addPropagatingTx are not called on account change - Assert happy path executes route and records propagating tx when token is unchanged

Ryjen1 added 14 commits November 4, 2026 16:21

fix(walletconnect): use case-insensitive address comparison in Wallet…

4d319b6

…Wrapper - Use isSameAddress helper to avoid rejecting dApp transactions with EIP-55 casing differences - Add regression test verifying case-insensitive sender acceptance

fix(reown): serialize signMessage through wallet-operation queue

411a07d

- Acquire chainOpLock in signMessage to prevent interleaving with switch-and-submit - Add regression test verifying signMessage waits for in-flight transaction submission

fix(ethers): capture startBlock before wallet submission

45a1fb5

- Query getBlockNumber prior to submitTransaction in submitWithWallet - Pass startBlock into getSubmittedTransaction and assert call ordering in test

fix(config): restore canonical @/config path alias imports

6a0e61f

- Restore import { config } from "@/config" in reown.ts and walletConnectStore.ts matching base branch - Update reown.test.ts mock target to match @/config

test(config): mock @/config alias in reown and walletConnectStore tests

7facb25

test(balances): verify amountsStore drops late async responses across…

090b471

… accounts

fix(history): reset activeAccount inside clearActivityHistory

6c74108

build(vitest): prioritize config aliases over src wildcard and remove…

477e4b1

… shim

test(walletconnect): add integration test for session pruning in init…

b33c44f

…ializeWalletConnect

Ryjen1 mentioned this pull request Nov 5, 2026

[GoodBounty] Complete external-wallet login in GoodWallet #76

Open

Ryjen1 changed the base branch from main to feat-connect-wallet-reown November 5, 2026 13:54

Ryjen1 force-pushed the fix/external-wallet-login-hardening branch from e652cf4 to d2e4f70 Compare November 5, 2026 14:07

Ryjen1 requested a balanced review from Copilot November 6, 2026 07:13

Copilot started reviewing on behalf of Ryjen1 November 6, 2026 07:13

View session