From 2f52ae53470d97810454446143ffd079e4a54728 Mon Sep 17 00:00:00 2001 From: Kriys94 Date: Thu, 18 Jun 2026 14:41:34 +0200 Subject: [PATCH 1/2] fix(assets-controller): Coalesce assets price --- .../assets-controller/src/AssetsController.ts | 25 +- .../src/data-sources/PriceDataSource.test.ts | 229 +++++++++++++++++- .../src/data-sources/PriceDataSource.ts | 86 ++++++- .../src/utils/coalescingBatchFetcher.ts | 218 +++++++++++++++++ 4 files changed, 521 insertions(+), 37 deletions(-) create mode 100644 packages/assets-controller/src/utils/coalescingBatchFetcher.ts diff --git a/packages/assets-controller/src/AssetsController.ts b/packages/assets-controller/src/AssetsController.ts index 35e716ec4b..818658766b 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -1204,26 +1204,12 @@ export class AssetsController extends BaseController< const removedChains = previous.filter((ch) => !activeChains.includes(ch)); if (addedChains.length > 0 || removedChains.length > 0) { - // Refresh subscriptions to use updated data source availability + // Refresh subscriptions to use updated data source availability. + // No one-time fetch needed here — #handleEnabledNetworksChanged + // handles fetches when the user enables a network, and + // #subscribeAssets re-subscribes with the correct chain assignment. this.#subscribeAssets(); } - - // If chains were added and we have selected accounts, do one-time fetch - if (addedChains.length > 0 && this.#getSelectedAccounts().length > 0) { - const addedEnabledChains = addedChains.filter((chain) => - this.#enabledChains.has(chain), - ); - if (addedEnabledChains.length > 0) { - log('Fetching balances for newly added chains', { addedEnabledChains }); - this.getAssets(this.#getSelectedAccounts(), { - chainIds: addedEnabledChains, - forceUpdate: true, - updateMode: 'merge', - }).catch((error) => { - log('Failed to fetch balance for added chains', { error }); - }); - } - } } /** @@ -1829,6 +1815,9 @@ export class AssetsController extends BaseController< return; } + // Currency changed — old cached prices are in the wrong currency. + this.#priceDataSource.invalidatePriceCache(); + this.getAssets(this.#getSelectedAccounts(), { forceUpdate: true, dataTypes: ['price'], diff --git a/packages/assets-controller/src/data-sources/PriceDataSource.test.ts b/packages/assets-controller/src/data-sources/PriceDataSource.test.ts index b87fbdba8b..175ba92785 100644 --- a/packages/assets-controller/src/data-sources/PriceDataSource.test.ts +++ b/packages/assets-controller/src/data-sources/PriceDataSource.test.ts @@ -556,13 +556,12 @@ describe('PriceDataSource', () => { expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); - jest.advanceTimersByTime(10000); - await Promise.resolve(); - - expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); - - jest.advanceTimersByTime(50000); - await Promise.resolve(); + // Advance one tick at a time, flushing microtasks between each so the + // async pollFn completes and inflight promises settle before the next tick. + for (let i = 2; i <= 7; i++) { + jest.advanceTimersByTime(10000); + await jest.advanceTimersByTimeAsync(0); + } expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(7); @@ -857,6 +856,222 @@ describe('PriceDataSource', () => { controller.destroy(); }); + it('skips fetching prices for assets fetched within the freshness TTL', async () => { + const { controller, apiClient, getAssetsState } = setupController({ + balanceState: { + 'mock-account-id': { + [MOCK_NATIVE_ASSET]: { amount: '1000000000000000000' }, + }, + }, + priceResponse: { + [MOCK_NATIVE_ASSET]: createMockPriceData(2500), + }, + }); + + // First fetch — asset is stale, API is called + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Second fetch immediately after — asset is fresh, API is NOT called again + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + controller.destroy(); + }); + + it('re-fetches prices after the freshness TTL expires', async () => { + const { controller, apiClient, getAssetsState } = setupController({ + pollInterval: 10_000, + balanceState: { + 'mock-account-id': { + [MOCK_NATIVE_ASSET]: { amount: '1000000000000000000' }, + }, + }, + priceResponse: { + [MOCK_NATIVE_ASSET]: createMockPriceData(2500), + }, + }); + + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Advance past the TTL (pollInterval = 10s is used as freshness TTL) + jest.advanceTimersByTime(11_000); + + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); + + controller.destroy(); + }); + + it('invalidatePriceCache allows re-fetching assets that were fresh', async () => { + const { controller, apiClient } = setupController({ + priceResponse: { + [MOCK_TOKEN_ASSET]: createMockPriceData(1.0), + }, + }); + + const next = jest.fn().mockResolvedValue(undefined); + + // First call — populates freshness cache + const context1 = createMiddlewareContext({ + request: createDataRequest({ assetsForPriceUpdate: [MOCK_TOKEN_ASSET] }), + response: {}, + }); + await controller.assetsMiddleware(context1, next); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Second call — skipped (fresh) + const context2 = createMiddlewareContext({ + request: createDataRequest({ assetsForPriceUpdate: [MOCK_TOKEN_ASSET] }), + response: {}, + }); + await controller.assetsMiddleware(context2, next); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Invalidate cache, then fetch again — API is called + controller.invalidatePriceCache(); + const context3 = createMiddlewareContext({ + request: createDataRequest({ assetsForPriceUpdate: [MOCK_TOKEN_ASSET] }), + response: {}, + }); + await controller.assetsMiddleware(context3, next); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); + + controller.destroy(); + }); + + it('coalesces parallel fetches for the same asset into a single API call', async () => { + let resolveApi: ((value: Record) => void) | undefined; + const apiPromise = new Promise>((resolve) => { + resolveApi = resolve; + }); + + const { controller, apiClient } = setupController({ + priceResponse: { + [MOCK_TOKEN_ASSET]: createMockPriceData(1.0), + }, + }); + + // Make the API call hang until we resolve manually + apiClient.prices.fetchV3SpotPrices.mockReturnValue(apiPromise); + + const next = jest.fn().mockResolvedValue(undefined); + + // Fire two parallel middleware calls for the same asset + const context1 = createMiddlewareContext({ + request: createDataRequest({ assetsForPriceUpdate: [MOCK_TOKEN_ASSET] }), + response: {}, + }); + const context2 = createMiddlewareContext({ + request: createDataRequest({ assetsForPriceUpdate: [MOCK_TOKEN_ASSET] }), + response: {}, + }); + + const promise1 = controller.assetsMiddleware(context1, next); + const promise2 = controller.assetsMiddleware(context2, next); + + // Only ONE API call should have been made (second call joins inflight) + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Resolve the API + expect(resolveApi).toBeDefined(); + if (resolveApi) { + resolveApi({ [MOCK_TOKEN_ASSET]: createMockPriceData(1.0) }); + } + await Promise.all([promise1, promise2]); + + // Still only one API call total + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Both contexts received the price + expect(context1.response.assetsPrice?.[MOCK_TOKEN_ASSET]).toBeDefined(); + expect(context2.response.assetsPrice?.[MOCK_TOKEN_ASSET]).toBeDefined(); + + controller.destroy(); + }); + + it('freshness is per-asset — stale assets are fetched while fresh ones are skipped', async () => { + const { controller, apiClient, getAssetsState } = setupController({ + balanceState: { + 'mock-account-id': { + [MOCK_NATIVE_ASSET]: { amount: '1000000000000000000' }, + }, + }, + priceResponse: { + [MOCK_NATIVE_ASSET]: createMockPriceData(2500), + [MOCK_TOKEN_ASSET]: createMockPriceData(1.0), + }, + }); + + // Fetch only MOCK_NATIVE_ASSET via balance state + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledWith( + [MOCK_NATIVE_ASSET], + expect.anything(), + ); + + // Now middleware requests MOCK_TOKEN_ASSET (not yet fetched) and MOCK_NATIVE_ASSET (fresh) + const next = jest.fn().mockResolvedValue(undefined); + const context = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [MOCK_TOKEN_ASSET, MOCK_NATIVE_ASSET], + }), + response: {}, + }); + await controller.assetsMiddleware(context, next); + + // Only MOCK_TOKEN_ASSET should be sent to the API (MOCK_NATIVE_ASSET is fresh) + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenLastCalledWith( + [MOCK_TOKEN_ASSET], + expect.anything(), + ); + + controller.destroy(); + }); + + it('destroy clears the freshness cache', async () => { + const { controller, apiClient, getAssetsState } = setupController({ + balanceState: { + 'mock-account-id': { + [MOCK_NATIVE_ASSET]: { amount: '1000000000000000000' }, + }, + }, + priceResponse: { + [MOCK_NATIVE_ASSET]: createMockPriceData(2500), + }, + }); + + await controller.fetch(createDataRequest(), getAssetsState); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + controller.destroy(); + + // Re-create a new controller instance to verify state is gone + // (destroy clears the priceFetchedAt map — for the same instance it won't poll after destroy) + const controller2 = new PriceDataSource({ + queryApiClient: + apiClient as unknown as PriceDataSourceOptions['queryApiClient'], + getSelectedCurrency: (): SupportedCurrency => 'usd', + }); + + const getAssetsState2 = (): AssetsControllerStateInternal => + ({ + assetsBalance: { + 'mock-account-id': { + [MOCK_NATIVE_ASSET]: { amount: '1000000000000000000' }, + }, + }, + }) as unknown as AssetsControllerStateInternal; + + await controller2.fetch(createDataRequest(), getAssetsState2); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); + + controller2.destroy(); + }); + it('destroy cleans up all subscriptions', async () => { const polygonAsset = 'eip155:137/erc20:0x0000000000000000000000000000000000001010' as Caip19AssetId; diff --git a/packages/assets-controller/src/data-sources/PriceDataSource.ts b/packages/assets-controller/src/data-sources/PriceDataSource.ts index 8019f4c3a8..1a5c66044f 100644 --- a/packages/assets-controller/src/data-sources/PriceDataSource.ts +++ b/packages/assets-controller/src/data-sources/PriceDataSource.ts @@ -16,6 +16,7 @@ import type { AssetsControllerStateInternal, } from '../types'; import { fetchWithTimeout } from '../utils'; +import { CoalescingBatchFetcher } from '../utils/coalescingBatchFetcher'; import type { SubscriptionRequest } from './AbstractDataSource'; import { reduceInBatchesSerially } from './evm-rpc-services'; @@ -45,6 +46,13 @@ export type PriceDataSourceConfig = { * the batch rejects so the caller can proceed without prices. */ fetchTimeoutMs?: number; + /** + * Minimum age (ms) before a price is considered stale and re-fetched. + * Assets fetched more recently than this are skipped to avoid redundant + * API calls from overlapping middleware / subscription / manual triggers. + * Defaults to pollInterval (60 000 ms). + */ + priceFreshnessTtlMs?: number; }; export type PriceDataSourceOptions = PriceDataSourceConfig & { @@ -137,6 +145,17 @@ export class PriceDataSource { readonly #fetchTimeoutMs: number; + /** + * Coalesces price fetches by asset ID: skips assets fetched within the + * freshness TTL and joins concurrent in-flight fetches for the same asset so + * overlapping triggers (middleware + subscription poll) don't issue duplicate + * API requests. + */ + readonly #coalescer: CoalescingBatchFetcher< + Caip19AssetId, + FungibleAssetPrice + >; + /** Active subscriptions by ID */ readonly #activeSubscriptions: Map< string, @@ -150,9 +169,14 @@ export class PriceDataSource { constructor(options: PriceDataSourceOptions) { this.#getSelectedCurrency = options.getSelectedCurrency; - this.#pollInterval = options.pollInterval ?? DEFAULT_POLL_INTERVAL; + this.#pollInterval = DEFAULT_POLL_INTERVAL; this.#apiClient = options.queryApiClient; this.#fetchTimeoutMs = options.fetchTimeoutMs ?? DEFAULT_FETCH_TIMEOUT_MS; + this.#coalescer = new CoalescingBatchFetcher({ + fetchBatch: (assetIds): Promise> => + this.#executeBatchFetch(assetIds), + freshnessTtlMs: options.priceFreshnessTtlMs ?? this.#pollInterval, + }); } // ============================================================================ @@ -277,14 +301,16 @@ export class PriceDataSource { } /** - * Fetch spot prices for all provided asset IDs, splitting into batches of - * PRICE_API_BATCH_SIZE to respect API limits. + * Execute the actual batched API call for a set of asset IDs and return + * parsed price results. Used as the `fetchBatch` callback for the coalescer, + * so it does NOT check freshness or inflight state — that is handled by + * {@link CoalescingBatchFetcher}. * - * @param assetIds - Array of CAIP-19 asset IDs - * @returns Spot prices response + * @param assetIds - Asset IDs to fetch (already filtered/deduplicated). + * @returns Parsed prices keyed by CAIP-19 asset ID. */ - async #fetchSpotPrices( - assetIds: string[], + async #executeBatchFetch( + assetIds: Caip19AssetId[], ): Promise> { const selectedCurrency = this.#getSelectedCurrency(); @@ -306,6 +332,7 @@ export class PriceDataSource { initialResult: [], }); + const fetchedAt = Date.now(); const prices: Record = {}; for (const { selectedCurrencyPrices, usdPrices } of batchResults) { @@ -321,12 +348,11 @@ export class PriceDataSource { continue; } - const caipAssetId = assetId as Caip19AssetId; - prices[caipAssetId] = { + prices[assetId as Caip19AssetId] = { ...marketData, assetPriceType: 'fungible', usdPrice: usdMarketData.price, - lastUpdated: Date.now(), + lastUpdated: fetchedAt, }; } } @@ -334,6 +360,20 @@ export class PriceDataSource { return prices; } + /** + * Fetch spot prices for all provided asset IDs, deduplicating via the + * coalescer (freshness TTL + per-asset inflight coalescing). + * + * @param assetIds - Array of CAIP-19 asset IDs. + * @returns Spot prices response (only contains entries for assets that were + * actually fetched or joined from inflight). + */ + async #fetchSpotPrices( + assetIds: Caip19AssetId[], + ): Promise> { + return this.#coalescer.fetch(assetIds); + } + /** * Get unique asset IDs from the assetsBalance state. * Filters by accounts and chains from the request. @@ -474,7 +514,20 @@ export class PriceDataSource { const pollInterval = request.updateInterval ?? this.#pollInterval; - // Create poll function - fetches prices using getAssetsState from subscription + // Ensure the freshness TTL doesn't exceed the effective poll interval. + // Otherwise, assets fetched on one poll would still be "fresh" when the + // next poll fires, causing the subscription to never re-fetch. + this.#coalescer.freshnessTtlMs = Math.min( + this.#coalescer.freshnessTtlMs, + pollInterval, + ); + + // Create poll function - fetches prices using getAssetsState from subscription. + // The freshness TTL naturally gates re-fetches: assets fetched less than + // `priceFreshnessTtlMs` ago are skipped, preventing duplicates when middleware + // or other triggers already fetched the same assets between polls. + // Concurrent middleware calls will join the inflight promise rather than + // issuing duplicate requests. const pollFn = async (): Promise => { try { const subscription = this.#activeSubscriptions.get(subscriptionId); @@ -482,7 +535,6 @@ export class PriceDataSource { return; } - // Fetch prices for all assets in balance state (uses subscription's getAssetsState) const fetchResponse = await this.fetch( subscription.request, subscription.getAssetsState, @@ -535,6 +587,15 @@ export class PriceDataSource { } } + /** + * Invalidate the price freshness cache, forcing the next fetch to call the + * API regardless of TTL. Use when external state changes (e.g. selected + * currency) require a full refresh. + */ + invalidatePriceCache(): void { + this.#coalescer.invalidate(); + } + /** * Destroy the data source and clean up all subscriptions. */ @@ -543,5 +604,6 @@ export class PriceDataSource { subscription.cleanup(); } this.#activeSubscriptions.clear(); + this.#coalescer.destroy(); } } diff --git a/packages/assets-controller/src/utils/coalescingBatchFetcher.ts b/packages/assets-controller/src/utils/coalescingBatchFetcher.ts new file mode 100644 index 0000000000..0039ae3aa0 --- /dev/null +++ b/packages/assets-controller/src/utils/coalescingBatchFetcher.ts @@ -0,0 +1,218 @@ +/** + * Executes the underlying batched fetch for a set of keys. Only keys that have + * a value need to be present in the returned record; keys the source had no + * data for are simply omitted (they are still marked fresh — see + * {@link CoalescingBatchFetcher.fetch}). + * + * @param keys - The keys to fetch (already filtered to stale, not-inflight keys). + * @returns The fetched values keyed by key. + */ +export type BatchFetchFn = ( + keys: Key[], +) => Promise>; + +export type CoalescingBatchFetcherOptions = { + /** Performs the actual batched fetch for stale, not-inflight keys. */ + fetchBatch: BatchFetchFn; + /** + * Minimum age (ms) before a key is considered stale and re-fetched. Keys + * fetched more recently than this are skipped entirely. + */ + freshnessTtlMs: number; +}; + +/** + * Deduplicates batched fetches by key across two dimensions: + * + * 1. **Freshness TTL** — keys fetched within `freshnessTtlMs` are skipped + * entirely. Note the freshness window only starts once a fetch *completes*, + * so it does not cover requests that are still in flight. + * 2. **Inflight coalescing** — if a fetch is already in progress for a key, + * concurrent callers join the existing promise instead of issuing a new + * request. This covers the request-in-flight window the freshness TTL + * structurally cannot. + * + * Both layers are per-key, so a call for a partially-overlapping set of keys + * reuses the fresh/inflight keys and only fetches the genuinely-missing ones. + * + * The fetch is batched: all stale, not-inflight keys from a single `fetch()` + * call are passed to `fetchBatch` together, then split into per-key results so + * each key can be joined independently. + */ +export class CoalescingBatchFetcher { + readonly #fetchBatch: BatchFetchFn; + + #freshnessTtlMs: number; + + /** Tracks the last successful fetch time per key (freshness gating). */ + readonly #fetchedAt = new Map(); + + /** + * Per-key inflight fetch promises. Each resolves to the value, or `undefined` + * if the batch failed or returned no data for that key. + */ + readonly #inflight = new Map>(); + + constructor(options: CoalescingBatchFetcherOptions) { + this.#fetchBatch = options.fetchBatch; + this.#freshnessTtlMs = options.freshnessTtlMs; + } + + /** + * Minimum age (ms) before a key is re-fetched. + * + * @returns The current freshness TTL in milliseconds. + */ + get freshnessTtlMs(): number { + return this.#freshnessTtlMs; + } + + set freshnessTtlMs(ms: number) { + this.#freshnessTtlMs = ms; + } + + /** + * Fetch values for the given keys, deduplicating against fresh and inflight + * fetches. + * + * @param keys - The keys to fetch. + * @returns Values keyed by key. Only contains entries for keys that were + * actually fetched (or joined from inflight) and had a value. + */ + async fetch(keys: Key[]): Promise> { + const { staleKeys, inflightKeys } = this.#partition(keys); + + if (staleKeys.length === 0 && inflightKeys.length === 0) { + return {} as Record; + } + + // Start a fetch for stale keys and join any fetches already in progress. + const batchPromise = + staleKeys.length > 0 ? this.#startBatchFetch(staleKeys) : undefined; + const values = await this.#joinInflight(inflightKeys); + + if (batchPromise) { + Object.assign(values, await batchPromise); + } + + return values; + } + + /** + * Clear the freshness cache, forcing the next fetch to re-request every key + * regardless of TTL. Does not affect inflight fetches. + */ + invalidate(): void { + this.#fetchedAt.clear(); + } + + /** Clear all freshness and inflight state. */ + destroy(): void { + this.#fetchedAt.clear(); + this.#inflight.clear(); + } + + /** + * Split keys into those that need a fresh fetch and those already being + * fetched by another caller. Keys still within the freshness TTL are dropped. + * + * @param keys - The keys to classify. + * @returns `staleKeys` (need fetching) and `inflightKeys` (join existing fetch). + */ + #partition(keys: Key[]): { staleKeys: Key[]; inflightKeys: Key[] } { + const now = Date.now(); + const staleKeys: Key[] = []; + const inflightKeys: Key[] = []; + + for (const key of keys) { + if (this.#isFresh(key, now)) { + continue; + } + if (this.#inflight.has(key)) { + inflightKeys.push(key); + } else { + staleKeys.push(key); + } + } + + return { staleKeys, inflightKeys }; + } + + /** + * Returns true if the key's last fetch is still within the freshness TTL. + * + * @param key - The key to check. + * @param now - Current timestamp (avoids repeated clock reads). + * @returns True if the key was fetched within the freshness TTL. + */ + #isFresh(key: Key, now: number): boolean { + const fetchedAt = this.#fetchedAt.get(key); + return fetchedAt !== undefined && now - fetchedAt < this.#freshnessTtlMs; + } + + /** + * Launch a batch fetch and register a per-key inflight promise for each key + * so concurrent callers can join. On success, all requested keys are marked + * fresh — including keys the source returned no value for, since the absence + * of a value is itself a valid answer that should not be re-asked until the + * TTL expires. A failed batch leaves keys stale so they are retried. + * + * @param staleKeys - Keys to fetch (none of which are already inflight). + * @returns The batch fetch promise. + */ + #startBatchFetch(staleKeys: Key[]): Promise> { + const batchPromise = this.#fetchBatch(staleKeys).then((values) => { + const fetchedAt = Date.now(); + for (const key of staleKeys) { + this.#fetchedAt.set(key, fetchedAt); + } + return values; + }); + + for (const key of staleKeys) { + const perKey = batchPromise.then( + (values) => values[key], + () => undefined, + ); + this.#inflight.set(key, perKey); + } + + // Clean up inflight entries once the batch settles (success or failure). + // Rejection is already surfaced to the caller via the returned batchPromise. + batchPromise + .finally(() => { + for (const key of staleKeys) { + this.#inflight.delete(key); + } + }) + .catch(() => undefined); + + return batchPromise; + } + + /** + * Join the inflight fetches for the given keys and collect their values. Keys + * whose inflight fetch produced no value are omitted. + * + * @param inflightKeys - Keys whose fetches are already in progress. + * @returns Values keyed by key. + */ + async #joinInflight(inflightKeys: Key[]): Promise> { + const values = {} as Record; + + const results = await Promise.all( + inflightKeys.map(async (key) => { + const value = await this.#inflight.get(key); + return [key, value] as const; + }), + ); + + for (const [key, value] of results) { + if (value !== undefined) { + values[key] = value; + } + } + + return values; + } +} From fc81286aa2fa910803ebb0b827b389e523db135e Mon Sep 17 00:00:00 2001 From: Kriys94 Date: Thu, 18 Jun 2026 15:23:24 +0200 Subject: [PATCH 2/2] fix(assets-controller): detectionMiddleware --- .../middlewares/DetectionMiddleware.test.ts | 115 ++++++++++++++++-- .../src/middlewares/DetectionMiddleware.ts | 68 +++++++---- 2 files changed, 150 insertions(+), 33 deletions(-) diff --git a/packages/assets-controller/src/middlewares/DetectionMiddleware.test.ts b/packages/assets-controller/src/middlewares/DetectionMiddleware.test.ts index 084759b59e..4f5fbd1718 100644 --- a/packages/assets-controller/src/middlewares/DetectionMiddleware.test.ts +++ b/packages/assets-controller/src/middlewares/DetectionMiddleware.test.ts @@ -5,6 +5,7 @@ import type { Context, DataRequest, Caip19AssetId, + AccountId, AssetsControllerStateInternal, } from '../types'; import { DetectionMiddleware } from './DetectionMiddleware'; @@ -56,26 +57,41 @@ function createDataRequest( function createAssetsState( metadataAssets: Caip19AssetId[] = [], + trackedBalances: Record = {}, + customAssets: Record = {}, ): AssetsControllerStateInternal { const assetsInfo: Record = {}; for (const assetId of metadataAssets) { assetsInfo[assetId] = { name: `Asset ${assetId}` }; } + const assetsBalance: Record> = {}; + for (const [accountId, assetIds] of Object.entries(trackedBalances)) { + assetsBalance[accountId] = {}; + for (const assetId of assetIds) { + assetsBalance[accountId][assetId] = { amount: '0' }; + } + } return { assetsInfo, - assetsBalance: {}, - customAssets: {}, + assetsBalance, + customAssets, } as AssetsControllerStateInternal; } function createMiddlewareContext( overrides?: Partial, stateMetadata: Caip19AssetId[] = [], + trackedBalances: Record = {}, + customAssets: Record = {}, ): Context { return { request: createDataRequest(), response: {}, - getAssetsState: jest.fn().mockReturnValue(createAssetsState(stateMetadata)), + getAssetsState: jest + .fn() + .mockReturnValue( + createAssetsState(stateMetadata, trackedBalances, customAssets), + ), ...overrides, }; } @@ -151,7 +167,7 @@ describe('DetectionMiddleware', () => { expect(next).toHaveBeenCalledWith(context); }); - it('includes all balance assets in detectedAssets even when they have metadata', async () => { + it('excludes assets already tracked in state balance (metadata presence is ignored)', async () => { const { middleware } = setupController(); const context = createMiddlewareContext( { @@ -164,20 +180,21 @@ describe('DetectionMiddleware', () => { }, }, }, + // Metadata is irrelevant to detection; only tracked balances matter. [MOCK_ASSET_1, MOCK_NATIVE_ASSET], + // Both assets are already tracked in state balance. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1, MOCK_NATIVE_ASSET] }, ); const next = jest.fn().mockImplementation((ctx) => Promise.resolve(ctx)); await middleware.assetsMiddleware(context, next); - // All assets in balance are included so prices (and metadata when needed) are fetched - expect(context.response.detectedAssets).toStrictEqual({ - [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1, MOCK_NATIVE_ASSET], - }); + // Nothing is new relative to state, so detectedAssets stays undefined. + expect(context.response.detectedAssets).toBeUndefined(); expect(next).toHaveBeenCalledWith(context); }); - it('includes all balance assets in mixed scenario (metadata presence is ignored)', async () => { + it('includes only assets that are new relative to state balance', async () => { const { middleware } = setupController(); const context = createMiddlewareContext( { @@ -191,14 +208,16 @@ describe('DetectionMiddleware', () => { }, }, }, - [MOCK_ASSET_1], + [], + // MOCK_ASSET_1 is already tracked; the other two are new. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1] }, ); const next = jest.fn().mockImplementation((ctx) => Promise.resolve(ctx)); await middleware.assetsMiddleware(context, next); expect(context.response.detectedAssets).toStrictEqual({ - [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1, MOCK_ASSET_2, MOCK_NATIVE_ASSET], + [MOCK_ACCOUNT_ID]: [MOCK_ASSET_2, MOCK_NATIVE_ASSET], }); expect(next).toHaveBeenCalledWith(context); }); @@ -263,6 +282,80 @@ describe('DetectionMiddleware', () => { expect(next).toHaveBeenCalledWith(context); }); + it('includes new custom assets not yet tracked in state balance', async () => { + const { middleware } = setupController(); + const context = createMiddlewareContext( + { + response: { + assetsBalance: { + [MOCK_ACCOUNT_ID]: { + [MOCK_ASSET_1]: { amount: '1000' }, + }, + }, + }, + }, + [], + // MOCK_ASSET_1 already tracked. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1] }, + // MOCK_ASSET_2 is a custom asset with no balance entry yet. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_2] }, + ); + const next = jest.fn().mockImplementation((ctx) => Promise.resolve(ctx)); + + await middleware.assetsMiddleware(context, next); + + expect(context.response.detectedAssets).toStrictEqual({ + [MOCK_ACCOUNT_ID]: [MOCK_ASSET_2], + }); + expect(next).toHaveBeenCalledWith(context); + }); + + it('excludes custom assets that are already tracked in state balance', async () => { + const { middleware } = setupController(); + const context = createMiddlewareContext( + { + response: { + assetsBalance: { + [MOCK_ACCOUNT_ID]: { + [MOCK_ASSET_1]: { amount: '1000' }, + }, + }, + }, + }, + [], + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1] }, + // Custom asset is already tracked in balance, so it is not new. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_1] }, + ); + const next = jest.fn().mockImplementation((ctx) => Promise.resolve(ctx)); + + await middleware.assetsMiddleware(context, next); + + expect(context.response.detectedAssets).toBeUndefined(); + expect(next).toHaveBeenCalledWith(context); + }); + + it('includes new custom assets for accounts not present in the balance response', async () => { + const { middleware } = setupController(); + const context = createMiddlewareContext( + { + response: {}, + }, + [], + {}, + // Custom asset for an account with no balance response. + { [MOCK_ACCOUNT_ID]: [MOCK_ASSET_2] }, + ); + const next = jest.fn().mockImplementation((ctx) => Promise.resolve(ctx)); + + await middleware.assetsMiddleware(context, next); + + expect(context.response.detectedAssets).toStrictEqual({ + [MOCK_ACCOUNT_ID]: [MOCK_ASSET_2], + }); + expect(next).toHaveBeenCalledWith(context); + }); + it('only runs for balance dataType', async () => { const { middleware } = setupController(); const context = createMiddlewareContext({ diff --git a/packages/assets-controller/src/middlewares/DetectionMiddleware.ts b/packages/assets-controller/src/middlewares/DetectionMiddleware.ts index f5827ea3d0..6db6b4893a 100644 --- a/packages/assets-controller/src/middlewares/DetectionMiddleware.ts +++ b/packages/assets-controller/src/middlewares/DetectionMiddleware.ts @@ -16,15 +16,23 @@ createModuleLogger(projectLogger, CONTROLLER_NAME); // ============================================================================ /** - * DetectionMiddleware builds the set of assets that downstream sources use for - * metadata and price fetching. + * DetectionMiddleware builds the set of newly detected assets that downstream + * sources use for one-off metadata and price fetching. + * + * "Detected" means new relative to the AssetsController state: an asset is only + * reported for an account when it is not already tracked in + * `state.assetsBalance[accountId]`. Assets already known to the controller are + * excluded so that: + * - `AssetsController:assetsDetected` events only fire for genuinely new assets + * - prices/metadata are fetched once for new assets (recurring price refreshes + * are handled by the price subscription, not by this detection path) * * This middleware: - * - Includes every asset that appears in response.assetsBalance (so prices and - * metadata are fetched for existing assets as well as new ones) - * - Includes each account's custom assets from state (so custom tokens get - * metadata and prices even when they have no balance yet) - * - Fills response.detectedAssets with these asset IDs per account + * - Includes assets from response.assetsBalance that are not yet in state + * - Includes each account's custom assets from state that are not yet tracked + * (so newly added custom tokens get metadata and prices before they have a + * balance entry) + * - Fills response.detectedAssets with these new asset IDs per account * * TokenDataSource and PriceDataSource both key off detectedAssets. TokenDataSource * then filters to only fetch metadata for assets that lack it; PriceDataSource @@ -44,12 +52,14 @@ export class DetectionMiddleware { } /** - * Get the middleware that builds detectedAssets for metadata and price fetching. + * Get the middleware that builds detectedAssets for one-off metadata and + * price fetching. * * This middleware: - * 1. Includes all assets from response.assetsBalance (so prices are fetched for existing assets too) - * 2. Merges each account's custom assets from state - * 3. Fills response.detectedAssets with these asset IDs per account + * 1. Includes assets from response.assetsBalance that are not already tracked + * in state (`state.assetsBalance[accountId]`) + * 2. Merges each account's custom assets from state that are not yet tracked + * 3. Fills response.detectedAssets with these new asset IDs per account * * @returns The middleware function for the assets pipeline. */ @@ -57,29 +67,39 @@ export class DetectionMiddleware { return forDataTypes(['balance'], async (ctx, next) => { const { request, response } = ctx; - // Get state for custom assets + // Get state to compare against already-tracked assets const state = ctx.getAssetsState(); - const { customAssets: stateCustomAssets } = state; + const { customAssets: stateCustomAssets, assetsBalance: stateBalances } = + state; const detectedAssets: Record = {}; - // 1. From balance response: include every asset with balance (so prices + metadata path include existing assets) + // Returns the set of asset IDs already tracked for an account in state. + const getKnownAssets = (accountId: AccountId): Set => + new Set( + Object.keys(stateBalances?.[accountId] ?? {}) as Caip19AssetId[], + ); + + // 1. From balance response: include only assets that are new relative to state if (response.assetsBalance) { for (const [accountId, accountBalances] of Object.entries( response.assetsBalance, )) { + const knownAssets = getKnownAssets(accountId); const detected: Caip19AssetId[] = []; for (const assetId of Object.keys( accountBalances as Record, - )) { - detected.push(assetId as Caip19AssetId); + ) as Caip19AssetId[]) { + if (!knownAssets.has(assetId)) { + detected.push(assetId); + } } - // Merge this account's custom assets from state + // Merge this account's custom assets from state that aren't tracked yet const customForAccount = stateCustomAssets?.[accountId] ?? []; for (const assetId of customForAccount) { - if (!detected.includes(assetId)) { + if (!knownAssets.has(assetId) && !detected.includes(assetId)) { detected.push(assetId); } } @@ -90,15 +110,19 @@ export class DetectionMiddleware { } } - // 2. Accounts in request that weren't in balance response: include their custom assets + // 2. Accounts in request that weren't in balance response: include their + // custom assets that are not already tracked in state for (const { account } of request.accountsWithSupportedChains) { const accountId = account.id; if (detectedAssets[accountId]) { continue; } - const customForAccount = stateCustomAssets?.[accountId] ?? []; - if (customForAccount.length > 0) { - detectedAssets[accountId] = customForAccount; + const knownAssets = getKnownAssets(accountId); + const newCustomAssets = (stateCustomAssets?.[accountId] ?? []).filter( + (assetId) => !knownAssets.has(assetId), + ); + if (newCustomAssets.length > 0) { + detectedAssets[accountId] = newCustomAssets; } }