Skip to content

fix(walletconnect): restore per-session channel id as the Snap request origin - #34284

Open
adonesky1 wants to merge 4 commits into
mainfrom
fix/wc-snap-origin-channel-id
Open

fix(walletconnect): restore per-session channel id as the Snap request origin#34284
adonesky1 wants to merge 4 commits into
mainfrom
fix/wc-snap-origin-channel-id

Conversation

@adonesky1

@adonesky1 adonesky1 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

refactor(walletconnect): use stable origin for Tron snap requests replaced the per-session WalletConnect channelId with the shared constant 'wallet-connect' as the origin passed to MultichainRoutingService:handleRequest. That collapses every WalletConnect dapp into a single Snap-visible principal: MultichainRoutingService forwards origin verbatim into SnapKeyring.submitRequest / SnapController:handleRequest, so Snaps can no longer distinguish one WC session from another. Any Snap-side logic keyed on the request origin (display, SIWS domain checks, per-origin state) would treat all WalletConnect dapps as equivalent. This is the "origin confusion" flagged in the follow-up Slack discussion, where the alignment reached was:

  • origin stays unspoofable, always: for remote transports (WC/SDK) that is the wallet-generated connection/channel ID, mirroring how EVM permissions are keyed.
  • The dapp's self-reported URL travels separately in a distinctly-named, display-only field (originMetadata) with explicit unverified framing, and never feeds Blockaid/SIWS. This half requires Snaps-platform support first (WPC-1194 / WPC-1195) and is deliberately not included here.

This PR restores the invariant: the per-session channelId is threaded again from WalletConnect2Session.handleAdapterRequest → adapter → createSnapCallerMultichainRoutingService. It also:

  • Documents origin accurately on the adapter contract (the pre-existing doc called it a "URL-like dapp origin", which is what invited the regression): it is the unspoofable session identifier, never the self-reported URL, never a shared transport constant.
  • Removes the multichain-layer WALLET_CONNECT_ORIGIN = 'wallet-connect' constant, which also collided confusingly with the legacy WALLET_CONNECT_ORIGIN = 'wc::' in app/util/walletconnect.
  • Adds router.test.ts pinning the invariant at the exact point the regression happened: the router must forward the caller-supplied per-session origin and keep distinct sessions distinct.

Known, accepted tradeoff (short-term, per the Slack thread): Snaps may render the channel UUID in confirmation UI instead of a friendly label. The proper fix for display is the upcoming originMetadata field with unverified framing, not a shared origin.

Video Demo

Screen.Recording.2026-08-04.at.4.54.26.PM.mov

Snap-side compatibility notes

