From cfcd7d25eef9dabf693921a6f1ab0bb8d1d93533 Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 11 Jul 2025 00:26:47 +0200 Subject: [PATCH 1/6] fix: handle snaps error --- .../MultichainAssetsRatesController.test.ts | 186 ++++++++++++++++++ .../MultichainAssetsRatesController.ts | 83 +++++--- 2 files changed, 243 insertions(+), 26 deletions(-) diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts index 3167fc6ecc2..46157020df0 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts +++ b/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'; diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts index fd79a9c5901..dbadc4dff64 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( @@ -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) { From b1b46045bb728d8efe2f474ce14ab8a4f67e9caf Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 11 Jul 2025 00:33:23 +0200 Subject: [PATCH 2/6] fix: add changelog --- packages/assets-controllers/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index e49ca9e4188..4bd55891837 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,6 +13,13 @@ 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 From 2a9bbd050211bca534cc40e03065c19f4749ee3f Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 11 Jul 2025 00:36:02 +0200 Subject: [PATCH 3/6] fix: rebase --- packages/assets-controllers/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 4bd55891837..6e9eda57628 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -14,6 +14,7 @@ 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 From c5c06add2480d9f390996cab27b74654501f49ab Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 18 Jul 2025 17:49:04 +0200 Subject: [PATCH 4/6] fix: fix PR comments --- packages/assets-controllers/CHANGELOG.md | 12 +++++----- .../MultichainAssetsRatesController.ts | 24 ++++++++----------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 6e9eda57628..f899dec1e6a 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,12 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## [71.0.0] - -### Changed - -- **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)) @@ -21,6 +15,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 +## [71.0.0] + +### Changed + +- **BREAKING:** Bump peer dependency `@metamask/phishing-controller` to `^13.0.0` ([#6098](https://github.com/MetaMask/core/pull/6098)) + ## [70.0.1] ### Changed diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts index dbadc4dff64..c9e7276b0fa 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts +++ b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts @@ -650,7 +650,7 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro | OnAssetsConversionResponse | OnAssetHistoricalPriceResponse | OnAssetsMarketDataResponse - | null + | undefined > { try { return (await this.messagingSystem.call('SnapController:handleRequest', { @@ -666,20 +666,16 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro | OnAssetsConversionResponse | OnAssetHistoricalPriceResponse | OnAssetsMarketDataResponse - | null; + | undefined; } 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; + console.error(`Snap request failed for ${handler}:`, { + snapId, + handler, + message: (error as Error).message, + params, + }); + // Ignore + return undefined; } } From 4ed61b1ffdcdf2d87d48e857fe7800005d87fe43 Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 18 Jul 2025 18:02:51 +0200 Subject: [PATCH 5/6] fix: fix changelog --- packages/assets-controllers/CHANGELOG.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 1b209af24fb..2392145f813 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- 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)) @@ -14,9 +18,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 -### Changed - -- Bump `@metamask/keyring-api` from `^18.0.0` to `^19.0.0` ([#6146](https://github.com/MetaMask/core/pull/6146)) ## [72.0.0] From 1e2009d7ac444dfc6cc283f0c1d46867f2de0d90 Mon Sep 17 00:00:00 2001 From: salimtb Date: Fri, 18 Jul 2025 18:06:49 +0200 Subject: [PATCH 6/6] fix: fix unit tests --- .../MultichainAssetsRatesController.test.ts | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts index 46157020df0..cfcef7160a9 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts +++ b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts @@ -598,32 +598,6 @@ describe('MultichainAssetsRatesController', () => { 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],