From d19c31d460d9f3affb074278d527c91f86d15502 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 9 Dec 2025 12:32:46 +0100 Subject: [PATCH 01/18] chore: integrate storageService with tokenListController --- packages/assets-controllers/package.json | 1 + .../src/TokenListController.test.ts | 1 - .../src/TokenListController.ts | 244 ++++++++++++++---- .../assets-controllers/tsconfig.build.json | 1 + packages/assets-controllers/tsconfig.json | 1 + yarn.lock | 3 +- 6 files changed, 201 insertions(+), 50 deletions(-) diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index 0f14de3a4a1..693b2f5133d 100644 --- a/packages/assets-controllers/package.json +++ b/packages/assets-controllers/package.json @@ -78,6 +78,7 @@ "@metamask/snaps-controllers": "^14.0.1", "@metamask/snaps-sdk": "^9.0.0", "@metamask/snaps-utils": "^11.0.0", + "@metamask/storage-service": "^0.0.0", "@metamask/transaction-controller": "^62.5.0", "@metamask/utils": "^11.8.1", "@types/bn.js": "^5.1.5", diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index b6a03aa7b52..01b5b556a49 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -1331,7 +1331,6 @@ describe('TokenListController', () => { ).toMatchInlineSnapshot(` Object { "preventPollingOnNetworkRestart": false, - "tokensChainsCache": Object {}, } `); }); diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 6ee57c476bb..c4172b30cbb 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -11,6 +11,11 @@ import type { NetworkControllerGetNetworkClientByIdAction, } from '@metamask/network-controller'; import { StaticIntervalPollingController } from '@metamask/polling-controller'; +import type { + StorageServiceSetItemAction, + StorageServiceGetItemAction, + StorageServiceRemoveItemAction, +} from '@metamask/storage-service'; import type { Hex } from '@metamask/utils'; import { Mutex } from 'async-mutex'; @@ -65,7 +70,11 @@ export type GetTokenListState = ControllerGetStateAction< export type TokenListControllerActions = GetTokenListState; -type AllowedActions = NetworkControllerGetNetworkClientByIdAction; +type AllowedActions = + | NetworkControllerGetNetworkClientByIdAction + | StorageServiceSetItemAction + | StorageServiceGetItemAction + | StorageServiceRemoveItemAction; type AllowedEvents = NetworkControllerStateChangeEvent; @@ -78,7 +87,7 @@ export type TokenListControllerMessenger = Messenger< const metadata: StateMetadata = { tokensChainsCache: { includeInStateLogs: false, - persist: true, + persist: false, // Persisted separately via StorageService includeInDebugSnapshot: true, usedInUi: true, }, @@ -110,17 +119,20 @@ export class TokenListController extends StaticIntervalPollingController { - private readonly mutex = new Mutex(); + readonly #mutex = new Mutex(); - private intervalId?: ReturnType; + // Storage key for StorageService + static readonly #storageKey = 'tokensChainsCache'; - private readonly intervalDelay: number; + #intervalId?: ReturnType; - private readonly cacheRefreshThreshold: number; + readonly #intervalDelay: number; - private chainId: Hex; + readonly #cacheRefreshThreshold: number; - private abortController: AbortController; + #chainId: Hex; + + #abortController: AbortController; /** * Creates a TokenListController instance. @@ -159,12 +171,27 @@ export class TokenListController extends StaticIntervalPollingController { + // Migrate existing cache from state to StorageService if needed + return this.#migrateStateToStorage(); + }) + .catch((error) => { + console.error( + 'TokenListController: Failed to load cache from storage:', + error, + ); + }); + if (onNetworkStateChange) { // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/no-misused-promises @@ -183,25 +210,124 @@ export class TokenListController extends StaticIntervalPollingController { + try { + const { result, error } = await this.messenger.call( + 'StorageService:getItem', + name, + TokenListController.#storageKey, + ); + + if (error) { + console.error( + 'TokenListController: Error loading cache from storage:', + error, + ); + return; + } + + if (result) { + // Load from StorageService into state + this.update((state) => { + state.tokensChainsCache = result as TokensChainsCache; + }); + } + } catch (error) { + console.error( + 'TokenListController: Failed to load cache from storage:', + error, + ); + } + } + + /** + * Save tokensChainsCache from state to StorageService. + * This persists large token data outside of the persisted state. + * + * @returns A promise that resolves when saving is complete. + */ + async #saveCacheToStorage(): Promise { + try { + await this.messenger.call( + 'StorageService:setItem', + name, + TokenListController.#storageKey, + this.state.tokensChainsCache, + ); + } catch (error) { + console.error( + 'TokenListController: Failed to save cache to storage:', + error, + ); + } + } + + /** + * Migrate tokensChainsCache from old persisted state to StorageService. + * This handles backward compatibility for users upgrading from versions + * where tokensChainsCache was persisted in state. + * + * @returns A promise that resolves when migration is complete. + */ + async #migrateStateToStorage(): Promise { + try { + // Check if state has data (from old persisted state) + const hasStateData = + this.state.tokensChainsCache && + Object.keys(this.state.tokensChainsCache).length > 0; + + if (!hasStateData) { + return; + } + + // Check if StorageService already has data + const { result } = await this.messenger.call( + 'StorageService:getItem', + name, + TokenListController.#storageKey, + ); + + // If StorageService is empty but state has data, migrate it + if (!result) { + await this.#saveCacheToStorage(); + } + } catch (error) { + console.error( + 'TokenListController: Failed to migrate cache to storage:', + error, + ); + } + } + /** * Updates state and restarts polling on changes to the network controller * state. * * @param networkControllerState - The updated network controller state. */ - async #onNetworkControllerStateChange(networkControllerState: NetworkState) { + async #onNetworkControllerStateChange( + networkControllerState: NetworkState, + ): Promise { const selectedNetworkClient = this.messenger.call( 'NetworkController:getNetworkClientById', networkControllerState.selectedNetworkClientId, ); const { chainId } = selectedNetworkClient.configuration; - if (this.chainId !== chainId) { - this.abortController.abort(); - this.abortController = new AbortController(); - this.chainId = chainId; + if (this.#chainId !== chainId) { + this.#abortController.abort(); + this.#abortController = new AbortController(); + this.#chainId = chainId; if (this.state.preventPollingOnNetworkRestart) { - this.clearingTokenListData(); + this.clearingTokenListData().catch((error) => { + console.error('Failed to clear token list data:', error); + }); } } } @@ -214,8 +340,8 @@ export class TokenListController extends StaticIntervalPollingController { + if (!isTokenListSupportedForNetwork(this.#chainId)) { return; } await this.#startDeprecatedPolling(); @@ -227,8 +353,8 @@ export class TokenListController extends StaticIntervalPollingController { + this.#stopPolling(); await this.#startDeprecatedPolling(); } @@ -238,8 +364,8 @@ export class TokenListController extends StaticIntervalPollingController { // renaming this to avoid collision with base class - await safelyExecute(() => this.fetchTokenList(this.chainId)); + await safelyExecute(() => this.fetchTokenList(this.#chainId)); // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/no-misused-promises - this.intervalId = setInterval(async () => { - await safelyExecute(() => this.fetchTokenList(this.chainId)); - }, this.intervalDelay); + this.#intervalId = setInterval(async () => { + await safelyExecute(() => this.fetchTokenList(this.#chainId)); + }, this.#intervalDelay); } /** @@ -293,12 +419,13 @@ export class TokenListController extends StaticIntervalPollingController { - const releaseLock = await this.mutex.acquire(); + const releaseLock = await this.#mutex.acquire(); try { if (this.isCacheValid(chainId)) { return; @@ -309,7 +436,7 @@ export class TokenListController extends StaticIntervalPollingController fetchTokenListByChainId( chainId, - this.abortController.signal, + this.#abortController.signal, ) as Promise, ); @@ -328,22 +455,30 @@ export class TokenListController extends StaticIntervalPollingController { - const newDataCache: DataCache = { data: {}, timestamp: Date.now() }; - state.tokensChainsCache[chainId] ??= newDataCache; - state.tokensChainsCache[chainId].data = tokenList; - state.tokensChainsCache[chainId].timestamp = Date.now(); + state.tokensChainsCache[chainId] = newDataCache; }); + + // Persist to StorageService (async, non-blocking) + await this.#saveCacheToStorage(); return; } // No response - fallback to previous state, or initialise empty if (!tokensFromAPI) { + const newDataCache: DataCache = { data: {}, timestamp: Date.now() }; this.update((state) => { - const newDataCache: DataCache = { data: {}, timestamp: Date.now() }; state.tokensChainsCache[chainId] ??= newDataCache; state.tokensChainsCache[chainId].timestamp = Date.now(); }); + + // Persist to StorageService + await this.#saveCacheToStorage(); } } finally { releaseLock(); @@ -355,20 +490,33 @@ export class TokenListController extends StaticIntervalPollingController { - return { - ...this.state, - tokensChainsCache: {}, - }; + async clearingTokenListData(): Promise { + // Clear state + this.update((state) => { + state.tokensChainsCache = {}; }); + + // Clear from StorageService + try { + await this.messenger.call( + 'StorageService:removeItem', + name, + TokenListController.#storageKey, + ); + } catch (error) { + console.error( + 'TokenListController: Failed to clear cache from storage:', + error, + ); + } } /** diff --git a/packages/assets-controllers/tsconfig.build.json b/packages/assets-controllers/tsconfig.build.json index 5ef0e52c3b8..d24f83bf4e8 100644 --- a/packages/assets-controllers/tsconfig.build.json +++ b/packages/assets-controllers/tsconfig.build.json @@ -18,6 +18,7 @@ { "path": "../preferences-controller/tsconfig.build.json" }, { "path": "../polling-controller/tsconfig.build.json" }, { "path": "../permission-controller/tsconfig.build.json" }, + { "path": "../storage-service/tsconfig.build.json" }, { "path": "../transaction-controller/tsconfig.build.json" }, { "path": "../phishing-controller/tsconfig.build.json" } ], diff --git a/packages/assets-controllers/tsconfig.json b/packages/assets-controllers/tsconfig.json index a537b98ca39..5a4630d0a47 100644 --- a/packages/assets-controllers/tsconfig.json +++ b/packages/assets-controllers/tsconfig.json @@ -18,6 +18,7 @@ { "path": "../phishing-controller" }, { "path": "../polling-controller" }, { "path": "../permission-controller" }, + { "path": "../storage-service" }, { "path": "../transaction-controller" } ], "include": ["../../types", "./src", "../../tests"] diff --git a/yarn.lock b/yarn.lock index 700332d93df..c7e7bbc187d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2663,6 +2663,7 @@ __metadata: "@metamask/snaps-controllers": "npm:^14.0.1" "@metamask/snaps-sdk": "npm:^9.0.0" "@metamask/snaps-utils": "npm:^11.0.0" + "@metamask/storage-service": "npm:^0.0.0" "@metamask/transaction-controller": "npm:^62.5.0" "@metamask/utils": "npm:^11.8.1" "@ts-bridge/cli": "npm:^0.6.4" @@ -4919,7 +4920,7 @@ __metadata: languageName: node linkType: hard -"@metamask/storage-service@workspace:packages/storage-service": +"@metamask/storage-service@npm:^0.0.0, @metamask/storage-service@workspace:packages/storage-service": version: 0.0.0-use.local resolution: "@metamask/storage-service@workspace:packages/storage-service" dependencies: From bffca81de8e058d993f8ccc58fdc7193b03f85d6 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 9 Dec 2025 12:57:03 +0100 Subject: [PATCH 02/18] chore: lint --- packages/assets-controllers/src/TokenListController.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 01b5b556a49..84a3f4e11a5 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -1069,7 +1069,7 @@ describe('TokenListController', () => { state: existingState, }); expect(controller.state).toStrictEqual(existingState); - controller.clearingTokenListData(); + await controller.clearingTokenListData(); expect(controller.state.tokensChainsCache).toStrictEqual({}); From 339ad1138f4f65f313d190768966a02a0c87a2f7 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 9 Dec 2025 15:08:25 +0100 Subject: [PATCH 03/18] fix: ut --- .../src/TokenListController.test.ts | 280 +++++++++++++++++- 1 file changed, 277 insertions(+), 3 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 84a3f4e11a5..7064bb79975 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -22,6 +22,7 @@ import type { TokenListMap, TokenListState, TokenListControllerMessenger, + TokensChainsCache, } from './TokenListController'; import { TokenListController } from './TokenListController'; import { advanceTime } from '../../../tests/helpers'; @@ -478,8 +479,42 @@ type RootMessenger = Messenger< AllTokenListControllerEvents >; +// Mock storage for StorageService +const mockStorage = new Map(); + const getMessenger = (): RootMessenger => { - return new Messenger({ namespace: MOCK_ANY_NAMESPACE }); + const messenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE }); + + // Register StorageService mock handlers + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:getItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + const value = mockStorage.get(storageKey); + return value ? { result: value } : {}; + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:setItem', + (controllerNamespace: string, key: string, value: unknown) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.set(storageKey, value); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:removeItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.delete(storageKey); + }, + ); + + return messenger; }; const getRestrictedMessenger = ( @@ -496,13 +531,23 @@ const getRestrictedMessenger = ( }); messenger.delegate({ messenger: tokenListControllerMessenger, - actions: ['NetworkController:getNetworkClientById'], + actions: [ + 'NetworkController:getNetworkClientById', + 'StorageService:getItem', + 'StorageService:setItem', + 'StorageService:removeItem', + ], events: ['NetworkController:stateChange'], }); return tokenListControllerMessenger; }; describe('TokenListController', () => { + beforeEach(() => { + // Clear mock storage between tests + mockStorage.clear(); + }); + afterEach(() => { jest.clearAllTimers(); sinon.restore(); @@ -1354,6 +1399,235 @@ describe('TokenListController', () => { `); }); }); + + describe('StorageService migration', () => { + it('should migrate tokensChainsCache from state to StorageService on first launch', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + // Simulate old persisted state with tokensChainsCache + const oldPersistedState = { + tokensChainsCache: { + [ChainId.mainnet]: { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }, + }, + preventPollingOnNetworkRestart: false, + }; + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + state: oldPersistedState, + }); + + // Fetch tokens to trigger save to storage (migration happens asynchronously in constructor) + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList); + + await controller.fetchTokenList(ChainId.mainnet); + + // Verify data was saved to StorageService + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toBeDefined(); + const resultCache = result as TokensChainsCache; + expect(resultCache[ChainId.mainnet]).toBeDefined(); + expect(resultCache[ChainId.mainnet].data).toBeDefined(); + + controller.destroy(); + }); + + it('should not overwrite StorageService if it already has data', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + // Pre-populate StorageService with existing data + const existingStorageData = { + [ChainId.mainnet]: { + data: { '0xExistingToken': { name: 'Existing', symbol: 'EXT' } }, + timestamp: Date.now(), + }, + }; + await messenger.call( + 'StorageService:setItem', + 'TokenListController', + 'tokensChainsCache', + existingStorageData, + ); + + // Initialize with different state data + const stateWithDifferentData = { + tokensChainsCache: { + [ChainId.mainnet]: { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }, + }, + preventPollingOnNetworkRestart: false, + }; + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + state: stateWithDifferentData, + }); + + // Wait for migration logic to run + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify StorageService still has original data (not overwritten) + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toStrictEqual(existingStorageData); + const resultCache = result as TokensChainsCache; + expect(resultCache[ChainId.mainnet].data).toStrictEqual( + existingStorageData[ChainId.mainnet].data, + ); + + controller.destroy(); + }); + + it('should not migrate when state has empty tokensChainsCache', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + state: { tokensChainsCache: {}, preventPollingOnNetworkRestart: false }, + }); + + // Wait for migration logic to run + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify nothing was saved to StorageService + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toBeUndefined(); + + controller.destroy(); + }); + + it('should save and load tokensChainsCache from StorageService', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + // Create controller and fetch tokens (which saves to storage) + const controller1 = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList); + + await controller1.fetchTokenList(ChainId.mainnet); + const savedCache = controller1.state.tokensChainsCache; + + controller1.destroy(); + + // Verify data is in StorageService + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toBeDefined(); + expect(result).toStrictEqual(savedCache); + }); + + it('should save tokensChainsCache to StorageService when fetching tokens', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + await controller.fetchTokenList(ChainId.mainnet); + + // Verify data was saved to StorageService (fetchTokenList awaits the save) + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toBeDefined(); + const resultCache = result as TokensChainsCache; + expect(resultCache[ChainId.mainnet]).toBeDefined(); + expect(resultCache[ChainId.mainnet].data).toBeDefined(); + + controller.destroy(); + }); + + it('should clear tokensChainsCache from StorageService when clearing data', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + // Pre-populate StorageService + const storageData = { + [ChainId.mainnet]: { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }, + }; + await messenger.call( + 'StorageService:setItem', + 'TokenListController', + 'tokensChainsCache', + storageData, + ); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + state: { + tokensChainsCache: storageData, + preventPollingOnNetworkRestart: false, + }, + }); + + // Wait a bit for async initialization to complete + await new Promise((resolve) => setTimeout(resolve, 50)); + + await controller.clearingTokenListData(); + + // Verify data was removed from StorageService (clearingTokenListData awaits the removal) + const { result } = await messenger.call( + 'StorageService:getItem', + 'TokenListController', + 'tokensChainsCache', + ); + + expect(result).toBeUndefined(); + expect(controller.state.tokensChainsCache).toStrictEqual({}); + + controller.destroy(); + }); + }); }); /** @@ -1362,7 +1636,7 @@ describe('TokenListController', () => { * @param chainId - The chain ID. * @returns The constructed path. */ -function getTokensPath(chainId: Hex) { +function getTokensPath(chainId: Hex): string { return `/tokens/${convertHexToDecimal( chainId, )}?occurrenceFloor=3&includeNativeAssets=false&includeTokenFees=false&includeAssetType=false&includeERC20Permit=false&includeStorage=false`; From ec25b7283950259fbb8e46eb3c12d7860001d319 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 9 Dec 2025 18:05:03 +0100 Subject: [PATCH 04/18] fix: lint --- eslint-suppressions.json | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/eslint-suppressions.json b/eslint-suppressions.json index ffe8e50d8bd..77824ab09e5 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -517,7 +517,7 @@ }, "packages/assets-controllers/src/TokenListController.test.ts": { "@typescript-eslint/explicit-function-return-type": { - "count": 2 + "count": 1 }, "id-denylist": { "count": 2 @@ -526,14 +526,6 @@ "count": 7 } }, - "packages/assets-controllers/src/TokenListController.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 6 - }, - "no-restricted-syntax": { - "count": 7 - } - }, "packages/assets-controllers/src/TokenRatesController.test.ts": { "@typescript-eslint/explicit-function-return-type": { "count": 4 From 9c9078b3c3e9593806cdd4e8c82151f989d81772 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 10 Dec 2025 16:39:28 +0100 Subject: [PATCH 05/18] fix: changelog --- packages/assets-controllers/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 9b2ce0d0f8f..927eb4528a8 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `TokenListController` now persists `tokensChainsCache` via `StorageService` instead of controller state to reduce memory usage ([#7413](https://github.com/MetaMask/core/pull/7413)) + - Includes migration logic to automatically move existing cache data on first launch after upgrade + - `tokensChainsCache` state metadata now has `persist: false` to prevent duplicate persistence - Reduce severity of ERC721 metadata interface log from `console.error` to `console.warn` ([#7412](https://github.com/MetaMask/core/pull/7412)) - Fixes [#24988](https://github.com/MetaMask/metamask-extension/issues/24988) - Bump `@metamask/transaction-controller` from `^62.4.0` to `^62.6.0` ([#7325](https://github.com/MetaMask/core/pull/7325), [#7430](https://github.com/MetaMask/core/pull/7430)) From 5c9329a4ecf2696b0b58350c312967779a1cbc27 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 15 Dec 2025 15:18:44 +0100 Subject: [PATCH 06/18] fix: per chain file save --- packages/assets-controllers/CHANGELOG.md | 6 +- .../src/TokenListController.test.ts | 123 ++++++----- .../src/TokenListController.ts | 204 ++++++++++++++---- 3 files changed, 233 insertions(+), 100 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 927eb4528a8..4127f8824ac 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,8 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `TokenListController` now persists `tokensChainsCache` via `StorageService` instead of controller state to reduce memory usage ([#7413](https://github.com/MetaMask/core/pull/7413)) - - Includes migration logic to automatically move existing cache data on first launch after upgrade +- `TokenListController` now persists `tokensChainsCache` via `StorageService` using per-chain files to reduce write amplification ([#7413](https://github.com/MetaMask/core/pull/7413)) + - Each chain's token cache (~100-500KB) is stored in a separate file, avoiding rewriting all chains (~5MB) on every update + - Includes migration logic to automatically split existing cache data into per-chain files on first launch after upgrade + - All chains are loaded in parallel at startup to maintain compatibility with TokenDetectionController - `tokensChainsCache` state metadata now has `persist: false` to prevent duplicate persistence - Reduce severity of ERC721 metadata interface log from `console.error` to `console.warn` ([#7412](https://github.com/MetaMask/core/pull/7412)) - Fixes [#24988](https://github.com/MetaMask/metamask-extension/issues/24988) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 7064bb79975..f5c38177b08 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -22,7 +22,7 @@ import type { TokenListMap, TokenListState, TokenListControllerMessenger, - TokensChainsCache, + DataCache, } from './TokenListController'; import { TokenListController } from './TokenListController'; import { advanceTime } from '../../../tests/helpers'; @@ -514,6 +514,20 @@ const getMessenger = (): RootMessenger => { }, ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:getAllKeys', + (controllerNamespace: string) => { + const keys: string[] = []; + mockStorage.forEach((_value, key) => { + // Extract key without namespace prefix + const keyWithoutNamespace = key.replace(`${controllerNamespace}:`, ''); + keys.push(keyWithoutNamespace); + }); + return keys; + }, + ); + return messenger; }; @@ -536,6 +550,7 @@ const getRestrictedMessenger = ( 'StorageService:getItem', 'StorageService:setItem', 'StorageService:removeItem', + 'StorageService:getAllKeys', ], events: ['NetworkController:stateChange'], }); @@ -1422,24 +1437,22 @@ describe('TokenListController', () => { state: oldPersistedState, }); - // Fetch tokens to trigger save to storage (migration happens asynchronously in constructor) - nock(tokenService.TOKEN_END_POINT_API) - .get(getTokensPath(ChainId.mainnet)) - .reply(200, sampleMainnetTokenList); - - await controller.fetchTokenList(ChainId.mainnet); + // Wait for async migration to complete + await new Promise((resolve) => setTimeout(resolve, 100)); - // Verify data was saved to StorageService + // Verify data was migrated to StorageService (per-chain file) + const chainStorageKey = `tokensChainsCache:${ChainId.mainnet}`; const { result } = await messenger.call( 'StorageService:getItem', 'TokenListController', - 'tokensChainsCache', + chainStorageKey, ); expect(result).toBeDefined(); - const resultCache = result as TokensChainsCache; - expect(resultCache[ChainId.mainnet]).toBeDefined(); - expect(resultCache[ChainId.mainnet].data).toBeDefined(); + const resultCache = result as DataCache; + expect(resultCache.data).toBeDefined(); + expect(resultCache.timestamp).toBeDefined(); + expect(resultCache.data).toStrictEqual(sampleMainnetTokensChainsCache); controller.destroy(); }); @@ -1448,18 +1461,17 @@ describe('TokenListController', () => { const messenger = getMessenger(); const restrictedMessenger = getRestrictedMessenger(messenger); - // Pre-populate StorageService with existing data - const existingStorageData = { - [ChainId.mainnet]: { - data: { '0xExistingToken': { name: 'Existing', symbol: 'EXT' } }, - timestamp: Date.now(), - }, + // Pre-populate StorageService with existing data (per-chain file) + const existingChainData: DataCache = { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), }; + const chainStorageKey = `tokensChainsCache:${ChainId.mainnet}`; await messenger.call( 'StorageService:setItem', 'TokenListController', - 'tokensChainsCache', - existingStorageData, + chainStorageKey, + existingChainData, ); // Initialize with different state data @@ -1486,14 +1498,12 @@ describe('TokenListController', () => { const { result } = await messenger.call( 'StorageService:getItem', 'TokenListController', - 'tokensChainsCache', + chainStorageKey, ); - expect(result).toStrictEqual(existingStorageData); - const resultCache = result as TokensChainsCache; - expect(resultCache[ChainId.mainnet].data).toStrictEqual( - existingStorageData[ChainId.mainnet].data, - ); + expect(result).toStrictEqual(existingChainData); + const resultCache = result as DataCache; + expect(resultCache.data).toStrictEqual(existingChainData.data); controller.destroy(); }); @@ -1511,14 +1521,16 @@ describe('TokenListController', () => { // Wait for migration logic to run await new Promise((resolve) => setTimeout(resolve, 100)); - // Verify nothing was saved to StorageService - const { result } = await messenger.call( - 'StorageService:getItem', + // Verify nothing was saved to StorageService (check no per-chain files) + const allKeys = await messenger.call( + 'StorageService:getAllKeys', 'TokenListController', - 'tokensChainsCache', + ); + const cacheKeys = allKeys.filter((key) => + key.startsWith('tokensChainsCache:'), ); - expect(result).toBeUndefined(); + expect(cacheKeys).toHaveLength(0); controller.destroy(); }); @@ -1542,15 +1554,16 @@ describe('TokenListController', () => { controller1.destroy(); - // Verify data is in StorageService + // Verify data is in StorageService (per-chain file) + const chainStorageKey = `tokensChainsCache:${ChainId.mainnet}`; const { result } = await messenger.call( 'StorageService:getItem', 'TokenListController', - 'tokensChainsCache', + chainStorageKey, ); expect(result).toBeDefined(); - expect(result).toStrictEqual(savedCache); + expect(result).toStrictEqual(savedCache[ChainId.mainnet]); }); it('should save tokensChainsCache to StorageService when fetching tokens', async () => { @@ -1568,17 +1581,18 @@ describe('TokenListController', () => { await controller.fetchTokenList(ChainId.mainnet); - // Verify data was saved to StorageService (fetchTokenList awaits the save) + // Verify data was saved to StorageService (per-chain file) + const chainStorageKey = `tokensChainsCache:${ChainId.mainnet}`; const { result } = await messenger.call( 'StorageService:getItem', 'TokenListController', - 'tokensChainsCache', + chainStorageKey, ); expect(result).toBeDefined(); - const resultCache = result as TokensChainsCache; - expect(resultCache[ChainId.mainnet]).toBeDefined(); - expect(resultCache[ChainId.mainnet].data).toBeDefined(); + const resultCache = result as DataCache; + expect(resultCache.data).toBeDefined(); + expect(resultCache.timestamp).toBeDefined(); controller.destroy(); }); @@ -1587,25 +1601,26 @@ describe('TokenListController', () => { const messenger = getMessenger(); const restrictedMessenger = getRestrictedMessenger(messenger); - // Pre-populate StorageService - const storageData = { - [ChainId.mainnet]: { - data: sampleMainnetTokensChainsCache, - timestamp: Date.now(), - }, + // Pre-populate StorageService (per-chain file) + const chainData: DataCache = { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), }; + const chainStorageKey = `tokensChainsCache:${ChainId.mainnet}`; await messenger.call( 'StorageService:setItem', 'TokenListController', - 'tokensChainsCache', - storageData, + chainStorageKey, + chainData, ); const controller = new TokenListController({ chainId: ChainId.mainnet, messenger: restrictedMessenger, state: { - tokensChainsCache: storageData, + tokensChainsCache: { + [ChainId.mainnet]: chainData, + }, preventPollingOnNetworkRestart: false, }, }); @@ -1615,14 +1630,16 @@ describe('TokenListController', () => { await controller.clearingTokenListData(); - // Verify data was removed from StorageService (clearingTokenListData awaits the removal) - const { result } = await messenger.call( - 'StorageService:getItem', + // Verify data was removed from StorageService (per-chain file removed) + const allKeys = await messenger.call( + 'StorageService:getAllKeys', 'TokenListController', - 'tokensChainsCache', + ); + const cacheKeys = allKeys.filter((key) => + key.startsWith('tokensChainsCache:'), ); - expect(result).toBeUndefined(); + expect(cacheKeys).toHaveLength(0); expect(controller.state.tokensChainsCache).toStrictEqual({}); controller.destroy(); diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index c4172b30cbb..8d43ef3ebe3 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -15,6 +15,7 @@ import type { StorageServiceSetItemAction, StorageServiceGetItemAction, StorageServiceRemoveItemAction, + StorageServiceGetAllKeysAction, } from '@metamask/storage-service'; import type { Hex } from '@metamask/utils'; import { Mutex } from 'async-mutex'; @@ -43,7 +44,7 @@ export type TokenListToken = { export type TokenListMap = Record; -type DataCache = { +export type DataCache = { timestamp: number; data: TokenListMap; }; @@ -74,7 +75,8 @@ type AllowedActions = | NetworkControllerGetNetworkClientByIdAction | StorageServiceSetItemAction | StorageServiceGetItemAction - | StorageServiceRemoveItemAction; + | StorageServiceRemoveItemAction + | StorageServiceGetAllKeysAction; type AllowedEvents = NetworkControllerStateChangeEvent; @@ -121,8 +123,18 @@ export class TokenListController extends StaticIntervalPollingController { readonly #mutex = new Mutex(); - // Storage key for StorageService - static readonly #storageKey = 'tokensChainsCache'; + // Storage key prefix for per-chain files + static readonly #storageKeyPrefix = 'tokensChainsCache'; + + /** + * Get storage key for a specific chain. + * + * @param chainId - The chain ID. + * @returns Storage key for the chain. + */ + static #getChainStorageKey(chainId: Hex): string { + return `${TokenListController.#storageKeyPrefix}:${chainId}`; + } #intervalId?: ReturnType; @@ -212,30 +224,64 @@ export class TokenListController extends StaticIntervalPollingController { try { - const { result, error } = await this.messenger.call( - 'StorageService:getItem', + // Get all keys for this controller + const allKeys = await this.messenger.call( + 'StorageService:getAllKeys', name, - TokenListController.#storageKey, ); - if (error) { - console.error( - 'TokenListController: Error loading cache from storage:', - error, - ); - return; + // Filter keys that belong to tokensChainsCache (per-chain files) + const cacheKeys = allKeys.filter((key) => + key.startsWith(`${TokenListController.#storageKeyPrefix}:`), + ); + + if (cacheKeys.length === 0) { + return; // No cached data } - if (result) { - // Load from StorageService into state + // Load all chains in parallel + const chainCaches = await Promise.all( + cacheKeys.map(async (key) => { + // Extract chainId from key: 'tokensChainsCache:0x1' → '0x1' + const chainId = key.split(':')[1] as Hex; + + const { result, error } = await this.messenger.call( + 'StorageService:getItem', + name, + key, + ); + + if (error) { + console.error( + `TokenListController: Error loading cache for ${chainId}:`, + error, + ); + return null; + } + + return result ? { chainId, data: result as DataCache } : null; + }), + ); + + // Build complete cache from loaded chains + const loadedCache: TokensChainsCache = {}; + chainCaches.forEach((chainCache) => { + if (chainCache) { + loadedCache[chainCache.chainId] = chainCache.data; + } + }); + + // Load into state (all chains available for TokenDetectionController) + if (Object.keys(loadedCache).length > 0) { this.update((state) => { - state.tokensChainsCache = result as TokensChainsCache; + state.tokensChainsCache = loadedCache; }); } } catch (error) { @@ -247,56 +293,107 @@ export class TokenListController extends StaticIntervalPollingController { + async #saveChainCacheToStorage(chainId: Hex): Promise { try { + const chainData = this.state.tokensChainsCache[chainId]; + + if (!chainData) { + console.warn(`TokenListController: No cache data for chain ${chainId}`); + return; + } + + const storageKey = TokenListController.#getChainStorageKey(chainId); + await this.messenger.call( 'StorageService:setItem', name, - TokenListController.#storageKey, - this.state.tokensChainsCache, + storageKey, + chainData, ); } catch (error) { console.error( - 'TokenListController: Failed to save cache to storage:', + `TokenListController: Failed to save cache for ${chainId}:`, error, ); } } /** - * Migrate tokensChainsCache from old persisted state to StorageService. - * This handles backward compatibility for users upgrading from versions - * where tokensChainsCache was persisted in state. + * Migrate tokensChainsCache from old storage formats to per-chain files. + * Handles backward compatibility for users upgrading from: + * 1. Old persisted state (tokensChainsCache was in state) + * 2. Old single-file StorageService (all chains in one file) * * @returns A promise that resolves when migration is complete. */ async #migrateStateToStorage(): Promise { try { - // Check if state has data (from old persisted state) - const hasStateData = + let dataToMigrate: TokensChainsCache | null = null; + + // Check for old single-file storage (previous StorageService version) + const { result: oldStorageData } = await this.messenger.call( + 'StorageService:getItem', + name, + TokenListController.#storageKeyPrefix, // Old key without chain suffix + ); + + if (oldStorageData) { + // Migrate from old single-file storage + dataToMigrate = oldStorageData as TokensChainsCache; + console.log( + 'TokenListController: Migrating from single-file to per-chain storage', + ); + } else if ( this.state.tokensChainsCache && - Object.keys(this.state.tokensChainsCache).length > 0; + Object.keys(this.state.tokensChainsCache).length > 0 + ) { + // Check if per-chain files already exist + const allKeys = await this.messenger.call( + 'StorageService:getAllKeys', + name, + ); + const hasPerChainFiles = allKeys.some((key) => + key.startsWith(`${TokenListController.#storageKeyPrefix}:`), + ); - if (!hasStateData) { - return; + if (!hasPerChainFiles) { + // Migrate from old persisted state + dataToMigrate = this.state.tokensChainsCache; + console.log( + 'TokenListController: Migrating from persisted state to per-chain storage', + ); + } } - // Check if StorageService already has data - const { result } = await this.messenger.call( - 'StorageService:getItem', - name, - TokenListController.#storageKey, + if (!dataToMigrate) { + return; // Nothing to migrate + } + + // Split into per-chain files + await Promise.all( + Object.keys(dataToMigrate).map((chainId) => + this.#saveChainCacheToStorage(chainId as Hex), + ), ); - // If StorageService is empty but state has data, migrate it - if (!result) { - await this.#saveCacheToStorage(); + // Remove old single-file storage if it existed + if (oldStorageData) { + await this.messenger.call( + 'StorageService:removeItem', + name, + TokenListController.#storageKeyPrefix, + ); } + + console.log( + 'TokenListController: Migration to per-chain storage complete', + ); } catch (error) { console.error( 'TokenListController: Failed to migrate cache to storage:', @@ -464,8 +561,8 @@ export class TokenListController extends StaticIntervalPollingController { // Clear state @@ -504,12 +601,29 @@ export class TokenListController extends StaticIntervalPollingController + key.startsWith(`${TokenListController.#storageKeyPrefix}:`), + ); + + await Promise.all( + cacheKeys.map((key) => + this.messenger.call('StorageService:removeItem', name, key), + ), + ); + + // Also remove old single-file storage if it exists (cleanup) await this.messenger.call( 'StorageService:removeItem', name, - TokenListController.#storageKey, + TokenListController.#storageKeyPrefix, ); } catch (error) { console.error( From 0c0e10167b788c0a75cb640635e40e24a83c94ff Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 15 Dec 2025 18:08:12 +0100 Subject: [PATCH 07/18] fix: update @metamask/storage-service version in package.json and yarn.lock --- packages/assets-controllers/package.json | 2 +- yarn.lock | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index 7a54579f9a4..edebf771eee 100644 --- a/packages/assets-controllers/package.json +++ b/packages/assets-controllers/package.json @@ -78,8 +78,8 @@ "@metamask/snaps-controllers": "^14.0.1", "@metamask/snaps-sdk": "^9.0.0", "@metamask/snaps-utils": "^11.0.0", + "@metamask/storage-service": "^0.0.1", "@metamask/transaction-controller": "^62.7.0", - "@metamask/storage-service": "^0.0.0", "@metamask/utils": "^11.8.1", "@types/bn.js": "^5.1.5", "@types/uuid": "^8.3.0", diff --git a/yarn.lock b/yarn.lock index 48b261fb7bb..9101faaadef 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2662,6 +2662,7 @@ __metadata: "@metamask/snaps-controllers": "npm:^14.0.1" "@metamask/snaps-sdk": "npm:^9.0.0" "@metamask/snaps-utils": "npm:^11.0.0" + "@metamask/storage-service": "npm:^0.0.1" "@metamask/transaction-controller": "npm:^62.7.0" "@metamask/utils": "npm:^11.8.1" "@ts-bridge/cli": "npm:^0.6.4" @@ -4941,7 +4942,7 @@ __metadata: languageName: node linkType: hard -"@metamask/storage-service@npm:^0.0.0, @metamask/storage-service@workspace:packages/storage-service": +"@metamask/storage-service@npm:^0.0.1, @metamask/storage-service@workspace:packages/storage-service": version: 0.0.0-use.local resolution: "@metamask/storage-service@workspace:packages/storage-service" dependencies: From c8b0472c50e0e7bec533bedb5cc30409276a4290 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 15 Dec 2025 19:21:28 +0100 Subject: [PATCH 08/18] test: enhance TokenListController tests for error handling in cache loading and saving --- .../src/TokenListController.test.ts | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index f5c38177b08..fa499133d1b 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -1644,6 +1644,197 @@ describe('TokenListController', () => { controller.destroy(); }); + + it('should handle errors when loading individual chain cache files', async () => { + // Pre-populate storage with two chains + const validChainData: DataCache = { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }; + const binanceChainData: DataCache = { + data: sampleBinanceTokensChainsCache, + timestamp: Date.now(), + }; + + mockStorage.set( + `TokenListController:tokensChainsCache:${ChainId.mainnet}`, + validChainData, + ); + mockStorage.set( + `TokenListController:tokensChainsCache:${ChainId.goerli}`, + binanceChainData, + ); + + // Create messenger with getItem that returns error for goerli + const messengerWithErrors = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Register getItem handler that returns error for goerli + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getItem', + (controllerNamespace: string, key: string) => { + if (key === `tokensChainsCache:${ChainId.goerli}`) { + return { error: 'Failed to load chain data' }; + } + const storageKey = `${controllerNamespace}:${key}`; + const value = mockStorage.get(storageKey); + return value ? { result: value } : {}; + }, + ); + + // Register other handlers normally + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:setItem', + (controllerNamespace: string, key: string, value: unknown) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.set(storageKey, value); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:removeItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.delete(storageKey); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getAllKeys', + (controllerNamespace: string) => { + const keys: string[] = []; + mockStorage.forEach((_value, key) => { + const keyWithoutNamespace = key.replace( + `${controllerNamespace}:`, + '', + ); + keys.push(keyWithoutNamespace); + }); + return keys; + }, + ); + + const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); + + // Mock console.error to verify it's called for the error case + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + // Wait for async loading to complete + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify that mainnet chain loaded successfully + expect(controller.state.tokensChainsCache[ChainId.mainnet]).toBeDefined(); + expect( + controller.state.tokensChainsCache[ChainId.mainnet].data, + ).toStrictEqual(sampleMainnetTokensChainsCache); + + // Verify that goerli chain is not in the cache (due to error) + expect( + controller.state.tokensChainsCache[ChainId.goerli], + ).toBeUndefined(); + + // Verify console.error was called with the error + expect(consoleErrorSpy).toHaveBeenCalledWith( + `TokenListController: Error loading cache for ${ChainId.goerli}:`, + 'Failed to load chain data', + ); + + consoleErrorSpy.mockRestore(); + controller.destroy(); + }); + + it('should handle StorageService errors when saving cache', async () => { + // Create a messenger with setItem that throws errors + const messengerWithErrors = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Register all handlers, but make setItem throw + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + const value = mockStorage.get(storageKey); + return value ? { result: value } : {}; + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:setItem', + () => { + throw new Error('Storage write failed'); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:removeItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.delete(storageKey); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getAllKeys', + (controllerNamespace: string) => { + const keys: string[] = []; + mockStorage.forEach((_value, key) => { + const keyWithoutNamespace = key.replace( + `${controllerNamespace}:`, + '', + ); + keys.push(keyWithoutNamespace); + }); + return keys; + }, + ); + + const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); + + // Mock console.error to verify it's called for save errors + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + // Wait for async initialization + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Try to fetch tokens - this should trigger save which will fail + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList); + + await controller.fetchTokenList(ChainId.mainnet); + + // Verify console.error was called with the save error + expect(consoleErrorSpy).toHaveBeenCalledWith( + `TokenListController: Failed to save cache for ${ChainId.mainnet}:`, + expect.any(Error), + ); + + // Verify state was still updated even though save failed + expect(controller.state.tokensChainsCache[ChainId.mainnet]).toBeDefined(); + + consoleErrorSpy.mockRestore(); + controller.destroy(); + }); }); }); From 5490244cda65a19846347af37fc1f0c0d7638a23 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 15 Dec 2025 19:28:34 +0100 Subject: [PATCH 09/18] test: add test --- .../src/TokenListController.test.ts | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index fa499133d1b..130defb2fe8 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -1835,6 +1835,85 @@ describe('TokenListController', () => { consoleErrorSpy.mockRestore(); controller.destroy(); }); + + it('should handle errors during migration to StorageService', async () => { + // Create messenger where getAllKeys throws to cause migration logic to fail + const messengerWithErrors = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Register getItem to return empty (no old storage data) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getItem', + () => { + return {}; // No old single-file storage + }, + ); + + // Register setItem normally + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:setItem', + (controllerNamespace: string, key: string, value: unknown) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.set(storageKey, value); + }, + ); + + // Register getAllKeys to throw error during migration check + // This will cause the migration logic itself to fail (not just the save) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getAllKeys', + () => { + throw new Error('Failed to get keys during migration'); + }, + ); + + // Register removeItem (not used in this test but required) + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:removeItem', + () => { + // Do nothing + }, + ); + + const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); + + // Mock console.error to verify it's called for migration errors + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + // Initialize with state data that will trigger migration + const stateWithData = { + tokensChainsCache: { + [ChainId.mainnet]: { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }, + }, + preventPollingOnNetworkRestart: false, + }; + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + state: stateWithData, + }); + + // Wait for async migration to attempt and fail + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify console.error was called with the migration error + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'TokenListController: Failed to migrate cache to storage:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); + controller.destroy(); + }); }); }); From eec88692cbf3407d657badc6020eb435c8dc16ab Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 15 Dec 2025 20:46:09 +0100 Subject: [PATCH 10/18] test: fix test --- .../src/TokenListController.test.ts | 199 +++++++++++++++++- .../src/TokenListController.ts | 71 ++----- 2 files changed, 219 insertions(+), 51 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 130defb2fe8..e187a1dcd92 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -519,10 +519,13 @@ const getMessenger = (): RootMessenger => { 'StorageService:getAllKeys', (controllerNamespace: string) => { const keys: string[] = []; + const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { - // Extract key without namespace prefix - const keyWithoutNamespace = key.replace(`${controllerNamespace}:`, ''); - keys.push(keyWithoutNamespace); + // Only include keys for this namespace + if (key.startsWith(prefix)) { + const keyWithoutNamespace = key.substring(prefix.length); + keys.push(keyWithoutNamespace); + } }); return keys; }, @@ -1195,6 +1198,96 @@ describe('TokenListController', () => { }); }); + it('should handle errors when clearing data on network change', async () => { + // Create messenger where getAllKeys throws during network change + const messenger = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Register getAllKeys to throw error + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:getAllKeys', + () => { + throw new Error('Failed to get keys during network change'); + }, + ); + + // Register other handlers + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:getItem', + () => ({}), + ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:setItem', + (_controllerNamespace: string, _key: string, _value: unknown) => { + // Do nothing - testing error path + }, + ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'StorageService:removeItem', + (_controllerNamespace: string, _key: string) => { + // Do nothing - testing error path + }, + ); + + const getNetworkClientById = buildMockGetNetworkClientById({ + [InfuraNetworkType.mainnet]: buildInfuraNetworkClientConfiguration( + InfuraNetworkType.mainnet, + ), + [InfuraNetworkType.sepolia]: buildInfuraNetworkClientConfiguration( + InfuraNetworkType.sepolia, + ), + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).registerActionHandler( + 'NetworkController:getNetworkClientById', + getNetworkClientById, + ); + + const restrictedMessenger = getRestrictedMessenger(messenger); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + preventPollingOnNetworkRestart: true, + messenger: restrictedMessenger, + }); + + // Wait for initialization + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Mock console.error to verify error handling + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + // Trigger network change (should try to clear data and catch error) + // Using type assertion since we're testing with a minimal messenger + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messenger as any).publish( + 'NetworkController:stateChange', + { + selectedNetworkClientId: InfuraNetworkType.sepolia, + networkConfigurationsByChainId: {}, + networksMetadata: {}, + }, + [], + ); + + // Wait for async error handling + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Verify error was logged from clearingTokenListData's internal error handling + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'TokenListController: Failed to clear cache from storage:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); + controller.destroy(); + }); + describe('startPolling', () => { let clock: sinon.SinonFakeTimers; const pollingIntervalTime = 1000; @@ -1914,6 +2007,106 @@ describe('TokenListController', () => { consoleErrorSpy.mockRestore(); controller.destroy(); }); + + it('should handle errors when clearing cache from StorageService', async () => { + // Create messenger where getAllKeys throws + const messengerWithErrors = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Register getAllKeys to throw error + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getAllKeys', + () => { + throw new Error('Failed to get keys'); + }, + ); + + // Register other handlers + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:getItem', + () => ({}), + ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:setItem', + (_controllerNamespace: string, _key: string, _value: unknown) => { + // Do nothing - testing error path in getAllKeys + }, + ); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (messengerWithErrors as any).registerActionHandler( + 'StorageService:removeItem', + (_controllerNamespace: string, _key: string) => { + // Do nothing - testing error path in getAllKeys + }, + ); + + const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + // Wait for initialization + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Mock console.error + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + // Try to clear - should catch error + await controller.clearingTokenListData(); + + // Verify error was logged + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'TokenListController: Failed to clear cache from storage:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); + controller.destroy(); + }); + }); + + describe('deprecated methods', () => { + it('should restart polling when restart() is called', async () => { + const messenger = getMessenger(); + const restrictedMessenger = getRestrictedMessenger(messenger); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + interval: 100, + }); + + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList) + .persist(); + + // Start initial polling + await controller.start(); + + // Wait for first fetch + await new Promise((resolve) => setTimeout(resolve, 150)); + + const initialCache = { ...controller.state.tokensChainsCache }; + expect(initialCache[ChainId.mainnet]).toBeDefined(); + + // Restart polling + await controller.restart(); + + // Wait for another fetch + await new Promise((resolve) => setTimeout(resolve, 150)); + + // Verify polling continued + expect(controller.state.tokensChainsCache[ChainId.mainnet]).toBeDefined(); + + controller.destroy(); + }); }); }); diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 8d43ef3ebe3..1104ffde217 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -325,72 +325,47 @@ export class TokenListController extends StaticIntervalPollingController { try { - let dataToMigrate: TokensChainsCache | null = null; + // Check if we have data in state that needs migration + if ( + !this.state.tokensChainsCache || + Object.keys(this.state.tokensChainsCache).length === 0 + ) { + return; // No data to migrate + } - // Check for old single-file storage (previous StorageService version) - const { result: oldStorageData } = await this.messenger.call( - 'StorageService:getItem', + // Check if per-chain files already exist (migration already done) + const allKeys = await this.messenger.call( + 'StorageService:getAllKeys', name, - TokenListController.#storageKeyPrefix, // Old key without chain suffix + ); + const hasPerChainFiles = allKeys.some((key) => + key.startsWith(`${TokenListController.#storageKeyPrefix}:`), ); - if (oldStorageData) { - // Migrate from old single-file storage - dataToMigrate = oldStorageData as TokensChainsCache; - console.log( - 'TokenListController: Migrating from single-file to per-chain storage', - ); - } else if ( - this.state.tokensChainsCache && - Object.keys(this.state.tokensChainsCache).length > 0 - ) { - // Check if per-chain files already exist - const allKeys = await this.messenger.call( - 'StorageService:getAllKeys', - name, - ); - const hasPerChainFiles = allKeys.some((key) => - key.startsWith(`${TokenListController.#storageKeyPrefix}:`), - ); - - if (!hasPerChainFiles) { - // Migrate from old persisted state - dataToMigrate = this.state.tokensChainsCache; - console.log( - 'TokenListController: Migrating from persisted state to per-chain storage', - ); - } + if (hasPerChainFiles) { + return; // Already migrated } - if (!dataToMigrate) { - return; // Nothing to migrate - } + // Migrate from old persisted state to per-chain files + console.log( + 'TokenListController: Migrating from persisted state to per-chain storage', + ); // Split into per-chain files await Promise.all( - Object.keys(dataToMigrate).map((chainId) => + Object.keys(this.state.tokensChainsCache).map((chainId) => this.#saveChainCacheToStorage(chainId as Hex), ), ); - // Remove old single-file storage if it existed - if (oldStorageData) { - await this.messenger.call( - 'StorageService:removeItem', - name, - TokenListController.#storageKeyPrefix, - ); - } - console.log( 'TokenListController: Migration to per-chain storage complete', ); From 9332b6eff5be59516727377cf79a69b666c0c28f Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 10:32:52 +0100 Subject: [PATCH 11/18] test: add test to ensure cache is loaded only once during multiple fetchTokenList calls --- .../src/TokenListController.test.ts | 101 ++++++++++++++++++ .../src/TokenListController.ts | 71 ++++++++---- 2 files changed, 151 insertions(+), 21 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index e187a1dcd92..1a10930fd66 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -2069,6 +2069,107 @@ describe('TokenListController', () => { consoleErrorSpy.mockRestore(); controller.destroy(); }); + + it('should only load cache from storage once even when fetchTokenList is called multiple times', async () => { + // Pre-populate storage with cached data + const chainData: DataCache = { + data: sampleMainnetTokensChainsCache, + timestamp: Date.now(), + }; + mockStorage.set( + `TokenListController:tokensChainsCache:${ChainId.mainnet}`, + chainData, + ); + + // Track how many times getItem is called + let getItemCallCount = 0; + let getAllKeysCallCount = 0; + + const trackingMessenger = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (trackingMessenger as any).registerActionHandler( + 'StorageService:getItem', + (controllerNamespace: string, key: string) => { + getItemCallCount += 1; + const storageKey = `${controllerNamespace}:${key}`; + const value = mockStorage.get(storageKey); + return value ? { result: value } : {}; + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (trackingMessenger as any).registerActionHandler( + 'StorageService:setItem', + (controllerNamespace: string, key: string, value: unknown) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.set(storageKey, value); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (trackingMessenger as any).registerActionHandler( + 'StorageService:removeItem', + (controllerNamespace: string, key: string) => { + const storageKey = `${controllerNamespace}:${key}`; + mockStorage.delete(storageKey); + }, + ); + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (trackingMessenger as any).registerActionHandler( + 'StorageService:getAllKeys', + (controllerNamespace: string) => { + getAllKeysCallCount += 1; + const keys: string[] = []; + const prefix = `${controllerNamespace}:`; + mockStorage.forEach((_value, key) => { + if (key.startsWith(prefix)) { + const keyWithoutNamespace = key.substring(prefix.length); + keys.push(keyWithoutNamespace); + } + }); + return keys; + }, + ); + + const restrictedMessenger = getRestrictedMessenger(trackingMessenger); + + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + // Wait for initialization to complete + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Record call counts after initialization + const getItemCallsAfterInit = getItemCallCount; + const getAllKeysCallsAfterInit = getAllKeysCallCount; + + // getAllKeys should be called twice during init (once for load, once for migration check) + expect(getAllKeysCallsAfterInit).toBe(2); + // getItem should be called once for the cached chain during load + expect(getItemCallsAfterInit).toBe(1); + + // Now call fetchTokenList multiple times + nock(tokenService.TOKEN_END_POINT_API) + .get(getTokensPath(ChainId.mainnet)) + .reply(200, sampleMainnetTokenList) + .persist(); + + await controller.fetchTokenList(ChainId.mainnet); + await controller.fetchTokenList(ChainId.mainnet); + await controller.fetchTokenList(ChainId.mainnet); + + // Verify getAllKeys was NOT called again after initialization + // (getItem may be called for other reasons, but getAllKeys is only used in load/migrate) + expect(getAllKeysCallCount).toBe(getAllKeysCallsAfterInit); + + controller.destroy(); + }); }); describe('deprecated methods', () => { diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 1104ffde217..3ecd5112c0a 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -123,6 +123,12 @@ export class TokenListController extends StaticIntervalPollingController { readonly #mutex = new Mutex(); + /** + * Promise that resolves when initialization (loading cache from storage) is complete. + * Methods that access the cache should await this before proceeding. + */ + readonly #initializationPromise: Promise; + // Storage key prefix for per-chain files static readonly #storageKeyPrefix = 'tokensChainsCache'; @@ -191,18 +197,9 @@ export class TokenListController extends StaticIntervalPollingController { - // Migrate existing cache from state to StorageService if needed - return this.#migrateStateToStorage(); - }) - .catch((error) => { - console.error( - 'TokenListController: Failed to load cache from storage:', - error, - ); - }); + // Load cache from StorageService on initialization and handle migration. + // Store the promise so other methods can await it to avoid race conditions. + this.#initializationPromise = this.#initializeFromStorage(); if (onNetworkStateChange) { // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -222,11 +219,36 @@ export class TokenListController extends StaticIntervalPollingController { + const releaseLock = await this.#mutex.acquire(); + try { + await this.#loadCacheFromStorage(); + await this.#migrateStateToStorage(); + } catch (error) { + console.error( + 'TokenListController: Failed to initialize from storage:', + error, + ); + } finally { + releaseLock(); + } + } + /** * Load tokensChainsCache from StorageService into state. * Loads all cached chains from separate per-chain files in parallel. * Called during initialization to restore cached data. * + * Note: This method merges loaded data with existing state to avoid + * overwriting any fresh data that may have been fetched concurrently. + * Caller must hold the mutex. + * * @returns A promise that resolves when loading is complete. */ async #loadCacheFromStorage(): Promise { @@ -278,10 +300,17 @@ export class TokenListController extends StaticIntervalPollingController 0) { this.update((state) => { - state.tokensChainsCache = loadedCache; + // Only load chains that don't already exist in state + // This prevents overwriting fresh API data with stale cached data + for (const [chainId, cacheData] of Object.entries(loadedCache)) { + if (!state.tokensChainsCache[chainId as Hex]) { + state.tokensChainsCache[chainId as Hex] = cacheData; + } + } }); } } catch (error) { @@ -497,6 +526,10 @@ export class TokenListController extends StaticIntervalPollingController { + // Wait for initialization to complete before fetching + // This ensures we have loaded any cached data from storage first + await this.#initializationPromise; + const releaseLock = await this.#mutex.acquire(); try { if (this.isCacheValid(chainId)) { @@ -571,6 +604,9 @@ export class TokenListController extends StaticIntervalPollingController { + // Wait for initialization to complete before clearing + await this.#initializationPromise; + // Clear state this.update((state) => { state.tokensChainsCache = {}; @@ -593,13 +629,6 @@ export class TokenListController extends StaticIntervalPollingController Date: Thu, 18 Dec 2025 11:18:30 +0100 Subject: [PATCH 12/18] fix: fix race condition on clearingTokenListData --- .../src/TokenListController.ts | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 3ecd5112c0a..f97da82b8b4 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -602,18 +602,27 @@ export class TokenListController extends StaticIntervalPollingController { // Wait for initialization to complete before clearing await this.#initializationPromise; - // Clear state - this.update((state) => { - state.tokensChainsCache = {}; - }); - - // Clear all per-chain files from StorageService + const releaseLock = await this.#mutex.acquire(); try { + // Clear state + this.update((state) => { + state.tokensChainsCache = {}; + }); + + // Clear all per-chain files from StorageService const allKeys = await this.messenger.call( 'StorageService:getAllKeys', name, @@ -634,6 +643,8 @@ export class TokenListController extends StaticIntervalPollingController Date: Thu, 18 Dec 2025 11:48:14 +0100 Subject: [PATCH 13/18] fix: fix state inconsistency on error --- .../src/TokenListController.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index f97da82b8b4..921570de946 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -603,13 +603,11 @@ export class TokenListController extends StaticIntervalPollingController { // Wait for initialization to complete before clearing @@ -617,12 +615,9 @@ export class TokenListController extends StaticIntervalPollingController { - state.tokensChainsCache = {}; - }); - - // Clear all per-chain files from StorageService + // Clear storage first to maintain consistency. + // If storage clearing fails, state remains unchanged and matches storage. + // This prevents stale data from reappearing after restart. const allKeys = await this.messenger.call( 'StorageService:getAllKeys', name, @@ -638,6 +633,11 @@ export class TokenListController extends StaticIntervalPollingController { + state.tokensChainsCache = {}; + }); } catch (error) { console.error( 'TokenListController: Failed to clear cache from storage:', From e74a52013924d74f74c3c5b890727f8a9e674f97 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 13:31:13 +0100 Subject: [PATCH 14/18] fix: refactor and improve migration log --- .../src/TokenListController.ts | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 921570de946..811ec204f6a 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -358,40 +358,52 @@ export class TokenListController extends StaticIntervalPollingController { try { // Check if we have data in state that needs migration - if ( - !this.state.tokensChainsCache || - Object.keys(this.state.tokensChainsCache).length === 0 - ) { + const chainsInState = Object.keys(this.state.tokensChainsCache) as Hex[]; + if (chainsInState.length === 0) { return; // No data to migrate } - // Check if per-chain files already exist (migration already done) + // Get existing per-chain files to determine which chains still need migration const allKeys = await this.messenger.call( 'StorageService:getAllKeys', name, ); - const hasPerChainFiles = allKeys.some((key) => - key.startsWith(`${TokenListController.#storageKeyPrefix}:`), + const existingChainKeys = new Set( + allKeys.filter((key) => + key.startsWith(`${TokenListController.#storageKeyPrefix}:`), + ), ); - if (hasPerChainFiles) { - return; // Already migrated + // Find chains that exist in state but not in storage (need migration) + const chainsMissingFromStorage = chainsInState.filter((chainId) => { + const storageKey = TokenListController.#getChainStorageKey(chainId); + return !existingChainKeys.has(storageKey); + }); + + if (chainsMissingFromStorage.length === 0) { + return; // All chains already migrated } - // Migrate from old persisted state to per-chain files + // Migrate only the chains that are missing from storage console.log( - 'TokenListController: Migrating from persisted state to per-chain storage', + `TokenListController: Migrating ${chainsMissingFromStorage.length} chain(s) from persisted state to per-chain storage`, ); - // Split into per-chain files + // Migrate chains in parallel. Individual failures are logged inside + // #saveChainCacheToStorage. If any fail, the cache will self-heal + // when fetchTokenList is called for that chain. await Promise.all( - Object.keys(this.state.tokensChainsCache).map((chainId) => - this.#saveChainCacheToStorage(chainId as Hex), + chainsMissingFromStorage.map((chainId) => + this.#saveChainCacheToStorage(chainId), ), ); From 6f637377d4ce90050f255f2c859e761353e8d7d9 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 13:48:20 +0100 Subject: [PATCH 15/18] fix: fix timestamp save --- .../src/TokenListController.test.ts | 22 ++++++++++-------- .../src/TokenListController.ts | 23 +++++++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 1a10930fd66..85ca14a8fca 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -1801,12 +1801,13 @@ describe('TokenListController', () => { 'StorageService:getAllKeys', (controllerNamespace: string) => { const keys: string[] = []; + const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { - const keyWithoutNamespace = key.replace( - `${controllerNamespace}:`, - '', - ); - keys.push(keyWithoutNamespace); + // Only include keys for this namespace + if (key.startsWith(prefix)) { + const keyWithoutNamespace = key.substring(prefix.length); + keys.push(keyWithoutNamespace); + } }); return keys; }, @@ -1885,12 +1886,13 @@ describe('TokenListController', () => { 'StorageService:getAllKeys', (controllerNamespace: string) => { const keys: string[] = []; + const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { - const keyWithoutNamespace = key.replace( - `${controllerNamespace}:`, - '', - ); - keys.push(keyWithoutNamespace); + // Only include keys for this namespace + if (key.startsWith(prefix)) { + const keyWithoutNamespace = key.substring(prefix.length); + keys.push(keyWithoutNamespace); + } }); return keys; }, diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 811ec204f6a..b34c1747c06 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -586,16 +586,21 @@ export class TokenListController extends StaticIntervalPollingController { - state.tokensChainsCache[chainId] ??= newDataCache; - state.tokensChainsCache[chainId].timestamp = Date.now(); - }); - - // Persist only this chain to StorageService - await this.#saveChainCacheToStorage(chainId); + const existingCache = this.state.tokensChainsCache[chainId]; + if (!existingCache) { + // No existing cache - initialize empty and persist + const newDataCache: DataCache = { data: {}, timestamp: Date.now() }; + this.update((state) => { + state.tokensChainsCache[chainId] = newDataCache; + }); + await this.#saveChainCacheToStorage(chainId); + } + // If there's existing cache, keep it as-is (don't update timestamp or persist) } } finally { releaseLock(); From a2c42fbe7b2ec771096d2a5351ba2604fb3433cd Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 14:52:13 +0100 Subject: [PATCH 16/18] fix: use promise.allSettled --- .../src/TokenListController.ts | 51 +++++++++++++++---- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index b34c1747c06..0661d88f76b 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -620,9 +620,10 @@ export class TokenListController extends StaticIntervalPollingController { + state.tokensChainsCache = {}; + }); + return; + } + + // Use Promise.allSettled to handle partial failures gracefully. + // This ensures all removals are attempted and we can track which succeeded. + const results = await Promise.allSettled( cacheKeys.map((key) => this.messenger.call('StorageService:removeItem', name, key), ), ); - // Only clear state after storage is successfully cleared + // Identify which chains failed to be removed from storage + const failedChainIds = new Set(); + results.forEach((result, index) => { + if (result.status === 'rejected') { + const key = cacheKeys[index]; + const chainId = key.split(':')[1] as Hex; + failedChainIds.add(chainId); + console.error( + `TokenListController: Failed to remove cache for chain ${chainId}:`, + result.reason, + ); + } + }); + + // Update state to match storage: keep only chains that failed to be removed this.update((state) => { - state.tokensChainsCache = {}; + if (failedChainIds.size === 0) { + state.tokensChainsCache = {}; + } else { + // Keep only chains that failed to be removed from storage + const preservedCache: TokensChainsCache = {}; + for (const chainId of failedChainIds) { + if (state.tokensChainsCache[chainId]) { + preservedCache[chainId] = state.tokensChainsCache[chainId]; + } + } + state.tokensChainsCache = preservedCache; + } }); } catch (error) { console.error( From d1d8354f05cf58326dc46188749a3bc65044fe6c Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 15:11:11 +0100 Subject: [PATCH 17/18] fix: clear state on storage access error --- packages/assets-controllers/CHANGELOG.md | 2 +- .../src/TokenListController.test.ts | 32 ++++++++++++++++--- .../src/TokenListController.ts | 6 ++++ 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 21338331969..36ffc0848f6 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `TokenListController` now persists `tokensChainsCache` via `StorageService` using per-chain files to reduce write amplification ([#7413](https://github.com/MetaMask/core/pull/7413)) +- **BREAKING:** `TokenListController` now persists `tokensChainsCache` via `StorageService` using per-chain files to reduce write amplification ([#7413](https://github.com/MetaMask/core/pull/7413)) - Each chain's token cache (~100-500KB) is stored in a separate file, avoiding rewriting all chains (~5MB) on every update - Includes migration logic to automatically split existing cache data into per-chain files on first launch after upgrade - All chains are loaded in parallel at startup to maintain compatibility with TokenDetectionController diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index 85ca14a8fca..3cf80032978 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -2011,17 +2011,21 @@ describe('TokenListController', () => { }); it('should handle errors when clearing cache from StorageService', async () => { - // Create messenger where getAllKeys throws + // Create messenger where getAllKeys throws only during clear const messengerWithErrors = new Messenger({ namespace: MOCK_ANY_NAMESPACE, }); - // Register getAllKeys to throw error + let shouldThrow = false; + // Register getAllKeys to throw error only when flag is set // eslint-disable-next-line @typescript-eslint/no-explicit-any (messengerWithErrors as any).registerActionHandler( 'StorageService:getAllKeys', () => { - throw new Error('Failed to get keys'); + if (shouldThrow) { + throw new Error('Failed to get keys'); + } + return []; // Return empty array for initialization }, ); @@ -2048,18 +2052,34 @@ describe('TokenListController', () => { const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); + // Initialize controller with pre-populated state + const initialCache = { + [ChainId.mainnet]: { + timestamp: Date.now(), + data: sampleMainnetTokensChainsCache, + }, + }; const controller = new TokenListController({ chainId: ChainId.mainnet, messenger: restrictedMessenger, + state: { tokensChainsCache: initialCache }, }); // Wait for initialization await new Promise((resolve) => setTimeout(resolve, 100)); + // Verify cache exists before clearing + expect( + Object.keys(controller.state.tokensChainsCache).length, + ).toBeGreaterThan(0); + + // Now enable throwing to test error handling during clear + shouldThrow = true; + // Mock console.error const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); - // Try to clear - should catch error + // Try to clear - should catch error but still clear state await controller.clearingTokenListData(); // Verify error was logged @@ -2068,6 +2088,10 @@ describe('TokenListController', () => { expect.any(Error), ); + // Verify state was still cleared despite the error + // This ensures consistent behavior with the no-keys case + expect(controller.state.tokensChainsCache).toStrictEqual({}); + consoleErrorSpy.mockRestore(); controller.destroy(); }); diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 0661d88f76b..6b18a1f0449 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -693,6 +693,12 @@ export class TokenListController extends StaticIntervalPollingController { + state.tokensChainsCache = {}; + }); } finally { releaseLock(); } From eb78626d65495a65ff9675ac2e32f2a87c7afeac Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 18 Dec 2025 15:19:12 +0100 Subject: [PATCH 18/18] fix: changelog --- packages/assets-controllers/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 36ffc0848f6..0d64d6152e6 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **BREAKING:** `TokenListController` now persists `tokensChainsCache` via `StorageService` using per-chain files to reduce write amplification ([#7413](https://github.com/MetaMask/core/pull/7413)) +- **BREAKING:** `TokenListController` now persists `tokensChainsCache` via `StorageService` using per-chain files to reduce write amplification ([#7413](https://github.com/MetaMask/core/pull/7413)) - Each chain's token cache (~100-500KB) is stored in a separate file, avoiding rewriting all chains (~5MB) on every update - Includes migration logic to automatically split existing cache data into per-chain files on first launch after upgrade - All chains are loaded in parallel at startup to maintain compatibility with TokenDetectionController