feat(walletconnect): add multichain routing and namespace approval for first non-EVM chain (Tron)#29428
feat(walletconnect): add multichain routing and namespace approval for first non-EVM chain (Tron)#29428Olivier-BB wants to merge 35 commits into
Conversation
|
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. |
…compatibility Normalize WalletConnect Tron requests to canonical Snap methods and sanitize params for JSON-RPC validity. Expand Tron session chain/account compatibility to support both decimal and hex CAIP references so dapps and wallet stay interoperable. Made-with: Cursor
Ensure tron signTransaction requests execute with metamask origin and adapt signature-only Snap results into a full signed transaction payload expected by WalletConnect dapps. Made-with: Cursor
…hain permission errors
3e5c974 to
c22d169
Compare
…ct chainChanged fallback he
…nd handle missing caveats safely
| }, | ||
| existingNamespaces: mergedNamespaces, | ||
| }); | ||
| Object.assign(mergedNamespaces, adapterSlices); |
There was a problem hiding this comment.
it's not clear what this does on the surface level. Is this effectively filtering out namespaces without accounts?
| optionalNamespaces: {}, | ||
| }); | ||
| const missingRequiredAdapterNamespaces = requiredAdapterNamespaces.filter( | ||
| (ns) => !mergedNamespaces[ns]?.chains?.length, |
There was a problem hiding this comment.
I'm having trouble understanding what is going on here
requiredAdapterNamespaces returns adapter generated namespaces from this.session.requiredNamespaces that have a matching adapter, which effectively is requiredNamespaces with an adapter and accounts.
mergedNamespaces contains this.session.namespace, which contains this.requiredNamespace
missingRequiredAdapterNamespaces returns this.session.requiredNamespaces with matching adapter and non-empty accounts but are missing entirely or have no chains defined in mergedNamespaces.
Is this right? Can this be explained in simpler terms?
| // (a session previously approved eip155 should keep eip155 on update). | ||
| Object.keys(currentNamespaces).forEach((key) => | ||
| allowedNamespaceKeys.add(key), | ||
| ); |
There was a problem hiding this comment.
currentNamespaces is this.session.namespaces. Is this not the same set of keys as this.session.requiredNamespaces and this.session.optionalNamespaces?
| const redirectNamespace = | ||
| requestNamespace === KnownCaipNamespace.Wallet | ||
| ? KnownCaipNamespace.Eip155 | ||
| : requestNamespace; |
There was a problem hiding this comment.
i don't think WC supports targeting the wallet or wallet:eip155 scopes. requestNamespace may be safe as is
| } | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
if the session exists, then a permission should exist for it. Are you seeing this not be the case?
| const permittedChainsFromSession = | ||
| this.session.topic !== this.channelId | ||
| ? await getPermittedChainsSafe(this.session.topic) | ||
| : []; |
There was a problem hiding this comment.
won't this always be empty array because permissions are not key'ed by this.session.topic, but rather channelId / this.session.pairingTopic?
| (chainId) => activeSessionChains.includes(chainId), | ||
| ); | ||
| const isPermittedRequestChain = | ||
| isPermittedByPermissionController || isPermittedByActiveSession; |
There was a problem hiding this comment.
the result from the PermissionController should be the source of truth ultimately. Both the activeSession and the permission are kept in sync - ish, so at minimum only one of these needs to be checked, not both. Unless you're seeing some reason otherwise?
| }): Promise<unknown> => | ||
| Engine.controllerMessenger.call('MultichainRoutingService:handleRequest', { | ||
| connectedAddresses, | ||
| origin: 'metamask', |
There was a problem hiding this comment.
this is an unsafe internal origin. The value used here should probably be the channelId
| } catch (err) { | ||
| console.warn(`WC2::init can't update session ${sessionKey}`); | ||
| DevLogger.log(`WC2::init can't update session ${sessionKey}`); | ||
| await this.cleanupBrokenRestoredSession(session, err); |
| const referencedAdapterNamespaces = proposalReferencedAdapterNamespaces( | ||
| proposal.params, | ||
| ); | ||
| const isMultichainOrigin = referencedAdapterNamespaces.length > 0; |
There was a problem hiding this comment.
@adonesky1 . does it really matter what value this flag is now?
| const allProposalNamespaces = { | ||
| ...optionalNamespaces, | ||
| ...requiredNamespaces, | ||
| } as Record< | ||
| string, | ||
| { chains?: string[]; methods?: string[]; events?: string[] } | ||
| >; |
| namespaces.wallet = { | ||
| chains: walletChains, | ||
| methods: namespaces.eip155.methods ?? [], | ||
| events: namespaces.eip155.events ?? [], | ||
| accounts: eip155Accounts.map((account) => { | ||
| const [, , address] = account.split(':'); | ||
| return `wallet:eip155:${address}` as `${string}:${string}:${string}`; | ||
| }), |
There was a problem hiding this comment.
is this wallet namespace used later? I feel like i've already seen special handling of wallet:eip155 that would imply that namespaces['wallet'] is not used, but maybe I'm wrong
| const allowedNamespaceKeys = new Set<string>([ | ||
| ...Object.keys(proposal.params.requiredNamespaces ?? {}), | ||
| ...Object.keys(proposal.params.optionalNamespaces ?? {}), | ||
| ]); |
| if (!session) { | ||
| const activeSession = this.getSession(sessionTopic); | ||
| if (activeSession) { | ||
| session = new WalletConnect2Session({ | ||
| web3Wallet: this.web3Wallet, | ||
| channelId: activeSession.pairingTopic, | ||
| navigation: this.navigation, | ||
| deeplink: true, | ||
| session: activeSession, | ||
| }); | ||
| this.sessions[sessionTopic] = session; | ||
| } | ||
| } |
| export const getChainChangedEmissionForWalletConnect = ({ | ||
| namespaces, | ||
| fallbackEvmDecimal, | ||
| fallbackEvmHex, |
There was a problem hiding this comment.
it feels odd for the caller to be responsible to provide the evm chain id formatted in both decimal and hex
| return String(raw); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
confused on how the WC changes would warrant this change here
| }); | ||
| }); | ||
|
|
||
| it('passes EVM chains to permissions hook for mixed wallet:eip155 + tron requests', () => { |
There was a problem hiding this comment.
right?
| it('passes EVM chains to permissions hook for mixed wallet:eip155 + tron requests', () => { | |
| it('passes EVM chains and Tron to permissions hook for mixed wallet:eip155 + tron requests', () => { |
| ...Object.keys(requestedCaip25CaveatValue.optionalScopes ?? {}), | ||
| ]; | ||
|
|
||
| // CAIP-25 may encode delegated namespace requests (for example wallet:eip155). |
There was a problem hiding this comment.
I'm not sure what encode delegated namespace requests means?
| // Expand only namespaces that were requested without explicit chains | ||
| // (e.g. wallet:eip155 delegated requests in mixed namespace proposals). | ||
| // Intentionally restrictive: if explicit eip155 chains are requested, treat | ||
| // that list as authoritative and do not expand to all eip155 networks. This | ||
| // prevents approving more chains than requested. Delegated namespace | ||
| // expansion still applies when no explicit chain ids are provided. | ||
| // TODO: check if this restrictive logic has already been applied in the past or if this modifies previous behavior. |
There was a problem hiding this comment.
I think this makes sense for this pass. Just noting that we are probably going to be loosening chain permissions quite a bit in the near future: i.e. you ask for 1 EVM chain you get all EVM chains. But TBD
|
|
||
| /** | ||
| * Returns a default CAIP-25 caveat value. | ||
| * Each chain appends its optional scope below (feature-flagged). |
There was a problem hiding this comment.
[nit]
| * Each chain appends its optional scope below (feature-flagged). | |
| * Each ecosystem (i.e. EVM, Tron) appends its optional scope below (feature-flagged). |
| ///: BEGIN:ONLY_INCLUDE_IF(tron) | ||
| optionalScopes[TrxScope.Mainnet] = { accounts: [] }; | ||
| ///: END:ONLY_INCLUDE_IF |
There was a problem hiding this comment.
not sure I'm following why this would be added as a default namespace for permissions? I'm struggling to figure out how to test this properly and I'm not totally remembering or understanding how this helper function is used but its name implies that this addition will mean Tron network permissions will be granted by default? TBH I'm not sure how that would be the case... but either way I'm not sure I understand the purpose of this addition.
| * @returns An array containing permitted chains for the specified host. | ||
| * @returns An array containing permitted CAIP chain IDs for the specified host. | ||
| */ | ||
| export const getPermittedChains = async ( |
There was a problem hiding this comment.
| export const getPermittedChains = async ( | |
| export const getPermittedCaipChainIds = async ( |
Yes the typing gives this context, but the sibling getPermittedAccounts is currently only returning EVM accounts... so this could get confusing
|
|
||
| export const normalizeCaipChainIdInboundForWalletConnectTron = ( | ||
| caipChainId: string, | ||
| ): string => { |
There was a problem hiding this comment.
[nit] shouldn't the return type also be caipChainId?
| const TRON_PREFIX = 'tron:'; | ||
| const DEFAULT_TRON_CHAIN_ID = 'tron:0x2b6653dc'; | ||
|
|
||
| export const normalizeCaipChainIdInboundForWalletConnectTron = ( |
There was a problem hiding this comment.
We should add a comment explaining why this normalization is required? I asked Claude to explain why this was necessary because it's nonobvious when just looking at it. WalletConnect tends to encode the Tron chainIds with hex values for the reference i.e. tron:0x2b6653dc while we use decimals for the reference tron:728126428
| return caipChainId; | ||
| }; | ||
|
|
||
| export const getCompatibleTronCaipChainIdsForWalletConnect = ( |
There was a problem hiding this comment.
Lets please add some JSdocs to explain why this utility function is necessary?
There was a problem hiding this comment.
Looking at where this is used, I'm not understanding why we need this encoded in both hex and decimal formats side by side ever?
| const listTronEoaAddresses = (): string[] => | ||
| Engine.context.AccountsController.listAccounts() | ||
| .filter((account: { type: string }) => account.type === TrxAccountType.Eoa) | ||
| .map((account: { address: string }) => account.address); |
There was a problem hiding this comment.
Just so I'm clear do we support Tron SCAs?
| * Shape of a single namespace slice in WalletConnect's approved | ||
| * namespaces map. Mirrors `SessionTypes.Namespace` but kept loose. | ||
| */ | ||
| export interface TronNamespaceSlice { |
There was a problem hiding this comment.
hmmm this seems tied to a mixed version of CAIP-25. Can you point me to the docs where this type is described?
| * matches `SessionTypes.Namespace.accounts` which is a plain string[]. | ||
| */ | ||
| accounts: string[]; | ||
| } |
There was a problem hiding this comment.
Do we have two different versions of this same interface? https://github.com/MetaMask/metamask-mobile/pull/29428/changes#diff-09c1410947d91a7bf8ff2bd7a39a10ad19c1b4d40083abfdc2c8b86d08580f73R47
Also TronNamespaceSlice is awfully similar...?
| const originalTransaction = | ||
| typeof transactionContainer === 'object' && | ||
| transactionContainer !== null && | ||
| !Array.isArray(transactionContainer) && | ||
| typeof (transactionContainer as Record<string, unknown>).transaction === | ||
| 'object' && | ||
| (transactionContainer as Record<string, unknown>).transaction !== null | ||
| ? ((transactionContainer as Record<string, unknown>) | ||
| .transaction as Record<string, unknown>) | ||
| : typeof transactionContainer === 'object' && | ||
| transactionContainer !== null && | ||
| !Array.isArray(transactionContainer) | ||
| ? (transactionContainer as Record<string, unknown>) | ||
| : undefined; |
There was a problem hiding this comment.
ummm this can't be reasoned about in any meaningful way
| .filter((account: { type: string }) => account.type === TrxAccountType.Eoa) | ||
| .map((account: { address: string }) => account.address); | ||
|
|
||
| export const buildTronScopedPermissionsNamespace = ({ |
There was a problem hiding this comment.
When a function gets this long I find it can help to add some inline comments explain what is going on at each step
Description
This PR refactors WalletConnect multichain handling by introducing a per-chain
ChainAdapterregistry and moving non-EVM behavior behind that adapter layer (with Tron as the first implementation). The goal is to reduce coupling in WalletConnect session code and make additional non-EVM chains easier to add.It also improves namespace approval and routing behavior for mixed EVM + Tron sessions by approving only requested namespaces/chains, preserving EVM chain selection, and normalizing Tron request/response mapping for compatibility with dapp expectations.
Changelog
CHANGELOG entry: Improved WalletConnect multichain compatibility by refining namespace approval and request routing for mixed EVM and Tron sessions.
Related issues
Fixes:
Manual testing steps