diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 2c57c22256e..ae9bd334a0e 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -332,14 +332,6 @@ "count": 2 } }, - "packages/assets-controllers/src/TokenListController.test.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 1 - }, - "id-denylist": { - "count": 2 - } - }, "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 f52a7b4f1ee..c7c860f01e7 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - When `isHomepageSectionsV1Enabled` is true, `AccountTrackerController` now uses all popular EVM networks (via `NetworkEnablementController:listPopularEvmNetworks`) for balance refresh on account change and keyring unlock, instead of only the enabled networks from `NetworkEnablementController` state ([#8117](https://github.com/MetaMask/core/pull/8117)) +### Fixed + +- Gate `TokenListController` polling on controller initialization to avoid duplicate token list API requests during startup races ([#8113](https://github.com/MetaMask/core/pull/8113)) + ## [100.1.0] ### Added diff --git a/packages/assets-controllers/src/TokenListController.test.ts b/packages/assets-controllers/src/TokenListController.test.ts index af869322f22..3960656d1f4 100644 --- a/packages/assets-controllers/src/TokenListController.test.ts +++ b/packages/assets-controllers/src/TokenListController.test.ts @@ -13,6 +13,7 @@ import type { MockAnyNamespace, } from '@metamask/messenger'; import type { NetworkState } from '@metamask/network-controller'; +import { StorageGetResult } from '@metamask/storage-service'; import type { Hex } from '@metamask/utils'; import nock from 'nock'; @@ -479,32 +480,39 @@ type RootMessenger = Messenger< const mockStorage = new Map(); const getMessenger = (): RootMessenger => { - const messenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE }); + const messenger: RootMessenger = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); // Register StorageService mock handlers - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messenger as any).registerActionHandler( + messenger.registerActionHandler( 'StorageService:getItem', - (controllerNamespace: string, key: string) => { + async ( + controllerNamespace: string, + key: string, + ): Promise => { const storageKey = `${controllerNamespace}:${key}`; const value = mockStorage.get(storageKey); - return value ? { result: value } : {}; + const result = (value ? { result: value } : {}) as StorageGetResult; + return result; }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messenger as any).registerActionHandler( + messenger.registerActionHandler( 'StorageService:setItem', - (controllerNamespace: string, key: string, value: unknown) => { + async ( + controllerNamespace: string, + key: string, + value: unknown, + ): Promise => { const storageKey = `${controllerNamespace}:${key}`; mockStorage.set(storageKey, value); }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messenger as any).registerActionHandler( + messenger.registerActionHandler( 'StorageService:getAllKeys', - (controllerNamespace: string) => { + async (controllerNamespace: string): Promise => { const keys: string[] = []; const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { @@ -654,7 +662,9 @@ describe('TokenListController', () => { let onNetworkStateChangeCallback!: (state: NetworkState) => void; const controller = new TokenListController({ chainId: ChainId.mainnet, - onNetworkStateChange: (cb) => (onNetworkStateChangeCallback = cb), + onNetworkStateChange: (callback): void => { + onNetworkStateChangeCallback = callback; + }, interval: 100, messenger: restrictedMessenger, }); @@ -1069,6 +1079,84 @@ describe('TokenListController', () => { controller.destroy(); }); + describe('_executePoll', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + const arrange = ({ + initializationDelayMs = 50, + }: { + initializationDelayMs?: number; + } = {}): { + controller: TokenListController; + fetchTokenListByChainIdSpy: jest.SpyInstance; + resolveMockWaitForInit: () => Promise; + } => { + const messenger = getMessenger(); + + // Mock getAllKeys that takes time to resolve + messenger.unregisterActionHandler('StorageService:getAllKeys'); + messenger.registerActionHandler( + 'StorageService:getAllKeys', + () => + new Promise((resolve) => { + setTimeout(() => resolve([]), initializationDelayMs); + }), + ); + + const restrictedMessenger = getRestrictedMessenger(messenger); + const fetchTokenListByChainIdSpy = jest + .spyOn(tokenService, 'fetchTokenListByChainId') + .mockResolvedValue(sampleMainnetTokenList); + const controller = new TokenListController({ + chainId: ChainId.mainnet, + messenger: restrictedMessenger, + }); + + const resolveMockWaitForInit = async (): Promise => { + await jestAdvanceTime({ duration: 50 }); + }; + + return { + controller, + fetchTokenListByChainIdSpy, + resolveMockWaitForInit, + }; + }; + + it('waits for initialize to finish before polling', async () => { + const { controller, fetchTokenListByChainIdSpy, resolveMockWaitForInit } = + arrange(); + + const initializePromise = controller.initialize(); + const pollPromise = controller._executePoll({ + chainId: ChainId.mainnet, + }); + + expect(fetchTokenListByChainIdSpy).not.toHaveBeenCalled(); + + await resolveMockWaitForInit(); + await initializePromise; + await pollPromise; + + expect(fetchTokenListByChainIdSpy).toHaveBeenCalledTimes(1); + }); + + it('polls normally when initialization is not in progress', async () => { + const { controller, fetchTokenListByChainIdSpy } = arrange({ + initializationDelayMs: 0, + }); + + await controller._executePoll({ chainId: ChainId.mainnet }); + expect(fetchTokenListByChainIdSpy).toHaveBeenCalledTimes(1); + }); + }); + describe('startPolling', () => { const pollingIntervalTime = 1000; beforeEach(() => { @@ -1583,38 +1671,45 @@ describe('TokenListController', () => { ); // Create messenger with getItem that returns error for goerli - const messengerWithErrors = new Messenger({ - namespace: MOCK_ANY_NAMESPACE, - }); + const messengerWithErrors = getMessenger(); // Register getItem handler that returns error for goerli - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( + messengerWithErrors.unregisterActionHandler('StorageService:getItem'); + messengerWithErrors.registerActionHandler( 'StorageService:getItem', - (controllerNamespace: string, key: string) => { + async ( + controllerNamespace: string, + key: string, + ): Promise => { if (key === `tokensChainsCache:${ChainId.goerli}`) { - return { error: 'Failed to load chain data' }; + return { + error: 'Failed to load chain data', + } as unknown as StorageGetResult; } const storageKey = `${controllerNamespace}:${key}`; const value = mockStorage.get(storageKey); - return value ? { result: value } : {}; + return (value ? { result: value } : {}) as StorageGetResult; }, ); // Register other handlers normally - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( + messengerWithErrors.unregisterActionHandler('StorageService:setItem'); + messengerWithErrors.registerActionHandler( 'StorageService:setItem', - (controllerNamespace: string, key: string, value: unknown) => { + async ( + controllerNamespace: string, + key: string, + value: unknown, + ): Promise => { const storageKey = `${controllerNamespace}:${key}`; mockStorage.set(storageKey, value); }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( + messengerWithErrors.unregisterActionHandler('StorageService:getAllKeys'); + messengerWithErrors.registerActionHandler( 'StorageService:getAllKeys', - (controllerNamespace: string) => { + async (controllerNamespace: string): Promise => { const keys: string[] = []; const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { @@ -1664,46 +1759,17 @@ describe('TokenListController', () => { 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, - }); + const messengerWithErrors = getMessenger(); // 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( + messengerWithErrors.unregisterActionHandler('StorageService:setItem'); + messengerWithErrors.registerActionHandler( 'StorageService:setItem', () => { throw new Error('Storage write failed'); }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( - 'StorageService:getAllKeys', - (controllerNamespace: string) => { - const keys: string[] = []; - const prefix = `${controllerNamespace}:`; - mockStorage.forEach((_value, key) => { - // Only include keys for this namespace - if (key.startsWith(prefix)) { - const keyWithoutNamespace = key.substring(prefix.length); - keys.push(keyWithoutNamespace); - } - }); - return keys; - }, - ); - const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); // Mock console.error to verify it's called for save errors @@ -1742,35 +1808,17 @@ describe('TokenListController', () => { it('should handle errors during debounced persistence', async () => { // Create messenger where setItem throws to cause persistence to fail - const messengerWithErrors = new Messenger({ - namespace: MOCK_ANY_NAMESPACE, - }); - - // Register getItem to return empty - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( - 'StorageService:getItem', - () => { - return {}; - }, - ); + const messengerWithErrors = getMessenger(); // Register setItem to throw error - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( + messengerWithErrors.unregisterActionHandler('StorageService:setItem'); + messengerWithErrors.registerActionHandler( 'StorageService:setItem', () => { throw new Error('Failed to save to storage'); }, ); - // Register getAllKeys normally - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (messengerWithErrors as any).registerActionHandler( - 'StorageService:getAllKeys', - () => [], - ); - const restrictedMessenger = getRestrictedMessenger(messengerWithErrors); // Mock console.error to verify it's called for persistence errors @@ -1819,34 +1867,26 @@ describe('TokenListController', () => { let getItemCallCount = 0; let getAllKeysCallCount = 0; - const trackingMessenger = new Messenger({ - namespace: MOCK_ANY_NAMESPACE, - }); + const trackingMessenger = getMessenger(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:getItem'); + trackingMessenger.registerActionHandler( 'StorageService:getItem', - (controllerNamespace: string, key: string) => { + async ( + controllerNamespace: string, + key: string, + ): Promise => { 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); + return (value ? { result: value } : {}) as StorageGetResult; }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:getAllKeys'); + trackingMessenger.registerActionHandler( 'StorageService:getAllKeys', - (controllerNamespace: string) => { + async (controllerNamespace: string): Promise => { getAllKeysCallCount += 1; const keys: string[] = []; const prefix = `${controllerNamespace}:`; @@ -1910,34 +1950,26 @@ describe('TokenListController', () => { // Track how many times setItem is called let setItemCallCount = 0; - const trackingMessenger = new Messenger({ - namespace: MOCK_ANY_NAMESPACE, - }); + const trackingMessenger = getMessenger(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger 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 - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:setItem'); + trackingMessenger.registerActionHandler( 'StorageService:setItem', - (controllerNamespace: string, key: string, value: unknown) => { + async ( + controllerNamespace: string, + key: string, + value: unknown, + ): Promise => { setItemCallCount += 1; const storageKey = `${controllerNamespace}:${key}`; mockStorage.set(storageKey, value); }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:getAllKeys'); + trackingMessenger.registerActionHandler( 'StorageService:getAllKeys', - (controllerNamespace: string) => { + async (controllerNamespace: string): Promise => { const keys: string[] = []; const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { @@ -1990,34 +2022,26 @@ describe('TokenListController', () => { // Track setItem calls and which chains are persisted const persistedChains: string[] = []; - 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) => { - const storageKey = `${controllerNamespace}:${key}`; - const value = mockStorage.get(storageKey); - return value ? { result: value } : {}; - }, - ); + const trackingMessenger = getMessenger(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:setItem'); + trackingMessenger.registerActionHandler( 'StorageService:setItem', - (controllerNamespace: string, key: string, value: unknown) => { + async ( + controllerNamespace: string, + key: string, + value: unknown, + ): Promise => { persistedChains.push(key); const storageKey = `${controllerNamespace}:${key}`; mockStorage.set(storageKey, value); }, ); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (trackingMessenger as any).registerActionHandler( + trackingMessenger.unregisterActionHandler('StorageService:getAllKeys'); + trackingMessenger.registerActionHandler( 'StorageService:getAllKeys', - (controllerNamespace: string) => { + async (controllerNamespace: string): Promise => { const keys: string[] = []; const prefix = `${controllerNamespace}:`; mockStorage.forEach((_value, key) => { diff --git a/packages/assets-controllers/src/TokenListController.ts b/packages/assets-controllers/src/TokenListController.ts index 13b387d3245..d0a18ca4a17 100644 --- a/packages/assets-controllers/src/TokenListController.ts +++ b/packages/assets-controllers/src/TokenListController.ts @@ -117,6 +117,11 @@ export class TokenListController extends StaticIntervalPollingController; + /** + * Promise for the in-flight initialization sequence. + */ + #initializePromise?: Promise; + /** * Promise that resolves when the current persist operation completes. * Used to prevent race conditions between persist operations. @@ -228,14 +233,43 @@ export class TokenListController extends StaticIntervalPollingController { - await this.#synchronizeCacheWithStorage(); + if (this.#initializePromise) { + await this.#initializePromise; + return; + } - // Subscribe to state changes to automatically persist tokensChainsCache - this.messenger.subscribe( - 'TokenListController:stateChange', - (newCache: TokensChainsCache) => this.#onCacheChanged(newCache), - (controllerState) => controllerState.tokensChainsCache, - ); + const executeInit = async (): Promise => { + try { + await this.#synchronizeCacheWithStorage(); + + // Subscribe to state changes to automatically persist tokensChainsCache + this.messenger.subscribe( + 'TokenListController:stateChange', + (newCache: TokensChainsCache) => this.#onCacheChanged(newCache), + (controllerState) => controllerState.tokensChainsCache, + ); + } catch { + // do nothing + } finally { + this.#initializePromise = undefined; + } + }; + + this.#initializePromise = executeInit(); + + await this.#initializePromise; + } + + /** + * Waits for any in-flight initialization to complete. + * Polling should not run against partially initialized state. + */ + async #waitForInitialization(): Promise { + try { + await this.#initializePromise; + } catch { + // do nothing + } } /** @@ -559,6 +593,7 @@ export class TokenListController extends StaticIntervalPollingController { + await this.#waitForInitialization(); return this.fetchTokenList(chainId); }