Verified against the Snap sources before making this change:

  • Tron (this PR's scope): safe. The Tron Snap pinned by mobile (@metamask/tron-wallet-snap 1.33.1) never validates the origin: formatOrigin maps metamask → "MetaMask", extracts the hostname from valid URLs, and falls back to the raw string otherwise. Channel IDs as origin is how Tron-over-WC originally shipped. Note the premise of refactor(walletconnect): use stable origin for Tron snap requests ("MUST stay in sync with the Snaps' known-origin label map") never shipped for Tron: that map only exists in the still-open fix: handle non-url origins, so on current production the shared constant renders as the literal string "wallet-connect" in confirmations for every WC dapp.
  • Solana (future adapters): blocker to resolve before reusing this pattern. The Solana Snap's parseOrigin throws on non-URL origins and is called from the confirmation views, so a channel-ID origin would break rendering of every signing confirmation. The open fix: handle non-url origins does not resolve this for channel IDs: it only whitelists metamask/wallet-connect (and forwards wallet-connect to Blockaid as the scan origin), i.e. it was designed around the shared-constant approach this PR reverts. Before Solana-over-WalletConnect lands, that Snap needs to accept arbitrary non-URL origins (hide from display, keep out of Blockaid) per the WPC-1194 / WPC-1195 direction.

Changelog

CHANGELOG entry: null

Related issues

Fixes: regression introduced in refactor(walletconnect): use stable origin for Tron snap requests

Related: WPC-1194, WPC-1195, WAPI-1576

Manual testing steps

Feature: Tron WalletConnect Snap origin

Scenario: user signs a Tron request through WalletConnect
  Given a dapp is connected to MetaMask Mobile through WalletConnect using the Tron namespace
  And the dapp submits a supported Tron signing request

  When MetaMask Mobile routes the request to the Tron Snap
  Then the request is sent through MultichainRoutingService with the session's channel id as origin
  And a second dapp connected over a different WalletConnect session presents a different origin to the Snap

Screenshots/Recordings

N/A. Routing-layer change covered by unit tests; no wallet UI changes.

Before

N/A

After

N/A

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
Security-sensitive origin semantics for Snap routing are corrected, but behavior changes from the prior shared-origin approach and may affect Snap UI or per-origin logic until separate unverified metadata lands.

Overview
Reverts a regression where every WalletConnect dapp was collapsed into one Snap-visible principal via a shared 'wallet-connect' origin on MultichainRoutingService:handleRequest.

WalletConnect2Session now passes this.channelId into non-EVM adapter handling as origin, and that value is threaded through adapters (e.g. Tron handleRequest) and createSnapCaller into MultichainRoutingService, so Snaps see one distinct principal per WC session again. The multichain WALLET_CONNECT_ORIGIN constant is removed; AdapterRequestContext / router JSDoc describe origin as the unspoofable session id, not the dapp metadata URL.

New router.test.ts (and updated adapter/helper tests) assert the caller-supplied per-session origin is forwarded and that different channel ids stay distinct.

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

@adonesky1
adonesky1 requested a review from a team as a code owner August 4, 2026 16:03
@github-actions

github-actions Bot commented Aug 4, 2026

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.

@github-actions github-actions Bot added the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label Aug 4, 2026
@adonesky1 adonesky1 added the team-mobile-platform Mobile Platform team label Aug 4, 2026
@metamask-ci metamask-ci Bot added the team-wallet-integrations Wallet Integrations team label Aug 4, 2026
@metamask-ci

metamask-ci Bot commented Aug 4, 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 Aug 4, 2026
@adonesky1
adonesky1 enabled auto-merge August 4, 2026 20:57

@Cal-L Cal-L 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

@adonesky1 adonesky1 removed the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeSnaps, SmokeMMConnect, SmokeMultiChainAPI, SmokeNetworkExpansion
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: high
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR modifies the WalletConnect multichain request routing pipeline with a security fix: replacing the shared constant WALLET_CONNECT_ORIGIN = 'wallet-connect' with a per-session channel ID as the Snap-visible origin. This ensures each WalletConnect session is a distinct principal when communicating with Snaps.

Key impacted areas:

  1. SmokeSnaps: Snaps now receive the per-session channel ID as their origin instead of the shared 'wallet-connect' constant. This directly affects how Snaps identify and authorize WalletConnect callers. Any Snap that processes WalletConnect requests (e.g., Tron Snap) will see a different origin format.

  2. SmokeMMConnect: WalletConnect connection flows are directly affected since WalletConnect2Session.ts now passes this.channelId as the origin through the entire request pipeline. The MMConnect tests exercise WalletConnect session establishment and request routing.

  3. SmokeMultiChainAPI: The multichain session handling (handleAdapterRequest, handleMultichainRequestByAdapter) now threads the origin through the entire call chain. CAIP-25 multi-chain sessions use this code path.

  4. SmokeNetworkExpansion: Non-EVM chain support (Tron via Snaps) is directly affected by the tron/adapter.ts changes which now forward the per-session origin to callTronSnap.

The changes are in app/core/WalletConnect/ which is a critical path. While the unit tests are updated to verify the new behavior, E2E tests are needed to confirm the end-to-end flow works correctly with the new origin-passing mechanism. The risk is high because this is a security-sensitive change affecting how Snaps identify WalletConnect callers.

Performance Test Selection:
The changes are purely in the WalletConnect multichain request routing logic - specifically adding an 'origin' parameter to the request pipeline. This is a security fix with no performance-sensitive code paths (no rendering, no asset loading, no onboarding, no swap flows). The changes don't affect any performance-measured scenarios.

View GitHub Actions results

@sonarqubecloud

sonarqubecloud Bot commented Aug 5, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:medium AI analysis: medium risk size-M team-mobile-platform Mobile Platform team team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants