refactor(walletconnect): migrate to WalletKit and implement multi-chain support #13515

abretonc7s · 2025-02-14T07:01:29Z

Description

This PR completes the migration from WalletConnect SE SDK (a wrapper adding an unnecessary layer over WalletConnect) to WalletKit, improving our existing multi-chain support implementation.

Motivation

WalletConnect SE SDK was an unnecessary abstraction layer over WalletConnect's core functionality
Our app already has initial multi-chain support capabilities that could be leveraged directly
Need for better session management and standardized error handling

Solution

Removed WalletConnect SE SDK dependency in favor of WalletKit
Enhanced existing multi-chain support with:
- Proper chain permission validation
- Standardized CAIP-2 chain ID handling
- Improved session management using namespaced permissions
Standardized error responses across WalletConnect interactions

Key implementation details:

Added getPermittedChains utility for better chain permission management
Updated session management to use proper namespacing
Improved error handling with consistent response formats
Updated test suites to cover new functionality

Related issues

Manual testing steps

Connect to a WalletConnect-enabled dApp
- Verify connection establishes successfully
- Check chain permissions are properly handled
Test multi-chain interactions
- Switch between different networks
- Verify proper permission handling
Test session management
- Change accounts
- Update sessions
- Verify proper error handling
Test disconnection flow
- Verify clean session termination
- Check proper cleanup

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards
I've completed the PR template to the best of my ability
I've included tests with coverage for new functionality
I've documented my code using JSDoc format
I've applied the right labels on the PR

Pre-merge reviewer checklist

I've manually tested the PR
I confirm that this PR addresses all acceptance criteria and includes necessary testing evidence
I've verified multi-chain support works as expected
I've verified error handling and session management
I've checked all dependency updates are compatible

github-actions · 2025-02-14T07:01:40Z

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

1 out of 2 committers have signed the CLA.
✅ @abretonc7s
❌ @chakra-guy

socket-security · 2025-02-14T07:03:00Z

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@reown/[email protected] 🔁 npm/@reown/[email protected]	None	`0`	3.11 MB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`0`	3.49 MB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`0`	135 kB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`0`	26.7 kB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`+3`	3.61 MB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`0`	3.59 MB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`0`	328 kB	gancho_walletconnect
npm/@walletconnect/[email protected] 🔁 npm/@walletconnect/[email protected]	None	`+4`	6.73 MB	gancho_walletconnect

View full report↗︎

chakra-guy

Nicely done

package.json

This reverts commit 88a6ba2.

package.json

codecov-commenter · 2025-02-20T09:42:45Z

Codecov Report

Attention: Patch coverage is 72.72727% with 24 lines in your changes missing coverage. Please review.

Project coverage is 62.74%. Comparing base (b5ea162) to head (41e0566).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
app/core/WalletConnect/wc-utils.ts	80.85%	8 Missing and 1 partial ⚠️
app/core/Permissions/index.ts	11.11%	8 Missing ⚠️
app/core/WalletConnect/WalletConnect2Session.ts	77.77%	3 Missing and 1 partial ⚠️
app/core/WalletConnect/WalletConnectV2.ts	76.92%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13515      +/-   ##
==========================================
+ Coverage   62.53%   62.74%   +0.21%     
==========================================
  Files        2005     2014       +9     
  Lines       44261    44396     +135     
  Branches     6006     6029      +23     
==========================================
+ Hits        27679    27857     +178     
+ Misses      14748    14707      -41     
+ Partials     1834     1832       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

christopherferreira9 · 2025-02-20T10:59:42Z

Screen.Recording.2025-02-20.at.10.59.11.AM.mov

chakra-guy

I have read the CLA Document and I hereby sign the CLA

…bile into wcmigration

This reverts commit bde6638.

christopherferreira9 · 2025-02-20T11:53:10Z

Critical Issue ⚠️: When the connection starts from a non synced state, the wallet is not changing automatically to the proper chain to display a confirmation screen, this means users will be sending transactions for an unwanted chain.

Screen.Recording.2025-02-20.at.11.48.38.AM.mov

Note:
Having a non synced state is normal, the wallet should change for the chain on the dapp in order to start processing a request. Here's a reference video:

Screen.Recording.2025-02-20.at.11.52.04.AM.mov

christopherferreira9 · 2025-02-20T11:56:52Z

SendTx seems to have the wrong origin in the pill

christopherferreira9 · 2025-02-20T12:01:55Z

Trying to sign after switching to a chain that hasn't been approved yet fails:

Screen.Recording.2025-02-20.at.11.58.07.AM.mov

Stacktrace:

