diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 8578041df8a..b5872480da7 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -474,7 +474,7 @@ }, "packages/assets-controllers/src/TokenListController.test.ts": { "@typescript-eslint/explicit-function-return-type": { - "count": 2 + "count": 1 }, "id-denylist": { "count": 2 @@ -483,14 +483,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 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)) diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index 847ac8af256..17e0218157a 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.6.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..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(); @@ -1069,7 +1114,7 @@ describe('TokenListController', () => { state: existingState, }); expect(controller.state).toStrictEqual(existingState); - controller.clearingTokenListData(); + await controller.clearingTokenListData(); expect(controller.state.tokensChainsCache).toStrictEqual({}); @@ -1331,7 +1376,6 @@ describe('TokenListController', () => { ).toMatchInlineSnapshot(` Object { "preventPollingOnNetworkRestart": false, - "tokensChainsCache": Object {}, } `); }); @@ -1355,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(); + }); + }); }); /** @@ -1363,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`; 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 9ab015904ab..fc625faa16b 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.6.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: