fix: handle snaps error

packages/assets-controllers/CHANGELOG.md

@@ -13,6 +13,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **BREAKING:** Bump peer dependency `@metamask/phishing-controller` to `^13.0.0` ([#6098](https://github.com/MetaMask/core/pull/6098))
 
+### Fixed
+
+- Improve error handling in `MultichainAssetsRatesController` for Snap request failures ([#6104](https://github.com/MetaMask/core/pull/6104))
+  - Enhanced `#handleSnapRequest` method with detailed error logging and graceful failure recovery
+  - Added null safety checks to prevent crashes when Snap requests return null
+  - Controller now continues operation when individual Snap requests fail instead of crashing
+  - Added comprehensive unit tests covering various error scenarios including JSON-RPC errors and network failures
+
 ## [70.0.1]
 
 ### Changed

packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts

@@ -526,6 +526,192 @@ describe('MultichainAssetsRatesController', () => {
     expect(updateSpy).toHaveBeenCalled();
   });
 
+  describe('error handling in snap requests', () => {
+    it('handles JSON-RPC parameter validation errors gracefully', async () => {
+      const { controller, messenger } = setupController();
+
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+      const paramValidationError = new Error(
+        'Invalid request params: At path: conversions.0.from -- Expected a value of type `CaipAssetType`, but received: `"swift:0/test-asset"`.',
+      );
+
+      const snapHandler = jest.fn().mockRejectedValue(paramValidationError);
+      messenger.registerActionHandler(
+        'SnapController:handleRequest',
+        snapHandler,
+      );
+
+      await controller.updateAssetsRates();
+
+      // Should have logged the error with detailed context
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Snap request failed for onAssetsConversion:',
+        expect.objectContaining({
+          snapId: 'test-snap',
+          handler: 'onAssetsConversion',
+          message: expect.stringContaining('Invalid request params'),
+          params: expect.objectContaining({
+            conversions: expect.arrayContaining([
+              expect.objectContaining({
+                from: 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501',
+                to: 'swift:0/iso4217:USD',
+              }),
+            ]),
+          }),
+        }),
+      );
+
+      // Should not update state when snap request fails
+      expect(controller.state.conversionRates).toStrictEqual({});
+
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('handles generic snap request errors gracefully', async () => {
+      const { controller, messenger } = setupController();
+
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+      const genericError = new Error('Network timeout');
+
+      const snapHandler = jest.fn().mockRejectedValue(genericError);
+      messenger.registerActionHandler(
+        'SnapController:handleRequest',
+        snapHandler,
+      );
+
+      await controller.updateAssetsRates();
+
+      // Should have logged the error with detailed context
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Snap request failed for onAssetsConversion:',
+        expect.objectContaining({
+          snapId: 'test-snap',
+          handler: 'onAssetsConversion',
+          message: 'Network timeout',
+          params: expect.any(Object),
+        }),
+      );
+
+      // Should not update state when snap request fails
+      expect(controller.state.conversionRates).toStrictEqual({});
+
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('handles unknown error types gracefully', async () => {
+      const { controller, messenger } = setupController();
+
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+      const unknownError = 'String error instead of Error object';
+
+      const snapHandler = jest.fn().mockRejectedValue(unknownError);
+      messenger.registerActionHandler(
+        'SnapController:handleRequest',
+        snapHandler,
+      );
+
+      await controller.updateAssetsRates();
+
+      // Should have logged the unknown error
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Unknown error in snap request:',
+        unknownError,
+      );
+
+      // Should not update state when snap request fails
+      expect(controller.state.conversionRates).toStrictEqual({});
+
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('handles mixed success and failure scenarios', async () => {
+      const { controller, messenger } = setupController({
+        accountsAssets: [fakeNonEvmAccount, fakeEvmAccount2],
+      });
+
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+
+      // Mock different responses for different calls
+      const snapHandler = jest
+        .fn()
+        .mockResolvedValueOnce(fakeAccountRates) // First call succeeds (onAssetsConversion)
+        .mockResolvedValueOnce({
+          marketData: {
+            'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': {
+              'swift:0/iso4217:USD': fakeMarketData,
+            },
+          },
+        }) // Second call succeeds (onAssetsMarketData)
+        .mockRejectedValueOnce(new Error('Snap request failed')) // Third call fails (onAssetsConversion)
+        .mockResolvedValueOnce(null); // Fourth call returns null (onAssetsMarketData)
+
+      messenger.registerActionHandler(
+        'SnapController:handleRequest',
+        snapHandler,
+      );
+
+      await controller.updateAssetsRates();
+
+      // Should have logged the error for the failed request
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Snap request failed for onAssetsConversion:',
+        expect.objectContaining({
+          message: 'Snap request failed',
+        }),
+      );
+
+      // Should still update state for the successful request
+      expect(controller.state.conversionRates).toMatchObject({
+        'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': {
+          rate: '202.11',
+          conversionTime: 1738539923277,
+          currency: 'swift:0/iso4217:USD',
+          marketData: fakeMarketData,
+        },
+      });
+
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('handles market data request errors independently', async () => {
+      const { controller, messenger } = setupController();
+
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+
+      // Mock onAssetsConversion to succeed but onAssetsMarketData to fail
+      const snapHandler = jest
+        .fn()
+        .mockResolvedValueOnce(fakeAccountRates) // onAssetsConversion succeeds
+        .mockRejectedValueOnce(new Error('Market data unavailable')); // onAssetsMarketData fails
+
+      messenger.registerActionHandler(
+        'SnapController:handleRequest',
+        snapHandler,
+      );
+
+      await controller.updateAssetsRates();
+
+      // Should have logged the market data error
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Snap request failed for onAssetsMarketData:',
+        expect.objectContaining({
+          message: 'Market data unavailable',
+        }),
+      );
+
+      // Should still update state with conversion rates (without market data)
+      expect(controller.state.conversionRates).toMatchObject({
+        'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': {
+          rate: '202.11',
+          conversionTime: 1738539923277,
+          currency: 'swift:0/iso4217:USD',
+        },
+      });
+
+      consoleErrorSpy.mockRestore();
+    });
+  });
+
   describe('fetchHistoricalPricesForAsset', () => {
     it('throws an error if call to snap fails', async () => {
       const testAsset = 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501';

packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts

@@ -352,7 +352,12 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro
       snapId: account?.metadata.snap?.id as SnapId,
       handler: HandlerType.OnAssetsConversion,
       params: conversions,
-    })) as OnAssetsConversionResponse;
+    })) as OnAssetsConversionResponse | null;
+
+    // If the snap request failed, return empty rates
+    if (!accountRatesResponse) {
+      return {};
+    }
 
     // Prepare assets param for onAssetsMarketData
     const currentCurrencyCaip =
@@ -366,7 +371,7 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro
       snapId: account?.metadata.snap?.id as SnapId,
       handler: HandlerType.OnAssetsMarketData,
       params: assetsParam as OnAssetsMarketDataArguments,
-    })) as OnAssetsMarketDataResponse;
+    })) as OnAssetsMarketDataResponse | null;
 
     // Merge market data into conversion rates if available
     const mergedRates = this.#mergeMarketDataIntoConversionRates(
