Skip to content

fix(walletconnect): attach eip155 accounts when proposal mixes eip155 with adapter namespaces - #33672

Merged
adonesky1 merged 3 commits into
mainfrom
fix/wpn-1704-wc-eip155-accounts-empty
Jul 24, 2026
Merged

fix(walletconnect): attach eip155 accounts when proposal mixes eip155 with adapter namespaces#33672
adonesky1 merged 3 commits into
mainfrom
fix/wpn-1704-wc-eip155-accounts-empty

Conversation

@adonesky1

@adonesky1 adonesky1 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Description

When a WalletConnect session proposal requests both eip155 and an adapter namespace (e.g. tron), two bugs combined so that the settled session's eip155 namespace came back with the right chains and methods but an empty accounts array, while Tron got a real account (reported by Reown, reproducible on lab.reown.com multichain-all with any connect button since they all propose all chains):

  1. The permission request never contained the requested eip155 chains. In WC2Manager.onSessionProposal, the CAIP-25 caveat value passed to requestPermissions starts as the default (wallet:eip155 only) and is enriched only by registered non-EVM adapters (currently Tron). Since nothing seeded the proposal's eip155:* chains, the approval UI's "specifically requested chains" branch pre-selected only Tron, leaving EVM networks unchecked.

  2. Manually checking EVM networks didn't attach accounts. handleNetworksSelected in MultichainAccountConnect updated selectedChainIds but never recomputed selectedCaipAccountIds. Since the initial account IDs were derived from the Tron-only default chain list, the confirm path (setChainIdsInCaip25CaveatValue + setNonSCACaipAccountIdsInCaip25CaveatValue) persisted the newly added eip155 scopes with accounts: []setNonSCACaipAccountIdsInCaip25CaveatValue only fills scopes whose namespace appears in the provided account IDs. The post-approval updatePermittedChains(channelId, ['eip155:<current>']) call then added the current EVM chain (which is why the namespace looked "correctly shaped"), but synced accounts from the caveat's existing (Tron-only) accounts, so eip155 stayed empty.

Fixes:

  • Added enrichCaveatValueForEip155 (wc-utils.ts), called from onSessionProposal right after enrichCaveatValueByAdapters. It reuses the existing enrichCaveatValueForNamespace util to seed the proposal's requested eip155 chains (filtered to configured networks, falling back to the wallet's current EVM chain) into the caveat's optionalScopes, exactly mirroring what the Tron adapter does for its namespace. The approval UI now pre-selects the requested EVM networks alongside Tron.
  • handleNetworksSelected now recomputes selectedCaipAccountIds from the currently selected account groups against the new chain selection (same logic handleAccountGroupsSelected already used), so any namespace added via the edit-networks screen gets accounts attached.

Changelog

CHANGELOG entry: Fixed WalletConnect connections requesting both EVM and non-EVM chains (e.g. eip155 + tron) connecting without any EVM account attached

Related issues

Fixes: WPN-1704

Manual testing steps

Feature: WalletConnect multichain session proposal (eip155 + tron)

