refactor: remove unused startApprovalFlow and endApprovalFlow hoo…

…ks from `wallet_addEthereumChain` and `wallet_switchEthereumChain` (#28189)

<!--
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**

Removes unneeded `startApprovalFlow` and `endApprovalFlow` hooks from
the `wallet_addEthereumChain` and related `switchChain` helper

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28189?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

No dapp or user facing changes

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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.

---------

Co-authored-by: MetaMask Bot <[email protected]>

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.js

@@ -18,8 +18,6 @@ const addEthereumChain = {
     getNetworkConfigurationByChainId: true,
     setActiveNetwork: true,
     requestUserApproval: true,
-    startApprovalFlow: true,
-    endApprovalFlow: true,
     getCurrentChainIdForDomain: true,
     getCaveat: true,
     requestPermittedChainsPermissionForOrigin: true,
@@ -40,8 +38,6 @@ async function addEthereumChainHandler(
     getNetworkConfigurationByChainId,
     setActiveNetwork,
     requestUserApproval,
-    startApprovalFlow,
-    endApprovalFlow,
     getCurrentChainIdForDomain,
     getCaveat,
     requestPermittedChainsPermissionForOrigin,
@@ -79,7 +75,6 @@ async function addEthereumChainHandler(
     );
   }
 
-  let approvalFlowId;
   let updatedNetwork = existingNetwork;
 
   let rpcIndex = existingNetwork?.rpcEndpoints.findIndex(({ url }) =>
@@ -99,8 +94,6 @@ async function addEthereumChainHandler(
     (firstValidBlockExplorerUrl &&
       blockExplorerIndex !== existingNetwork.defaultBlockExplorerUrlIndex)
   ) {
-    ({ id: approvalFlowId } = await startApprovalFlow());
-
     try {
       await requestUserApproval({
         origin,
@@ -183,7 +176,6 @@ async function addEthereumChainHandler(
         });
       }
     } catch (error) {
-      endApprovalFlow({ id: approvalFlowId });
       return end(error);
     }
   }
@@ -193,16 +185,13 @@ async function addEthereumChainHandler(
     const { networkClientId } =
       updatedNetwork.rpcEndpoints[updatedNetwork.defaultRpcEndpointIndex];
 
-    return switchChain(res, end, chainId, networkClientId, approvalFlowId, {
+    return switchChain(res, end, chainId, networkClientId, {
       isAddFlow: true,
       setActiveNetwork,
       getCaveat,
-      endApprovalFlow,
       requestPermittedChainsPermissionForOrigin,
       requestPermittedChainsPermissionIncrementalForOrigin,
     });
-  } else if (approvalFlowId) {
-    endApprovalFlow({ id: approvalFlowId });
   }
 
   res.result = null;

app/scripts/lib/rpc-method-middleware/handlers/add-ethereum-chain.test.js

@@ -69,8 +69,6 @@ const createMockedHandler = () => {
     setActiveNetwork: jest.fn(),
     requestUserApproval: jest.fn().mockResolvedValue(123),
     getCaveat: jest.fn(),
-    startApprovalFlow: () => ({ id: 'approvalFlowId' }),
-    endApprovalFlow: jest.fn(),
     addNetwork: jest.fn().mockResolvedValue({
       defaultRpcEndpointIndex: 0,
       rpcEndpoints: [{ networkClientId: 123 }],
@@ -185,10 +183,8 @@ describe('addEthereumChainHandler', () => {
       end,
       NON_INFURA_CHAIN_ID,
       123,
-      'approvalFlowId',
       {
         isAddFlow: true,
-        endApprovalFlow: mocks.endApprovalFlow,
         getCaveat: mocks.getCaveat,
         setActiveNetwork: mocks.setActiveNetwork,
         requestPermittedChainsPermissionForOrigin:
@@ -254,10 +250,8 @@ describe('addEthereumChainHandler', () => {
           end,
           '0x1',
           123,
-          'approvalFlowId',
           {
             isAddFlow: true,
-            endApprovalFlow: mocks.endApprovalFlow,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:
@@ -303,10 +297,8 @@ describe('addEthereumChainHandler', () => {
           end,
           '0xa',
           createMockOptimismConfiguration().rpcEndpoints[0].networkClientId,
-          undefined,
           {
             isAddFlow: true,
-            endApprovalFlow: mocks.endApprovalFlow,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js

@@ -1,4 +1,4 @@
-import { errorCodes, rpcErrors } from '@metamask/rpc-errors';
+import { rpcErrors } from '@metamask/rpc-errors';
 import {
   Caip25CaveatType,
   Caip25EndowmentPermissionName,
@@ -164,11 +164,9 @@ export function validateAddEthereumChainParams(params) {
  * @param end - The JSON RPC request's end callback.
  * @param {string} chainId - The chainId being switched to.
  * @param {string} networkClientId - The network client being switched to.
- * @param {string} [approvalFlowId] - The optional approval flow ID to handle.
  * @param {object} hooks - The hooks object.
  * @param {boolean} hooks.isAddFlow - The boolean determining if this call originates from wallet_addEthereumChain.
  * @param {Function} hooks.setActiveNetwork - The callback to change the current network for the origin.
- * @param {Function} hooks.endApprovalFlow - The optional callback to end the approval flow when approvalFlowId is provided.
  * @param {Function} hooks.getCaveat - The callback to get the CAIP-25 caveat for the origin.
  * @param {Function} hooks.requestPermittedChainsPermissionForOrigin - The callback to request a new permittedChains-equivalent CAIP-25 permission.
  * @param {Function} hooks.requestPermittedChainsPermissionIncrementalForOrigin - The callback to add a new chain to the permittedChains-equivalent CAIP-25 permission.
@@ -179,11 +177,9 @@ export async function switchChain(
   end,
   chainId,
   networkClientId,
-  approvalFlowId,
   {
     isAddFlow,
     setActiveNetwork,
-    endApprovalFlow,
     getCaveat,
     requestPermittedChainsPermissionForOrigin,
     requestPermittedChainsPermissionIncrementalForOrigin,
@@ -213,24 +209,8 @@ export async function switchChain(
 
     await setActiveNetwork(networkClientId);
     response.result = null;
+    return end();
   } catch (error) {
-    // We don't want to return an error if user rejects the request
-    // and this is a chained switch request after wallet_addEthereumChain.
-    // approvalFlowId is only defined when this call is of a
-    // wallet_addEthereumChain request so we can use it to determine
-    // if we should return an error
-    if (
-      error.code === errorCodes.provider.userRejectedRequest &&
-      approvalFlowId
-    ) {
-      response.result = null;
-      return end();
-    }
     return end(error);
-  } finally {
-    if (approvalFlowId) {
-      endApprovalFlow({ id: approvalFlowId });
-    }
   }
-  return end();
 }

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.test.ts

@@ -1,4 +1,4 @@
-import { errorCodes, rpcErrors } from '@metamask/rpc-errors';
+import { rpcErrors } from '@metamask/rpc-errors';
 import {
   Caip25CaveatType,
   Caip25EndowmentPermissionName,
@@ -12,25 +12,13 @@ describe('Ethereum Chain Utils', () => {
     const mocks = {
       isAddFlow: false,
       setActiveNetwork: jest.fn(),
-      endApprovalFlow: jest.fn(),
       getCaveat: jest.fn(),
       requestPermittedChainsPermissionForOrigin: jest.fn(),
       requestPermittedChainsPermissionIncrementalForOrigin: jest.fn(),
     };
     const response: { result?: true } = {};
-    const switchChain = (
-      chainId: Hex,
-      networkClientId: string,
-      approvalFlowId?: string,
-    ) =>
-      EthChainUtils.switchChain(
-        response,
-        end,
-        chainId,
-        networkClientId,
-        approvalFlowId,
-        mocks,
-      );
+    const switchChain = (chainId: Hex, networkClientId: string) =>
+      EthChainUtils.switchChain(response, end, chainId, networkClientId, mocks);
 
     return {
       mocks,
@@ -43,63 +31,30 @@ describe('Ethereum Chain Utils', () => {
   describe('switchChain', () => {
     it('gets the CAIP-25 caveat', async () => {
       const { mocks, switchChain } = createMockedSwitchChain();
-      await switchChain('0x1', 'mainnet', 'approvalFlowId');
+      await switchChain('0x1', 'mainnet');
 
       expect(mocks.getCaveat).toHaveBeenCalledWith({
         target: Caip25EndowmentPermissionName,
         caveatType: Caip25CaveatType,
       });
     });
 
-    it('passes through unexpected errors if approvalFlowId is not provided', async () => {
-      const { mocks, end, switchChain } = createMockedSwitchChain();
-      mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
-        new Error('unexpected error'),
-      );
-
-      await switchChain('0x1', 'mainnet', undefined);
-
-      expect(end).toHaveBeenCalledWith(new Error('unexpected error'));
-    });
-
-    it('passes through unexpected errors if approvalFlowId is provided', async () => {
+    it('passes through unexpected errors', async () => {
       const { mocks, end, switchChain } = createMockedSwitchChain();
       mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
         new Error('unexpected error'),
       );
 
-      await switchChain('0x1', 'mainnet', 'approvalFlowId');
+      await switchChain('0x1', 'mainnet');
 
       expect(end).toHaveBeenCalledWith(new Error('unexpected error'));
     });
 
-    it('ignores userRejectedRequest errors when approvalFlowId is provided', async () => {
-      const { mocks, end, response, switchChain } = createMockedSwitchChain();
-      mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce({
-        code: errorCodes.provider.userRejectedRequest,
-      });
-
-      await switchChain('0x1', 'mainnet', 'approvalFlowId');
-
-      expect(response.result).toStrictEqual(null);
-      expect(end).toHaveBeenCalledWith();
-    });
-
-    it('ends the approval flow when approvalFlowId is provided', async () => {
-      const { mocks, switchChain } = createMockedSwitchChain();
-
-      await switchChain('0x1', 'mainnet', 'approvalFlowId');
-
-      expect(mocks.endApprovalFlow).toHaveBeenCalledWith({
-        id: 'approvalFlowId',
-      });
-    });
-
     describe('with no existing CAIP-25 permission', () => {
       it('requests a switch chain approval without autoApprove if isAddFlow: false', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
         mocks.isAddFlow = false;
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionForOrigin,
@@ -108,24 +63,26 @@ describe('Ethereum Chain Utils', () => {
 
       it('switches to the chain', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the switch chain approval is rejected', async () => {
+      it('should handle errors if the switch chain grant fails', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
-        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce({
-          code: errorCodes.provider.userRejectedRequest,
-        });
+        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
+          new Error('failed to grant permittedChains'),
+        );
 
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionForOrigin,
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
-        expect(end).toHaveBeenCalledWith();
+        expect(end).toHaveBeenCalledWith(
+          new Error('failed to grant permittedChains'),
+        );
       });
     });
 
@@ -140,7 +97,7 @@ describe('Ethereum Chain Utils', () => {
             isMultichainOrigin: false,
           },
         });
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionIncrementalForOrigin,
@@ -158,20 +115,18 @@ describe('Ethereum Chain Utils', () => {
             isMultichainOrigin: false,
           },
         });
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionIncrementalForOrigin,
         ).toHaveBeenCalledWith({ chainId: '0x1', autoApprove: false });
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the permittedChains approval is rejected', async () => {
+      it('should handle errors if the permittedChains grant fails', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
         mocks.requestPermittedChainsPermissionIncrementalForOrigin.mockRejectedValueOnce(
-          {
-            code: errorCodes.provider.userRejectedRequest,
-          },
+          new Error('failed to grant permittedChains'),
         );
         mocks.getCaveat.mockReturnValue({
           value: {
@@ -180,13 +135,15 @@ describe('Ethereum Chain Utils', () => {
             isMultichainOrigin: false,
           },
         });
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionIncrementalForOrigin,
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
-        expect(end).toHaveBeenCalledWith();
+        expect(end).toHaveBeenCalledWith(
+          new Error('failed to grant permittedChains'),
+        );
       });
     });
 
@@ -205,7 +162,7 @@ describe('Ethereum Chain Utils', () => {
             isMultichainOrigin: true,
           },
         });
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionIncrementalForOrigin,
@@ -227,7 +184,7 @@ describe('Ethereum Chain Utils', () => {
           ),
         );
 
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
       });
@@ -247,7 +204,7 @@ describe('Ethereum Chain Utils', () => {
           ),
         );
 
-        await switchChain('0x1', 'mainnet', 'approvalFlowId');
+        await switchChain('0x1', 'mainnet');
 
         expect(end).toHaveBeenCalledWith(
           new Error(
@@ -277,7 +234,7 @@ describe('Ethereum Chain Utils', () => {
               isMultichainOrigin,
             },
           });
-          await switchChain('0x1', 'mainnet', 'approvalFlowId');
+          await switchChain('0x1', 'mainnet');
 
           expect(
             mocks.requestPermittedChainsPermissionIncrementalForOrigin,
@@ -297,7 +254,7 @@ describe('Ethereum Chain Utils', () => {
               isMultichainOrigin,
             },
           });
-          await switchChain('0x1', 'mainnet', 'approvalFlowId');
+          await switchChain('0x1', 'mainnet');
 
           expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
         });

app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js

@@ -64,7 +64,7 @@ async function switchEthereumChainHandler(
     );
   }
 
-  return switchChain(res, end, chainId, networkClientIdToSwitchTo, null, {
+  return switchChain(res, end, chainId, networkClientIdToSwitchTo, {
     setActiveNetwork,
     getCaveat,
     requestPermittedChainsPermissionForOrigin,