@@ -417,14 +422,22 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro
           'AccountsController:getSelectedMultichainAccount',
         );
       try {
-        const historicalPricesResponse = await this.#handleSnapRequest({
-          snapId: selectedAccount?.metadata.snap?.id as SnapId,
-          handler: HandlerType.OnAssetHistoricalPrice,
-          params: {
-            from: asset,
-            to: currentCaipCurrency,
+        const historicalPricesResponse = await this.messagingSystem.call(
+          'SnapController:handleRequest',
+          {
+            snapId: selectedAccount?.metadata.snap?.id as SnapId,
+            origin: 'metamask',
+            handler: HandlerType.OnAssetHistoricalPrice,
+            request: {
+              jsonrpc: '2.0',
+              method: HandlerType.OnAssetHistoricalPrice,
+              params: {
+                from: asset,
+                to: currentCaipCurrency,
+              },
+            },
           },
-        });
+        );
 
         // skip state update if no historical prices are returned
         if (!historicalPricesResponse) {
@@ -544,8 +557,12 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro
    * @returns A flattened rates object.
    */
   #flattenRates(
-    assetsConversionResponse: ConversionRatesWithMarketData,
+    assetsConversionResponse: ConversionRatesWithMarketData | null,
   ): Record<CaipAssetType, UnifiedAssetConversion | null> {
+    if (!assetsConversionResponse?.conversionRates) {
+      return {};
+    }
+
     const { conversionRates } = assetsConversionResponse;
 
     return Object.fromEntries(
@@ -635,26 +652,40 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro
     | OnAssetsMarketDataResponse
     | null
   > {
-    return this.messagingSystem.call('SnapController:handleRequest', {
-      snapId,
-      origin: 'metamask',
-      handler,
-      request: {
-        jsonrpc: '2.0',
-        method: handler,
-        params,
-      },
-    }) as Promise<
-      | OnAssetsConversionResponse
-      | OnAssetHistoricalPriceResponse
-      | OnAssetsMarketDataResponse
-      | null
-    >;
+    try {
+      return (await this.messagingSystem.call('SnapController:handleRequest', {
+        snapId,
+        origin: 'metamask',
+        handler,
+        request: {
+          jsonrpc: '2.0',
+          method: handler,
+          params,
+        },
+      })) as
+        | OnAssetsConversionResponse
+        | OnAssetHistoricalPriceResponse
+        | OnAssetsMarketDataResponse
+        | null;
+    } catch (error) {
+      // Handle specific error types
+      if (error instanceof Error) {
+        console.error(`Snap request failed for ${handler}:`, {
+          snapId,
+          handler,
+          message: error.message,
+          params,
+        });
+      } else {
+        console.error('Unknown error in snap request:', error);
+      }
+      return null;
+    }
   }
 
   #mergeMarketDataIntoConversionRates(
     accountRatesResponse: OnAssetsConversionResponse,
-    marketDataResponse: OnAssetsMarketDataResponse,
+    marketDataResponse: OnAssetsMarketDataResponse | null,
   ): ConversionRatesWithMarketData {
     // Early return if no market data to merge
     if (!marketDataResponse?.marketData) {