Scenario: dapp proposes eip155 and tron together
Given a build with the tron feature flag enabled
And a dapp (e.g. https://lab.reown.com/appkit/?name=multichain-all) configured for eip155 and tron

When the user connects via WalletConnect and scans the QR code
Then the permission screen pre-selects the requested EVM networks and Tron
And approving settles a session where the eip155 namespace contains accounts (e.g. eip155:1:0x...)

Scenario: user manually adds EVM networks on the permission screen
Given a WalletConnect connection request where only a non-EVM network is pre-selected

When the user opens "Edit networks", checks an EVM network, taps Update, and approves
Then the granted permission contains accounts for the newly added EVM scopes
And the settled session's eip155 namespace contains those accounts

Screenshots/Recordings

Before

Session settles with eip155: { chains: [...], methods: [...], accounts: [] } while tron gets a real account. Permission screen pre-selects only Tron.

Screen.Recording.2026-07-22.at.4.27.16.PM.mov

After

Permission screen pre-selects requested EVM networks and Tron; settled session attaches accounts to both namespaces.

Screen.Recording.2026-07-22.at.4.31.56.PM.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Changes how CAIP-25 permissions are built for WalletConnect and multichain connect approval; incorrect logic could grant wrong or empty accounts, but scope is a targeted bugfix with regression tests.

Overview
Fixes WPN-1704, where WalletConnect sessions that request eip155 together with an adapter namespace (e.g. Tron) could settle with correct EVM chains but no eip155 accounts.

WalletConnect proposal handling now calls new enrichCaveatValueForEip155 after adapter enrichment so requested eip155 chains (limited to configured networks, with fallback to the active EVM chain) are added to the CAIP-25 caveat before requestPermissions. The connect UI can pre-select EVM networks alongside Tron instead of only the adapter scope.

Connect approval UI no longer keeps selectedCaipAccountIds in separate state. It is derived from the selected account groups and selectedChainIds, so adding EVM networks in the network editor updates granted accounts for those scopes on approve.

Regression tests cover mixed eip155+Tron proposals, enrichCaveatValueForEip155, and the network-editor account sync path.

Reviewed by Cursor Bugbot for commit 962e40b. Bugbot is set up for automated code reviews on this repo. Configure here.

… with adapter namespaces

A WalletConnect session proposal combining eip155 with an adapter
namespace (e.g. Tron) produced a permission request whose only chain
scopes were the adapter's, so the approval UI pre-selected only that
namespace, and manually checking EVM networks still settled the session
with an empty eip155 accounts array.

Two fixes:
- Seed the proposal's requested eip155 chains into the CAIP-25 caveat
  value in onSessionProposal (mirroring adapter enrichCaveatValue), so
  the approval UI pre-selects the requested EVM networks.
- Recompute selectedCaipAccountIds in handleNetworksSelected so scopes
  added via the edit-networks screen get accounts attached instead of
  being persisted with empty account lists.

Fixes WPN-1704
@adonesky1
adonesky1 requested review from a team as code owners July 22, 2026 19:34
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamask-ci metamask-ci Bot added the team-wallet-integrations Wallet Integrations team label Jul 22, 2026
@metamask-ci

metamask-ci Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Pre-merge author checklist has only 5 of the required 8 items. Every checklist row must be present and consciously checked — do not delete rows.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@github-actions github-actions Bot added size-M risk:medium AI analysis: medium risk labels Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/Views/MultichainAccounts/MultichainAccountConnect/MultichainAccountConnect.test.tsx 0/145 0/214 0/301
app/core/WalletConnect/WalletConnectV2.test.ts 0/145 0/214 0/301

AI-detected flaky patterns

app/components/Views/MultichainAccounts/MultichainAccountConnect/MultichainAccountConnect.test.tsx

  • J10 — jest.spyOn without restoreAllMocks in afterEach (medium)
    • jest.spyOn(console, 'error') in the malformed CAIP account IDs test (and similar spies on Engine.context.PermissionController, useAnalytics) lacks restoreAllMocks in any afterEach. beforeEach only uses clearAllMocks. This matches J10 and can cause cross-test pollution. History shows 0 failures.
    • Suggested fix in app/components/Views/MultichainAccounts/MultichainAccountConnect/MultichainAccountConnect.test.tsx:280:
      -const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
      -    expect(() => {
      -      renderWithProvider(
      -        <MultichainAccountConnect
      -          route={{
      -            params: {
      +afterEach(() => {
      +    jest.clearAllMocks();
      +    jest.restoreAllMocks();
      +    jest.mocked(useAnalytics).mockReturnValue(
      +      createMockUseAnalyticsHook({
      +        trackEvent: mockTrackEvent,
      +      })
      +    );
      +  });
  • J1 — async callback without act() wrapping a state update (critical)
    • fireEvent.press on CONNECT_BUTTON triggers async permission flow (acceptPermissionsRequest, updateCaveat, grantPermissionsIncremental) but is not wrapped in act(). Subsequent waitFor does not replace act() for React state updates. Exact match to J1 symptoms and example (onPress handlers). No historical flaky signal but high risk for intermittent failures.
    • Suggested fix in app/components/Views/MultichainAccounts/MultichainAccountConnect/MultichainAccountConnect.test.tsx:165:
      -    const confirmButton = getByTestId(CommonSelectorsIDs.CONNECT_BUTTON);
      -    fireEvent.press(confirmButton);
      -    await waitFor(() => {
      -      expect(mockAcceptPermissionsRequestLocal).toHaveBeenCalledWith(
      -        expect.objectContaining({
      +    const confirmButton = getByTestId(CommonSelectorsIDs.CONNECT_BUTTON);
      +    await act(async () => {
      +      fireEvent.press(confirmButton);
      +    });
      +    await waitFor(() => {
      +      expect(mockAcceptPermissionsRequestLocal).toHaveBeenCalledWith(
      +        expect.objectContaining({

app/core/WalletConnect/WalletConnectV2.test.ts

  • J10 — jest.spyOn without restoreAllMocks in afterEach (medium)
    • Multiple jest.spyOn calls (e.g. on console.warn, web3Wallet methods, StorageWrapper) throughout the test file lack jest.restoreAllMocks() or equivalent mockRestore in afterEach (only clearAllMocks is used in the top-level afterEach). Spies persist and can alter behavior of later tests, matching the J10 pattern exactly. Historical hint not used (0 failures).
    • Suggested fix in app/core/WalletConnect/WalletConnectV2.test.ts:92:
      -afterEach(() => {
      -    jest.clearAllMocks();
      -    (WC2Manager as any).instance = undefined;
      -    (WC2Manager as any)._initialized = false;
      -  });
      +afterEach(() => {
      +    jest.clearAllMocks();
      +    jest.restoreAllMocks();
      +    (WC2Manager as any).instance = undefined;
      +    (WC2Manager as any)._initialized = false;
      +  });
  • J10 — jest.spyOn without restoreAllMocks in afterEach (medium)
    • This specific console.warn spy (and similar ones in session proposal, removePendings, and init tests) has no mockRestore() call. Matches J10 leakage pattern; clearAllMocks alone does not restore original console behavior. No historical flakiness signal.
    • Suggested fix in app/core/WalletConnect/WalletConnectV2.test.ts:75:
      -const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
      -      const result = await TestWC2Manager.init({});
      -      expect(result).toBeUndefined();
      -      expect(consoleWarnSpy).toHaveBeenCalledWith(
      -        'WC2::init missing navigation --- SKIP INIT',
      -      );
      +const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
      +      const result = await TestWC2Manager.init({});
      +      expect(result).toBeUndefined();
      +      expect(consoleWarnSpy).toHaveBeenCalledWith(
      +        'WC2::init missing navigation --- SKIP INIT',
      +      );
      +      consoleWarnSpy.mockRestore();

This check is informational only and does not block merging.

const selectedAccountGroups = Array.from(
new Set([
...supportedAccountGroups,
...connectedAccountGroupWithRequested,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unfortunately this component is difficult to follow now. My thinking is that handleAccountGroupsSelected and handleNetworksSelected would look pretty much the same. I see some differences:

  1. connectedAccountGroupWithRequested is included in handleNetworksSelected but not in handleAccountGroupsSelected
  2. setSelectedAccountGroupIds is included in handleAccountGroupsSelected but not in handleNetworksSelected

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

great callout — addressed in 126eef62. selectedCaipAccountIds is now derived from the selected groups + chains, so both handlers are simpler and there’s a single source of truth.

Comment thread app/core/WalletConnect/WalletConnectV2.ts
… and chains

Per review feedback: selectedCaipAccountIds was duplicated component state
that every writer recomputed from (account groups x chain ids); the WPN-1704
bug existed because one writer (handleNetworksSelected) forgot. Derive it
with useMemo instead so the granted accounts always follow the current
group/chain selection, and collapse both selection handlers back to plain
state setters.
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeMultiChainAPI, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeConfirmations
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 88%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR fixes a regression (WPN-1704) in WalletConnect V2 multi-namespace session proposals and the MultichainAccountConnect permission UI:

  1. WalletConnectV2.ts + wc-utils.ts: Adds enrichCaveatValueForEip155 to seed EVM (eip155) chains into the CAIP-25 caveat value during WalletConnect session proposals that mix eip155 with adapter namespaces (e.g., Tron). Without this fix, the approval UI would only pre-select the adapter namespace, leaving EVM networks unchecked.

  2. MultichainAccountConnect.tsx: Refactors selectedCaipAccountIds from duplicated state to a derived memo computed from selectedAccountGroupIds and selectedChainIds. This ensures that when users add networks via the network editor, the granted accounts are automatically recomputed for newly added namespaces. Previously, the state was not updated on network selection, causing eip155 scopes to be persisted with empty accounts arrays.

Tag selection rationale:

  • SmokeMultiChainAPI: Directly tests CAIP-25 multi-chain session API (wallet_createSession, wallet_getSession, wallet_revokeSession, wallet_sessionChanged) — the core permission system being modified. Required.
  • SmokeNetworkAbstractions: Tests the chain permission system for dApps, granting/revoking chain access, and the permission UI (MultichainAccountConnect is the approval UI). Required per SmokeMultiChainAPI dependency.
  • SmokeNetworkExpansion: Tests multi-chain provider architecture, dApp connect/disconnect flows, and multi-chain simultaneous connections — directly exercises the WalletConnect session proposal path being changed. Required per SmokeMultiChainAPI dependency.
  • SmokeConfirmations: Required per SmokeNetworkExpansion dependency (Solana flows hit confirmations), and the dApp connection approval flow involves confirmation-like UI.

The changes are unit-tested but affect core dApp connection permission flows that could impact multiple user-facing scenarios. The risk is medium because the changes are targeted bug fixes with clear scope, but they touch the critical CAIP-25 permission path used by all dApp connections.

Performance Test Selection:
The changes are focused on correctness fixes for WalletConnect session proposal handling and the MultichainAccountConnect permission UI. There are no changes to rendering performance, asset loading, onboarding flows, login flows, or other performance-sensitive paths. The refactoring of selectedCaipAccountIds from state to a memo is a correctness fix (not a performance optimization) and does not affect measured performance scenarios. No performance test tags are warranted.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

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

Lgtm

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

platform changes lgtm

@adonesky1
adonesky1 added this pull request to the merge queue Jul 24, 2026
Merged via the queue into main with commit 6874ad1 Jul 24, 2026
178 checks passed
@adonesky1
adonesky1 deleted the fix/wpn-1704-wc-eip155-accounts-empty branch July 24, 2026 18:11
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 24, 2026
@metamask-ci metamask-ci Bot added the release-8.6.0 Issue or pull request that will be included in release 8.6.0 label Jul 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

release-8.6.0 Issue or pull request that will be included in release 8.6.0 risk:medium AI analysis: medium risk size-M team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants