fix: fix granted permissions when adding an existing network via wallet_addEthereumChain

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

@@ -88,12 +88,14 @@ async function addEthereumChainHandler(
     : undefined;
 
   // If there's something to add or update
-  if (
+
+  const shouldAddOrUpdateNetwork =
     !existingNetwork ||
     rpcIndex !== existingNetwork.defaultRpcEndpointIndex ||
     (firstValidBlockExplorerUrl &&
-      blockExplorerIndex !== existingNetwork.defaultBlockExplorerUrlIndex)
-  ) {
+      blockExplorerIndex !== existingNetwork.defaultBlockExplorerUrlIndex);
+
+  if (shouldAddOrUpdateNetwork) {
     try {
       await requestUserApproval({
         origin,
@@ -180,20 +182,14 @@ async function addEthereumChainHandler(
     }
   }
 
-  // If the added or updated network is not the current chain, prompt the user to switch
-  if (chainId !== currentChainIdForDomain) {
-    const { networkClientId } =
-      updatedNetwork.rpcEndpoints[updatedNetwork.defaultRpcEndpointIndex];
-
-    return switchChain(res, end, chainId, networkClientId, {
-      isAddFlow: true,
-      setActiveNetwork,
-      getCaveat,
-      requestPermittedChainsPermissionForOrigin,
-      requestPermittedChainsPermissionIncrementalForOrigin,
-    });
-  }
+  const { networkClientId } =
+    updatedNetwork.rpcEndpoints[updatedNetwork.defaultRpcEndpointIndex];
 
-  res.result = null;
-  return end();
+  return switchChain(res, end, chainId, networkClientId, {
+    autoApprove: shouldAddOrUpdateNetwork,
+    setActiveNetwork,
+    getCaveat,
+    requestPermittedChainsPermissionForOrigin,
+    requestPermittedChainsPermissionIncrementalForOrigin,
+  });
 }

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

@@ -184,7 +184,7 @@ describe('addEthereumChainHandler', () => {
       NON_INFURA_CHAIN_ID,
       123,
       {
-        isAddFlow: true,
+        autoApprove: true,
         getCaveat: mocks.getCaveat,
         setActiveNetwork: mocks.setActiveNetwork,
         requestPermittedChainsPermissionForOrigin:
@@ -251,7 +251,7 @@ describe('addEthereumChainHandler', () => {
           '0x1',
           123,
           {
-            isAddFlow: true,
+            autoApprove: true,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:
@@ -264,7 +264,7 @@ describe('addEthereumChainHandler', () => {
     });
 
     describe('if the proposed networkConfiguration does not have a different rpcUrl from the one already in state', () => {
-      it('should only switch to the existing networkConfiguration if one already exists for the given chain id', async () => {
+      it('should only switch to the existing networkConfiguration if one already exists for the given chain id without auto approving the chain permission', async () => {
         const { mocks, end, handler } = createMockedHandler();
         mocks.getCurrentChainIdForDomain.mockReturnValue(CHAIN_IDS.MAINNET);
         mocks.getNetworkConfigurationByChainId.mockReturnValue(
@@ -298,7 +298,7 @@ describe('addEthereumChainHandler', () => {
           '0xa',
           createMockOptimismConfiguration().rpcEndpoints[0].networkClientId,
           {
-            isAddFlow: true,
+            autoApprove: false,
             getCaveat: mocks.getCaveat,
             setActiveNetwork: mocks.setActiveNetwork,
             requestPermittedChainsPermissionForOrigin:
@@ -338,30 +338,4 @@ describe('addEthereumChainHandler', () => {
       }),
     );
   });
-
-  it('should add result set to null to response object if the requested rpcUrl (and chainId) is currently selected', async () => {
-    const CURRENT_RPC_CONFIG = createMockNonInfuraConfiguration();
-
-    const { mocks, response, handler } = createMockedHandler();
-    mocks.getCurrentChainIdForDomain.mockReturnValue(
-      CURRENT_RPC_CONFIG.chainId,
-    );
-    mocks.getNetworkConfigurationByChainId.mockReturnValue(CURRENT_RPC_CONFIG);
-    await handler({
-      origin: 'example.com',
-      params: [
-        {
-          chainId: CURRENT_RPC_CONFIG.chainId,
-          chainName: 'Custom Network',
-          rpcUrls: [CURRENT_RPC_CONFIG.rpcEndpoints[0].url],
-          nativeCurrency: {
-            symbol: CURRENT_RPC_CONFIG.nativeCurrency,
-            decimals: 18,
-          },
-          blockExplorerUrls: ['https://custom.blockexplorer'],
-        },
-      ],
-    });
-    expect(response.result).toBeNull();
-  });
 });

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

@@ -165,20 +165,20 @@ export function validateAddEthereumChainParams(params) {
  * @param {string} chainId - The chainId being switched to.
  * @param {string} networkClientId - The network client being switched to.
  * @param {object} hooks - The hooks object.
- * @param {boolean} hooks.isAddFlow - The boolean determining if this call originates from wallet_addEthereumChain.
+ * @param {boolean} [hooks.autoApprove] - A boolean indicating whether the request should prompt the user or be automatically approved.
  * @param {Function} hooks.setActiveNetwork - The callback to change the current network for the origin.
  * @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.
- * @returns a null response on success or an error if user rejects an approval when isAddFlow is false or on unexpected errors.
+ * @returns a null response on success or an error if user rejects an approval when autoApprove is false or on unexpected errors.
  */
 export async function switchChain(
   response,
   end,
   chainId,
   networkClientId,
   {
-    isAddFlow,
+    autoApprove,
     setActiveNetwork,
     getCaveat,
     requestPermittedChainsPermissionForOrigin,
@@ -197,13 +197,13 @@ export async function switchChain(
       if (!ethChainIds.includes(chainId)) {
         await requestPermittedChainsPermissionIncrementalForOrigin({
           chainId,
-          autoApprove: isAddFlow,
+          autoApprove,
         });
       }
     } else {
       await requestPermittedChainsPermissionForOrigin({
         chainId,
-        autoApprove: isAddFlow,
+        autoApprove,
       });
     }
 

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

@@ -1,4 +1,4 @@
-import { rpcErrors } from '@metamask/rpc-errors';
+import { errorCodes, rpcErrors } from '@metamask/rpc-errors';
 import {
   Caip25CaveatType,
   Caip25EndowmentPermissionName,
@@ -10,7 +10,7 @@ describe('Ethereum Chain Utils', () => {
   const createMockedSwitchChain = () => {
     const end = jest.fn();
     const mocks = {
-      isAddFlow: false,
+      autoApprove: false,
       setActiveNetwork: jest.fn(),
       getCaveat: jest.fn(),
       requestPermittedChainsPermissionForOrigin: jest.fn(),
@@ -39,21 +39,10 @@ describe('Ethereum Chain Utils', () => {
       });
     });
 
-    it('passes through unexpected errors', async () => {
-      const { mocks, end, switchChain } = createMockedSwitchChain();
-      mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
-        new Error('unexpected error'),
-      );
-
-      await switchChain('0x1', 'mainnet');
-
-      expect(end).toHaveBeenCalledWith(new Error('unexpected error'));
-    });
-
     describe('with no existing CAIP-25 permission', () => {
-      it('requests a switch chain approval without autoApprove if isAddFlow: false', async () => {
+      it('requests a switch chain approval without autoApprove if autoApprove: false', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = false;
+        mocks.autoApprove = false;
         await switchChain('0x1', 'mainnet');
 
         expect(
@@ -68,28 +57,28 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the switch chain grant fails', async () => {
+      it('should throw an error if the switch chain approval is rejected', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
-        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce(
-          new Error('failed to grant permittedChains'),
-        );
+        mocks.requestPermittedChainsPermissionForOrigin.mockRejectedValueOnce({
+          code: errorCodes.provider.userRejectedRequest,
+        });
 
         await switchChain('0x1', 'mainnet');
 
         expect(
           mocks.requestPermittedChainsPermissionForOrigin,
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
-        expect(end).toHaveBeenCalledWith(
-          new Error('failed to grant permittedChains'),
-        );
+        expect(end).toHaveBeenCalledWith({
+          code: errorCodes.provider.userRejectedRequest,
+        });
       });
     });
 
     describe('with an existing CAIP-25 permission granted from the legacy flow (isMultichainOrigin: false) and the chainId is not already permissioned', () => {
-      it('requests a switch chain approval with autoApprove and switches to it if isAddFlow: true', async () => {
+      it('requests a switch chain approval with autoApprove and switches to it if autoApprove: true', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = true;
+        mocks.autoApprove = true;
         mocks.getCaveat.mockReturnValue({
           value: {
             requiredScopes: {},
@@ -105,9 +94,9 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('requests permittedChains approval without autoApprove then switches to it if isAddFlow: false', async () => {
+      it('requests permittedChains approval without autoApprove then switches to it if autoApprove: false', async () => {
         const { mocks, switchChain } = createMockedSwitchChain();
-        mocks.isAddFlow = false;
+        mocks.autoApprove = false;
         mocks.getCaveat.mockReturnValue({
           value: {
             requiredScopes: {},
@@ -123,10 +112,10 @@ describe('Ethereum Chain Utils', () => {
         expect(mocks.setActiveNetwork).toHaveBeenCalledWith('mainnet');
       });
 
-      it('should handle errors if the permittedChains grant fails', async () => {
+      it('should throw errors if the permittedChains grant fails', async () => {
         const { mocks, end, switchChain } = createMockedSwitchChain();
         mocks.requestPermittedChainsPermissionIncrementalForOrigin.mockRejectedValueOnce(
-          new Error('failed to grant permittedChains'),
+          new Error('failed to auto grant permittedChains'),
         );
         mocks.getCaveat.mockReturnValue({
           value: {
@@ -142,7 +131,7 @@ describe('Ethereum Chain Utils', () => {
         ).toHaveBeenCalled();
         expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
         expect(end).toHaveBeenCalledWith(
-          new Error('failed to grant permittedChains'),
+          new Error('failed to auto grant permittedChains'),
         );
       });
     });