diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 8aa353a3653..2392145f813 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -11,6 +11,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/keyring-api` from `^18.0.0` to `^19.0.0` ([#6146](https://github.com/MetaMask/core/pull/6146)) +### 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 + ## [72.0.0] ### Changed diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts index 3167fc6ecc2..cfcef7160a9 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts +++ b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts @@ -526,6 +526,166 @@ 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 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'; diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts index fd79a9c5901..c9e7276b0fa 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts +++ b/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 { + if (!assetsConversionResponse?.conversionRates) { + return {}; + } + const { conversionRates } = assetsConversionResponse; return Object.fromEntries( @@ -633,28 +650,38 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro | OnAssetsConversionResponse | OnAssetHistoricalPriceResponse | OnAssetsMarketDataResponse - | null + | undefined > { - return this.messagingSystem.call('SnapController:handleRequest', { - snapId, - origin: 'metamask', - handler, - request: { - jsonrpc: '2.0', - method: handler, + try { + return (await this.messagingSystem.call('SnapController:handleRequest', { + snapId, + origin: 'metamask', + handler, + request: { + jsonrpc: '2.0', + method: handler, + params, + }, + })) as + | OnAssetsConversionResponse + | OnAssetHistoricalPriceResponse + | OnAssetsMarketDataResponse + | undefined; + } catch (error) { + console.error(`Snap request failed for ${handler}:`, { + snapId, + handler, + message: (error as Error).message, params, - }, - }) as Promise< - | OnAssetsConversionResponse - | OnAssetHistoricalPriceResponse - | OnAssetsMarketDataResponse - | null - >; + }); + // Ignore + return undefined; + } } #mergeMarketDataIntoConversionRates( accountRatesResponse: OnAssetsConversionResponse, - marketDataResponse: OnAssetsMarketDataResponse, + marketDataResponse: OnAssetsMarketDataResponse | null, ): ConversionRatesWithMarketData { // Early return if no market data to merge if (!marketDataResponse?.marketData) {