{context: 'client'} 'request() -> isValidRequest() failed')

{context: 'client'} 'Missing or invalid. request() chainId: eip155:11155111'

Uncaught (in promise) UnknownRpcError: An unknown RPC error occurred.

sonarqubecloud · 2025-02-20T12:36:37Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
83.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

chakra-guy · 2025-02-20T13:32:46Z

Critical Issue ⚠️: When the connection starts from a non synced state, the wallet is not changing automatically to the proper chain to display a confirmation screen, this means users will be sending transactions for an unwanted chain.

Screen.Recording.2025-02-20.at.11.48.38.AM.mov
Note: Having a non synced state is normal, the wallet should change for the chain on the dapp in order to start processing a request. Here's a reference video:

Screen.Recording.2025-02-20.at.11.52.04.AM.mov

Resolved! Regarding the critical issue where the wallet doesn’t automatically switch chains from a non-synced state:

Root Cause: This was a dapp-side bug, not a mobile app issue. When reconnecting after switching to Arbitrum, the dapp sent personal_sign requests with chainId=eip155:1 (Ethereum) instead of eip155:42161 (Arbitrum). The wallet correctly processes the chain specified in the request, but the dapp failed to update its state (even though from the UI it looks like its good, the actual request is with the wrong chain id).

Verification: Testing confirmed that when the dapp switches to Ethereum, then Arbitrum, and signs, the mobile app properly switches chains.

Let me know if you need further clarification or additional test cases!

christopherferreira9 · 2025-02-20T14:27:50Z

Critical Issue ⚠️: When the connection starts from a non synced state, the wallet is not changing automatically to the proper chain to display a confirmation screen, this means users will be sending transactions for an unwanted chain.
Screen.Recording.2025-02-20.at.11.48.38.AM.mov
Note: Having a non synced state is normal, the wallet should change for the chain on the dapp in order to start processing a request. Here's a reference video:
Screen.Recording.2025-02-20.at.11.52.04.AM.mov

Resolved! Regarding the critical issue where the wallet doesn’t automatically switch chains from a non-synced state:

Root Cause: This was a dapp-side bug, not a mobile app issue. When reconnecting after switching to Arbitrum, the dapp sent personal_sign requests with chainId=eip155:1 (Ethereum) instead of eip155:42161 (Arbitrum). The wallet correctly processes the chain specified in the request, but the dapp failed to update its state (even though from the UI it looks like its good, the actual request is with the wrong chain id).

Verification: Testing confirmed that when the dapp switches to Ethereum, then Arbitrum, and signs, the mobile app properly switches chains.

Let me know if you need further clarification or additional test cases!

The wallet should inform the dapp the current active chain, otherwise the dapp might think they're making a request for one chain when in reality it is requesting for another. In reality, regardless of what the dapp sends, the wallet needs to act as a safeguard and switch to the chain being requested.
@ignaciosantise and @ganchoradkov , can you confirm what is the expected behavior in this situation?

ganchoradkov · 2025-02-21T09:21:07Z

Critical Issue ⚠️: When the connection starts from a non synced state, the wallet is not changing automatically to the proper chain to display a confirmation screen, this means users will be sending transactions for an unwanted chain.
Screen.Recording.2025-02-20.at.11.48.38.AM.mov
Note: Having a non synced state is normal, the wallet should change for the chain on the dapp in order to start processing a request. Here's a reference video:
Screen.Recording.2025-02-20.at.11.52.04.AM.mov

Resolved! Regarding the critical issue where the wallet doesn’t automatically switch chains from a non-synced state:
Root Cause: This was a dapp-side bug, not a mobile app issue. When reconnecting after switching to Arbitrum, the dapp sent personal_sign requests with chainId=eip155:1 (Ethereum) instead of eip155:42161 (Arbitrum). The wallet correctly processes the chain specified in the request, but the dapp failed to update its state (even though from the UI it looks like its good, the actual request is with the wrong chain id).
Verification: Testing confirmed that when the dapp switches to Ethereum, then Arbitrum, and signs, the mobile app properly switches chains.
Let me know if you need further clarification or additional test cases!

The wallet should inform the dapp the current active chain, otherwise the dapp might think they're making a request for one chain when in reality it is requesting for another. In reality, regardless of what the dapp sends, the wallet needs to act as a safeguard and switch to the chain being requested. @ignaciosantise and @ganchoradkov , can you confirm what is the expected behavior in this situation?

The recommended approach is for the wallet to accept requests in any of the approved chains in the session. Users can switch the chain on the dapp multiple times before sending a request and the wallet will not get notified at all of these local (dapp side) chain switches because

the chains are already approved by the user - asking them again to switch on the wallet is redundant
usually the wallet (on mobile device) is not active at the same time as the dapp

abretonc7s added 2 commits February 14, 2025 10:46

wip

60f64e0

Merge remote-tracking branch 'origin/main' into wcmigration

b62cc6c

metamaskbot added the team-sdk SDK team label Feb 14, 2025

abretonc7s added 7 commits February 14, 2025 16:43

wip

91e9fca

wip

db38979

wip

c34519e

wip

4bdfdf3

wip

b9a627c

unit tests

4ce6c94

test

873bc8b

abretonc7s changed the title ~~Wcmigration~~ feat: Migrate from @walletconnect/se-sdk to @reown/walletkit Feb 14, 2025

abretonc7s added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Feb 14, 2025

abretonc7s self-assigned this Feb 14, 2025

Merge remote-tracking branch 'origin/main' into wcmigration

eb82eb2

abretonc7s marked this pull request as ready for review February 14, 2025 09:50

abretonc7s requested review from a team as code owners February 14, 2025 09:50

abretonc7s changed the title ~~feat: Migrate from @walletconnect/se-sdk to @reown/walletkit~~ refactor(walletconnect): migrate to WalletKit and implement multi-chain support Feb 14, 2025

chakra-guy previously approved these changes Feb 14, 2025

View reviewed changes

wip

8b4fb9f

abretonc7s dismissed chakra-guy’s stale review via 8b4fb9f February 14, 2025 13:04

abretonc7s added 4 commits February 14, 2025 21:16

cleanup

71a34da

wip

01c94ca

cleanup

1aaf1b2

cleanup

952a7cc

ignaciosantise reviewed Feb 14, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

ignaciosantise reviewed Feb 14, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

chakra-guy added 2 commits February 19, 2025 15:30

Revert "pr comment"

3ae30be

This reverts commit 88a6ba2.

added back required deps

038d787

chakra-guy reviewed Feb 19, 2025

View reviewed changes

package.json Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into wcmigration

b3f0e23

chakra-guy self-assigned this Feb 19, 2025

ganchoradkov reviewed Feb 20, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

ganchoradkov reviewed Feb 20, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

pr comments

41e0566

chakra-guy and others added 3 commits February 20, 2025 10:49

added more tests

b0b869a

Merge branch 'main' into wcmigration

12a6bd4

Merge branch 'main' into wcmigration

fab4c90

fix ui

bde6638

christopherferreira9 added QA in Progress QA has started on the feature. WalletConnect WalletConnect related issue or bug and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 20, 2025

Merge branch 'main' into wcmigration

06aa5e4

chakra-guy reviewed Feb 20, 2025

View reviewed changes

chakra-guy added 2 commits February 20, 2025 12:27

Merge branch 'wcmigration' of https://github.com/MetaMask/metamask-mo…

2c81fb4

…bile into wcmigration

Revert "fix ui"

4ac085e

This reverts commit bde6638.

chakra-guy added 2 commits February 21, 2025 10:33

fix connection mismatch bug

8a8bf94

Merge branch 'main' into wcmigration

8d16f70

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(walletconnect): migrate to WalletKit and implement multi-chain support #13515

refactor(walletconnect): migrate to WalletKit and implement multi-chain support #13515

abretonc7s commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading

socket-security bot commented Feb 14, 2025 •

edited

Loading

chakra-guy left a comment

codecov-commenter commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025 •

edited

Loading

chakra-guy left a comment

christopherferreira9 commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

sonarqubecloud bot commented Feb 20, 2025

chakra-guy commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

ganchoradkov commented Feb 21, 2025

refactor(walletconnect): migrate to WalletKit and implement multi-chain support #13515

Are you sure you want to change the base?

refactor(walletconnect): migrate to WalletKit and implement multi-chain support #13515

Conversation

abretonc7s commented Feb 14, 2025 • edited Loading

Description

Motivation

Solution

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Feb 14, 2025 • edited Loading

socket-security bot commented Feb 14, 2025 • edited Loading

chakra-guy left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 20, 2025

Codecov Report

christopherferreira9 commented Feb 20, 2025 • edited Loading

chakra-guy left a comment

Choose a reason for hiding this comment

christopherferreira9 commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

sonarqubecloud bot commented Feb 20, 2025

Quality Gate passed

chakra-guy commented Feb 20, 2025

christopherferreira9 commented Feb 20, 2025

ganchoradkov commented Feb 21, 2025

abretonc7s commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading

socket-security bot commented Feb 14, 2025 •

edited

Loading

christopherferreira9 commented Feb 20, 2025 •

edited

Loading