-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(walletconnect): add multichain routing and namespace approval for first non-EVM chain (Tron) #29428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(walletconnect): add multichain routing and namespace approval for first non-EVM chain (Tron) #29428
Changes from 16 commits
0775bb3
bdd7ee3
dc7091d
bf68506
ac40253
4eca361
a6620f3
ebd1289
1c5c033
2a63d5a
f411910
e9e2b6f
567fd8d
c22d169
c79d2f2
1857154
4ac9e55
c96d410
4453b5e
b70ebb5
67d3c75
3d109b0
6e8a850
a33c251
a4244bb
57ff4b2
8eaab03
3a42f5a
7bcd18d
40fcb0b
a14098e
c98e3b9
a45c3c1
955a50f
72d4ea9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -356,6 +356,11 @@ jest.mock( | |
| }), | ||
| ); | ||
|
|
||
| const { useAccountGroupsForPermissions: mockUseAccountGroupsForPermissions } = | ||
| jest.requireMock( | ||
| '../../../hooks/useAccountGroupsForPermissions/useAccountGroupsForPermissions', | ||
| ); | ||
|
|
||
| // Mock useWalletInfo hook | ||
| jest.mock( | ||
| '../../../../components/Views/MultichainAccounts/WalletDetails/hooks/useWalletInfo', | ||
|
|
@@ -699,6 +704,39 @@ describe('MultichainAccountConnect', () => { | |
| }); | ||
| }); | ||
|
|
||
| it('passes EVM chains to permissions hook for mixed wallet:eip155 + tron requests', () => { | ||
| renderWithProvider( | ||
| <MultichainAccountConnect | ||
| route={{ | ||
| params: { | ||
| hostInfo: { | ||
| metadata: { | ||
| id: 'mockId', | ||
| origin: 'wc-channel-id', | ||
| isEip1193Request: false, | ||
| }, | ||
| permissions: createMockCaip25Permission({ | ||
| 'wallet:eip155': { | ||
| accounts: [], | ||
| }, | ||
| 'tron:728126428': { | ||
| accounts: [], | ||
| }, | ||
| }), | ||
| }, | ||
| permissionRequestId: 'test-mixed-request', | ||
| }, | ||
| }} | ||
| />, | ||
| { state: createMockState() }, | ||
| ); | ||
|
|
||
| const requestedChainIds = | ||
| mockUseAccountGroupsForPermissions.mock.calls.at(-1)?.[2] ?? []; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. guessing a plain ole
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. We used hook-call inspection because the regression is specifically in the derived chain list passed into useAccountGroupsForPermissions. We also added coverage for delegated expansion vs explicit-chain restriction to validate behavior. |
||
| expect(requestedChainIds).toContain('eip155:1'); | ||
| expect(requestedChainIds).toContain('tron:728126428'); | ||
| }); | ||
|
|
||
| describe('Phishing detection', () => { | ||
| describe('dapp scanning is enabled', () => { | ||
| it('does not show phishing modal for safe URLs', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ import { getPhishingTestResultAsync } from '../../../../util/phishingDetection.t | |
| import { | ||
| CaipAccountId, | ||
| CaipChainId, | ||
| CaipNamespace, | ||
| KnownCaipNamespace, | ||
| parseCaipChainId, | ||
| } from '@metamask/utils'; | ||
|
|
@@ -215,6 +216,32 @@ const MultichainAccountConnect = (props: AccountConnectProps) => { | |
| [requestedNamespaces], | ||
| ); | ||
|
|
||
| const requestedNamespacesForNetworkSelection = useMemo(() => { | ||
| const namespaces = new Set(requestedNamespacesWithoutWallet); | ||
| const rawScopeKeys = [ | ||
| ...Object.keys(requestedCaip25CaveatValue.requiredScopes ?? {}), | ||
| ...Object.keys(requestedCaip25CaveatValue.optionalScopes ?? {}), | ||
| ]; | ||
|
|
||
| // CAIP-25 may encode delegated namespace requests (for example wallet:eip155). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what |
||
| // Expand wallet:<namespace> scopes so default network selection still includes | ||
| // those concrete namespaces in multichain WalletConnect flows. | ||
| rawScopeKeys.forEach((scope) => { | ||
| if (!scope.startsWith(`${KnownCaipNamespace.Wallet}:`)) { | ||
| return; | ||
| } | ||
|
|
||
| const delegatedNamespace = scope.slice( | ||
| `${KnownCaipNamespace.Wallet}:`.length, | ||
| ); | ||
|
0xEdouardEth marked this conversation as resolved.
Outdated
|
||
| if (delegatedNamespace) { | ||
| namespaces.add(delegatedNamespace as CaipNamespace); | ||
| } | ||
| }); | ||
|
|
||
| return Array.from(namespaces); | ||
| }, [requestedNamespacesWithoutWallet, requestedCaip25CaveatValue]); | ||
|
|
||
| const networkConfigurations = useSelector( | ||
| selectNetworkConfigurationsByCaipChainId, | ||
| ); | ||
|
|
@@ -322,14 +349,11 @@ const MultichainAccountConnect = (props: AccountConnectProps) => { | |
| return defaultSelectedNetworkList; | ||
| } | ||
|
|
||
| let additionalChains: CaipChainId[] = []; | ||
| if (isEip1193Request) { | ||
| additionalChains = nonTestNetworkCaipChainIds.filter((caipChainId) => | ||
| requestedNamespacesWithoutWallet.includes( | ||
| parseCaipChainId(caipChainId).namespace, | ||
| ), | ||
| ); | ||
| } | ||
| const additionalChains = nonTestNetworkCaipChainIds.filter((caipChainId) => | ||
| requestedNamespacesForNetworkSelection.includes( | ||
| parseCaipChainId(caipChainId).namespace, | ||
| ), | ||
| ); | ||
|
|
||
| const supportedRequestedCaipChainIds = Array.from( | ||
| new Set([ | ||
|
|
@@ -350,12 +374,12 @@ const MultichainAccountConnect = (props: AccountConnectProps) => { | |
| ); | ||
| } | ||
|
|
||
| if (requestedNamespaces.length > 0) { | ||
| if (requestedNamespacesForNetworkSelection.length > 0) { | ||
| return Array.from( | ||
| new Set( | ||
| defaultSelectedNetworkList.filter((caipChainId) => { | ||
| const { namespace } = parseCaipChainId(caipChainId); | ||
| return requestedNamespaces.includes(namespace); | ||
| return requestedNamespacesForNetworkSelection.includes(namespace); | ||
| }), | ||
| ), | ||
| ); | ||
|
|
@@ -368,8 +392,7 @@ const MultichainAccountConnect = (props: AccountConnectProps) => { | |
| requestedCaipChainIds, | ||
| isEip1193Request, | ||
| currentlySelectedNetwork.chainId, | ||
| requestedNamespaces, | ||
| requestedNamespacesWithoutWallet, | ||
| requestedNamespacesForNetworkSelection, | ||
| alreadyConnectedCaipChainIds, | ||
| isSolanaWalletStandardRequest, | ||
| ]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,9 @@ | |||||
| parseCaipAccountId, | ||||||
| parseCaipChainId, | ||||||
| } from '@metamask/utils'; | ||||||
| ///: BEGIN:ONLY_INCLUDE_IF(tron) | ||||||
| import { TrxScope } from '@metamask/keyring-api'; | ||||||
| ///: END:ONLY_INCLUDE_IF | ||||||
| import { InternalAccount } from '@metamask/keyring-internal-api'; | ||||||
| import { | ||||||
| Caip25CaveatType, | ||||||
|
|
@@ -16,7 +19,7 @@ | |||||
| getAllScopesFromCaip25CaveatValue, | ||||||
| getCaipAccountIdsFromCaip25CaveatValue, | ||||||
| getEthAccounts, | ||||||
| getPermittedEthChainIds, | ||||||
|
Check warning on line 22 in app/core/Permissions/index.ts
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove if indeed it's an unused import |
||||||
| isInternalAccountInPermittedAccountIds, | ||||||
| setChainIdsInCaip25CaveatValue, | ||||||
| setNonSCACaipAccountIdsInCaip25CaveatValue, | ||||||
|
|
@@ -297,18 +300,25 @@ | |||||
|
|
||||||
| /** | ||||||
| * Returns a default CAIP-25 caveat value. | ||||||
| * Each chain appends its optional scope below (feature-flagged). | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit]
Suggested change
|
||||||
| * @returns Default {@link Caip25CaveatValue} | ||||||
| */ | ||||||
| export const getDefaultCaip25CaveatValue = (): Caip25CaveatValue => ({ | ||||||
| requiredScopes: {}, | ||||||
| optionalScopes: { | ||||||
| 'wallet:eip155': { | ||||||
| accounts: [], | ||||||
| }, | ||||||
| }, | ||||||
| sessionProperties: {}, | ||||||
| isMultichainOrigin: false, | ||||||
| }); | ||||||
| export const getDefaultCaip25CaveatValue = (): Caip25CaveatValue => { | ||||||
| const optionalScopes: Caip25CaveatValue['optionalScopes'] = { | ||||||
| 'wallet:eip155': { accounts: [] }, | ||||||
| }; | ||||||
|
|
||||||
| ///: BEGIN:ONLY_INCLUDE_IF(tron) | ||||||
| optionalScopes[TrxScope.Mainnet] = { accounts: [] }; | ||||||
| ///: END:ONLY_INCLUDE_IF | ||||||
|
Comment on lines
+311
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| return { | ||||||
| requiredScopes: {}, | ||||||
| optionalScopes, | ||||||
| sessionProperties: {}, | ||||||
| isMultichainOrigin: false, | ||||||
| }; | ||||||
| }; | ||||||
|
|
||||||
| // Returns the CAIP-25 caveat or undefined if it does not exist | ||||||
| export const getCaip25Caveat = (origin: string) => { | ||||||
|
|
@@ -624,10 +634,11 @@ | |||||
| }; | ||||||
|
|
||||||
| /** | ||||||
| * Get permitted chains for the given the host. | ||||||
| * Get permitted chains for the given the host, across all namespaces. | ||||||
| * Returns all non-wallet scopes stored in the CAIP-25 caveat. | ||||||
| * | ||||||
| * @param hostname - Subject to check if permissions exists. Ex: A Dapp is a subject. | ||||||
| * @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 ( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Yes the typing gives this context, but the sibling |
||||||
| hostname: string, | ||||||
|
|
@@ -640,14 +651,16 @@ | |||||
| ); | ||||||
|
|
||||||
| if (caveat) { | ||||||
| const chains = getPermittedEthChainIds(caveat.value).map( | ||||||
| (chainId: string) => | ||||||
| `${KnownCaipNamespace.Eip155.toString()}:${parseInt( | ||||||
| chainId, | ||||||
| )}` as CaipChainId, | ||||||
| return getAllScopesFromCaip25CaveatValue(caveat.value).filter( | ||||||
| (caipChainId: CaipChainId) => { | ||||||
| try { | ||||||
| const { namespace } = parseCaipChainId(caipChainId); | ||||||
| return namespace !== KnownCaipNamespace.Wallet; | ||||||
| } catch { | ||||||
| return false; | ||||||
| } | ||||||
| }, | ||||||
| ); | ||||||
|
|
||||||
| return chains; | ||||||
| } | ||||||
|
|
||||||
| return []; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right?