Skip to content

Commit 4918683

Browse files
jiexiadonesky1
andauthored
refactor: Align add/switch chain logic with Extension (Fix addEthereumChain unneeded switch approval) (#20649)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** * Fixes bug where `wallet_addEthereumChain` calls that result in a prompt to the user to approve the add was also prompting the user to permit the chain right afterwards * Changes `wallet_addEthereumChain` to not switch to a specific rpc endpoint in the scenario it already exists in the wallet. See Changelog below * Refactors add/switchEthereumChain and helper logic to align closer to Extension's as prerequisite work before a larger refactor to bring this shared logic into a core package ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Fixes a bug where the user is unnecessarily prompted to approve access to a network that they have just approved to add to their wallet. ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/jira/software/c/projects/WAPI/boards/2015?selectedIssue=WAPI-721 ## **Manual testing steps** 1. In the In-App Browser, visit chainlist.org 2. Add a rpc for a chain you do not have in your wallet yet 3. Accept the Add Chain approval, you should NOT get a Permit chain approval afterwards 4. Check your permissions and see that the chain is now permitted for chainlist.org as well ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** https://github.com/user-attachments/assets/19016412-872d-4edb-9947-267e9563c254 ### **After** https://github.com/user-attachments/assets/76d050eb-ce7b-4bc3-a037-d99643d9d734 ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Refactors add/switch chain logic to use incremental CAIP‑25 permissions with metadata, updates approval routing/UI, and revises WalletConnect and tests accordingly. > > - **RPC/Core**: > - **Hooks/Metadata**: Change `getRpcMethodMiddlewareHooks` to accept context object; propagate `pageMeta` in incremental permission requests. > - **Switch Logic**: Redesign `switchToNetwork` to use `networkClientId`, `nativeCurrency`, and `rpcUrl`; request CAIP‑25 permissions incrementally (with metadata) instead of extra approvals; set active network by client id; tweak analytics symbol. > - **wallet_addEthereumChain**: Validate/update or add networks; avoid unnecessary RPC endpoint switch when already present; call new `switchToNetwork`; emit metrics; update tests. > - **wallet_switchEthereumChain**: Resolve network config by `chainId`, error if unknown; call new `switchToNetwork` with derived `rpcUrl`/`networkClientId`; remove explicit approval flow; update tests. > - **WalletConnect**: Use new hooks API; clear pending approvals before prompting; pass WC metadata; call new `switchToNetwork`. > - **Approvals UI**: > - **PermissionApproval**: Skip navigation when `requestData.diff.permissionDiffMap[Caip25EndowmentPermissionName]` exists; fallback to `metadata.pageMeta`. > - **SwitchChainApproval**: Handle CAIP‑25 permission diffs; derive `chainId` via `getPermittedEthChainIds` and show `chainName` from store; use `metadata.rpcUrl`; update snapshots/tests. > - **Misc**: > - **useApprovalRequest**: Add fallback to `requestData.metadata.pageMeta`. > - **Tests**: Broad updates across RPC, WalletConnect, and Approvals to reflect new flow and APIs. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 30c3ce9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Alex Donesky <[email protected]>
1 parent 8ecec02 commit 4918683

18 files changed

+511
-485
lines changed

app/components/Approvals/PermissionApproval/PermissionApproval.test.tsx

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,30 @@ describe('PermissionApproval', () => {
210210
expect(navigationMock.navigate).toHaveBeenCalledTimes(0);
211211
});
212212

213+
it(`does not navigate if there is a permission diff for the ${Caip25EndowmentPermissionName} permission`, async () => {
214+
const navigationMock = {
215+
navigate: jest.fn(),
216+
};
217+
218+
mockApprovalRequest({
219+
type: ApprovalTypes.REQUEST_PERMISSIONS,
220+
requestData: {
221+
...HOST_INFO_MOCK,
222+
diff: {
223+
permissionDiffMap: {
224+
[Caip25EndowmentPermissionName]: {},
225+
},
226+
},
227+
},
228+
// TODO: Replace "any" with type
229+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
230+
} as any);
231+
232+
render(<PermissionApproval navigation={navigationMock} />);
233+
234+
expect(navigationMock.navigate).toHaveBeenCalledTimes(0);
235+
});
236+
213237
it(`does not navigate if no ${Caip25EndowmentPermissionName} permission`, async () => {
214238
const navigationMock = {
215239
navigate: jest.fn(),

app/components/Approvals/PermissionApproval/PermissionApproval.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ const PermissionApproval = (props: PermissionApprovalProps) => {
4747
metadata: { id },
4848
} = requestData;
4949

50+
if (requestData?.diff?.permissionDiffMap?.[Caip25EndowmentPermissionName]) {
51+
// Use the SwitchChainApproval component to handle this request instead
52+
return;
53+
}
54+
5055
if (isProcessing.current) return;
5156

5257
isProcessing.current = true;

app/components/Approvals/SwitchChainApproval/SwitchChainApproval.test.tsx

Lines changed: 47 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,23 @@ import React from 'react';
22
import useApprovalRequest from '../../Views/confirmations/hooks/useApprovalRequest';
33
import { shallow } from 'enzyme';
44
import { ApprovalTypes } from '../../../core/RPCMethods/RPCMethodMiddleware';
5-
import { ApprovalRequest } from '@metamask/approval-controller';
65
import SwitchChainApproval from './SwitchChainApproval';
76
import { networkSwitched } from '../../../actions/onboardNetwork';
87
// eslint-disable-next-line import/no-namespace
98
import * as networks from '../../../util/networks';
9+
import {
10+
Caip25CaveatType,
11+
Caip25EndowmentPermissionName,
12+
} from '@metamask/chain-agnostic-permission';
13+
14+
jest.mock('../../../selectors/networkController', () => ({
15+
...jest.requireActual('../../../selectors/networkController'),
16+
selectEvmNetworkConfigurationsByChainId: () => ({
17+
'0x1': {
18+
name: 'Ethereum Mainnet',
19+
},
20+
}),
21+
}));
1022

1123
jest.mock('../../hooks/useNetworksByNamespace/useNetworksByNamespace', () => ({
1224
useNetworksByNamespace: () => ({
@@ -54,11 +66,7 @@ jest.mock('react-redux', () => ({
5466
useSelector: jest.fn((selector) => selector()),
5567
}));
5668

57-
const URL_MOCK = 'test.com';
58-
59-
// TODO: Replace "any" with type
60-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
61-
const mockApprovalRequest = (approvalRequest?: ApprovalRequest<any>) => {
69+
const mockApprovalRequest = (approvalRequest?: unknown) => {
6270
(
6371
useApprovalRequest as jest.MockedFn<typeof useApprovalRequest>
6472
).mockReturnValue({
@@ -69,9 +77,31 @@ const mockApprovalRequest = (approvalRequest?: ApprovalRequest<any>) => {
6977
} as any);
7078
};
7179

80+
const URL_MOCK = 'test.com';
81+
82+
const mockApprovalRequestData = {
83+
metadata: {
84+
rpcUrl: URL_MOCK,
85+
},
86+
diff: {
87+
permissionDiffMap: {
88+
[Caip25EndowmentPermissionName]: {
89+
[Caip25CaveatType]: {
90+
requiredScopes: {
91+
'eip155:1': {
92+
accounts: [],
93+
},
94+
},
95+
optionalScopes: {},
96+
},
97+
},
98+
},
99+
},
100+
};
101+
72102
describe('SwitchChainApproval', () => {
73103
beforeEach(() => {
74-
jest.resetAllMocks();
104+
jest.clearAllMocks();
75105
jest.spyOn(networks, 'isPortfolioViewEnabled').mockReturnValue(false);
76106
jest
77107
.spyOn(networks, 'isRemoveGlobalNetworkSelectorEnabled')
@@ -81,9 +111,8 @@ describe('SwitchChainApproval', () => {
81111
it('renders', () => {
82112
mockApprovalRequest({
83113
type: ApprovalTypes.SWITCH_ETHEREUM_CHAIN,
84-
// TODO: Replace "any" with type
85-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
86-
} as any);
114+
requestData: mockApprovalRequestData,
115+
});
87116

88117
const wrapper = shallow(<SwitchChainApproval />);
89118

@@ -109,12 +138,8 @@ describe('SwitchChainApproval', () => {
109138
it('calls networkSwitched action when confirm is pressed', () => {
110139
mockApprovalRequest({
111140
type: ApprovalTypes.SWITCH_ETHEREUM_CHAIN,
112-
requestData: {
113-
rpcUrl: URL_MOCK,
114-
},
115-
// TODO: Replace "any" with type
116-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
117-
} as any);
141+
requestData: mockApprovalRequestData,
142+
});
118143

119144
const wrapper = shallow(<SwitchChainApproval />);
120145
wrapper.find('SwitchCustomNetwork').simulate('confirm');
@@ -130,12 +155,8 @@ describe('SwitchChainApproval', () => {
130155
jest.spyOn(networks, 'isPortfolioViewEnabled').mockReturnValue(true);
131156
mockApprovalRequest({
132157
type: ApprovalTypes.SWITCH_ETHEREUM_CHAIN,
133-
requestData: {
134-
rpcUrl: URL_MOCK,
135-
},
136-
} as ApprovalRequest<{
137-
rpcUrl: string;
138-
}>);
158+
requestData: mockApprovalRequestData,
159+
});
139160

140161
const wrapper = shallow(<SwitchChainApproval />);
141162
wrapper.find('SwitchCustomNetwork').simulate('confirm');
@@ -155,14 +176,8 @@ describe('SwitchChainApproval', () => {
155176

156177
mockApprovalRequest({
157178
type: ApprovalTypes.SWITCH_ETHEREUM_CHAIN,
158-
requestData: {
159-
rpcUrl: URL_MOCK,
160-
chainId: '0x1',
161-
},
162-
} as ApprovalRequest<{
163-
rpcUrl: string;
164-
chainId: string;
165-
}>);
179+
requestData: mockApprovalRequestData,
180+
});
166181

167182
const wrapper = shallow(<SwitchChainApproval />);
168183
wrapper.find('SwitchCustomNetwork').simulate('confirm');
@@ -184,14 +199,8 @@ describe('SwitchChainApproval', () => {
184199

185200
mockApprovalRequest({
186201
type: ApprovalTypes.SWITCH_ETHEREUM_CHAIN,
187-
requestData: {
188-
rpcUrl: URL_MOCK,
189-
chainId: '0x1',
190-
},
191-
} as ApprovalRequest<{
192-
rpcUrl: string;
193-
chainId: string;
194-
}>);
202+
requestData: mockApprovalRequestData,
203+
});
195204

196205
const wrapper = shallow(<SwitchChainApproval />);
197206
wrapper.find('SwitchCustomNetwork').simulate('confirm');

app/components/Approvals/SwitchChainApproval/SwitchChainApproval.tsx

Lines changed: 49 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { ApprovalTypes } from '../../../core/RPCMethods/RPCMethodMiddleware';
44
import ApprovalModal from '../ApprovalModal';
55
import SwitchCustomNetwork from '../../UI/SwitchCustomNetwork';
66
import { networkSwitched } from '../../../actions/onboardNetwork';
7-
import { useDispatch } from 'react-redux';
7+
import { useDispatch, useSelector } from 'react-redux';
88
import {
99
isPortfolioViewEnabled,
1010
isRemoveGlobalNetworkSelectorEnabled,
@@ -14,6 +14,12 @@ import {
1414
useNetworksByNamespace,
1515
} from '../../hooks/useNetworksByNamespace/useNetworksByNamespace';
1616
import { useNetworkSelection } from '../../hooks/useNetworkSelection/useNetworkSelection';
17+
import {
18+
Caip25CaveatType,
19+
Caip25EndowmentPermissionName,
20+
getPermittedEthChainIds,
21+
} from '@metamask/chain-agnostic-permission';
22+
import { selectEvmNetworkConfigurationsByChainId } from '../../../selectors/networkController';
1723

1824
const SwitchChainApproval = () => {
1925
const {
@@ -31,37 +37,64 @@ const SwitchChainApproval = () => {
3137
networks,
3238
});
3339

40+
const evmNetworkConfigurations = useSelector(
41+
selectEvmNetworkConfigurationsByChainId,
42+
);
43+
44+
const caip25CaveatValue =
45+
approvalRequest?.requestData?.diff?.permissionDiffMap?.[
46+
Caip25EndowmentPermissionName
47+
]?.[Caip25CaveatType];
48+
49+
const permittedEthChainIds = caip25CaveatValue
50+
? getPermittedEthChainIds(caip25CaveatValue)
51+
: [];
52+
53+
// This approval view only handles one chainId added to permissions at a time, and the permissionDiffMap should only contain one chainId
54+
const chainId = permittedEthChainIds[0];
55+
3456
const onConfirm = useCallback(() => {
3557
defaultOnConfirm();
3658

3759
// If portfolio view is enabled should set network filter
3860
if (isPortfolioViewEnabled()) {
3961
if (isRemoveGlobalNetworkSelectorEnabled()) {
40-
selectNetwork(approvalRequest?.requestData?.chainId);
62+
selectNetwork(chainId);
4163
}
4264
}
4365

4466
dispatch(
4567
networkSwitched({
46-
networkUrl: approvalRequest?.requestData?.rpcUrl,
68+
networkUrl: approvalRequest?.requestData?.metadata?.rpcUrl,
4769
networkStatus: true,
4870
}),
4971
);
50-
}, [approvalRequest, defaultOnConfirm, dispatch, selectNetwork]);
72+
}, [approvalRequest, defaultOnConfirm, dispatch, selectNetwork, chainId]);
5173

52-
if (approvalRequest?.type !== ApprovalTypes.SWITCH_ETHEREUM_CHAIN)
53-
return null;
74+
if (
75+
approvalRequest?.requestData?.diff?.permissionDiffMap?.[
76+
Caip25EndowmentPermissionName
77+
] ||
78+
// TODO: Revisit removing this when we DRY the addEthereumChain and switchEthereumChain handlers into core
79+
approvalRequest?.type === ApprovalTypes.SWITCH_ETHEREUM_CHAIN
80+
) {
81+
const customNetworkInformation = {
82+
chainId,
83+
chainName: evmNetworkConfigurations[chainId]?.name,
84+
};
5485

55-
return (
56-
<ApprovalModal isVisible onCancel={onReject}>
57-
<SwitchCustomNetwork
58-
onCancel={onReject}
59-
onConfirm={onConfirm}
60-
currentPageInformation={pageMeta}
61-
customNetworkInformation={approvalRequest?.requestData}
62-
/>
63-
</ApprovalModal>
64-
);
86+
return (
87+
<ApprovalModal isVisible onCancel={onReject}>
88+
<SwitchCustomNetwork
89+
onCancel={onReject}
90+
onConfirm={onConfirm}
91+
currentPageInformation={pageMeta}
92+
customNetworkInformation={customNetworkInformation}
93+
/>
94+
</ApprovalModal>
95+
);
96+
}
97+
return null;
6598
};
6699

67100
export default SwitchChainApproval;

app/components/Approvals/SwitchChainApproval/__snapshots__/SwitchChainApproval.test.tsx.snap

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@ exports[`SwitchChainApproval renders 1`] = `
55
isVisible={true}
66
>
77
<SwitchCustomNetwork
8+
customNetworkInformation={
9+
{
10+
"chainId": "0x1",
11+
"chainName": "Ethereum Mainnet",
12+
}
13+
}
814
onConfirm={[Function]}
915
/>
1016
</ApprovalModal>

app/components/Views/confirmations/hooks/useApprovalRequest.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,10 @@ const useApprovalRequest = () => {
5151
);
5252

5353
const pageMeta = useMemo(
54-
() => approvalRequest?.requestData?.pageMeta ?? {},
54+
() =>
55+
approvalRequest?.requestData?.pageMeta ??
56+
approvalRequest?.requestData?.metadata?.pageMeta ??
57+
{},
5558
[approvalRequest],
5659
);
5760

app/core/RPCMethods/RPCMethodMiddleware.test.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1725,7 +1725,21 @@ describe('getRpcMethodMiddleware', () => {
17251725

17261726
describe('getRpcMethodMiddlewareHooks', () => {
17271727
const testOrigin = 'https://test.com';
1728-
const hooks = getRpcMethodMiddlewareHooks(testOrigin);
1728+
const mockUrl = { current: 'https://test.com' };
1729+
const mockTitle = { current: 'Test Site' };
1730+
const mockIcon = { current: undefined };
1731+
const mockAnalytics = { test: true };
1732+
const mockGetSource = jest.fn(() => 'test-source');
1733+
1734+
const hooks = getRpcMethodMiddlewareHooks({
1735+
origin: testOrigin,
1736+
url: mockUrl,
1737+
title: mockTitle,
1738+
icon: mockIcon,
1739+
analytics: mockAnalytics,
1740+
channelId: 'test-channel',
1741+
getSource: mockGetSource,
1742+
});
17291743

17301744
beforeEach(() => {
17311745
jest.clearAllMocks();

0 commit comments

Comments
 (0)