From 44bc6a62d4a796e75d9d7106fa313607b16a35ad Mon Sep 17 00:00:00 2001 From: gomes <17035424+gomesalexandre@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:41:53 +0200 Subject: [PATCH 1/2] fix(assets-controller): price deduper drops erc20 prices for inflight joiners and can mix currencies Two coupled bugs in PriceDataSource's price-fetching deduper: 1. The deduper's inflight-joiner path looked up the batch's raw API response by the requested key. MetaMask requests/stores checksummed CAIP-19 asset IDs, but the Price API's response echoes back lowercase-cased ERC-20 addresses. A caller starting the batch was rescued by AssetsController's own downstream key re-normalization, but a caller joining the same fetch as an inflight promise looked its checksummed key up directly against the raw (lowercase) response and got nothing back -- every ERC-20 price silently vanished for inflight joiners, self-healing only after the freshness TTL expired. 2. The deduper's cache/inflight key had no currency component, so a currency switch racing an in-flight fetch could join (or later receive) a price fetched under the previously-selected currency. Fix: introduce a composite `currency:assetId` deduper key (`PriceDeduperKey`), normalize asset IDs consistently at the point a key is built and at the point the API response is matched back against it (using `safeNormalizeAssetId` throughout, since response data is untrusted and a single malformed key must not poison an otherwise-valid batch), and decode the currency from the key inside `#executeBatchFetch` instead of re-reading the live selected currency at execution time (a second, independent source of the same race). Composite keys alone stop a *new* request from joining a stale-currency fetch, but a slow stale-currency fetch already independently in flight would otherwise still complete and let its now-superseded values reach the caller. `#executeBatchFetch` now discards its own result if the selected currency has moved on by the time the batch settles, closing that second half of the race. Two new regression-test describe blocks, plus a `forceUpdate`/ `invalidateKeys` normalization test and a checksummed-vs-lowercase caller-coalescing test, all verified red-before/green-after against a stashed pre-fix version of the source. Existing tests using an all-lowercase mock asset ID (a coincidentally-unaffected 0.3% of real address space) updated to assert the now-normalized key. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lh6V2uPTUqauqq45BM7m5k --- packages/assets-controller/CHANGELOG.md | 5 + .../src/data-sources/PriceDataSource.test.ts | 302 +++++++++++++++++- .../src/data-sources/PriceDataSource.ts | 201 ++++++++++-- 3 files changed, 480 insertions(+), 28 deletions(-) diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 45acf6cd067..8ce2ae36a9c 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Bump `@metamask/transaction-controller` from `^69.6.1` to `^69.7.0` ([#10046](https://github.com/MetaMask/core/pull/10046)) +### Fixed + +- Fix `PriceDataSource` losing ERC-20 prices for callers that joined an in-flight fetch, caused by the deduper matching the Price API's response (lowercase-cased addresses) against the caller's checksummed request key ([#10061](https://github.com/MetaMask/core/pull/10061)) +- Fix a currency switch racing an in-flight price fetch, which could let a caller join (or receive) a price fetched under the previously-selected currency ([#10061](https://github.com/MetaMask/core/pull/10061)) + ## [14.0.3] ### Changed diff --git a/packages/assets-controller/src/data-sources/PriceDataSource.test.ts b/packages/assets-controller/src/data-sources/PriceDataSource.test.ts index c2fb5b1a61a..69e66085c15 100644 --- a/packages/assets-controller/src/data-sources/PriceDataSource.test.ts +++ b/packages/assets-controller/src/data-sources/PriceDataSource.test.ts @@ -828,11 +828,17 @@ describe('PriceDataSource', () => { await controller.assetsMiddleware(context, next); + // `assetsForPriceUpdate` is not pre-normalized by the caller; the data + // source normalizes it before both the outgoing API request and the key + // it stores the result under (real ERC-20 addresses are checksummed — + // MOCK_TOKEN_ASSET is deliberately all-lowercase so this exercises that). expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledWith( - [MOCK_TOKEN_ASSET], + [normalizeAssetId(MOCK_TOKEN_ASSET)], { currency: 'usd', includeMarketData: true }, ); - expect(context.response.assetsPrice?.[MOCK_TOKEN_ASSET]).toStrictEqual({ + expect( + context.response.assetsPrice?.[normalizeAssetId(MOCK_TOKEN_ASSET)], + ).toStrictEqual({ assetPriceType: 'fungible', price: 1.0, usdPrice: 1.0, @@ -868,7 +874,9 @@ describe('PriceDataSource', () => { [normalizeAssetId(MOCK_TOKEN_ASSET)], { currency: 'usd', includeMarketData: true }, ); - expect(context.response.assetsPrice?.[MOCK_TOKEN_ASSET]).toStrictEqual({ + expect( + context.response.assetsPrice?.[normalizeAssetId(MOCK_TOKEN_ASSET)], + ).toStrictEqual({ assetPriceType: 'fungible', price: 1.0, usdPrice: 1.0, @@ -960,7 +968,9 @@ describe('PriceDataSource', () => { await controller.assetsMiddleware(context, next); expect(context.response.assetsPrice?.[anotherAsset]).toBeDefined(); - expect(context.response.assetsPrice?.[MOCK_TOKEN_ASSET]).toBeDefined(); + expect( + context.response.assetsPrice?.[normalizeAssetId(MOCK_TOKEN_ASSET)], + ).toBeDefined(); controller.destroy(); }); @@ -1138,9 +1148,19 @@ describe('PriceDataSource', () => { // 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(); + // Both contexts received the price, keyed by the normalized (checksummed) + // asset ID — including context2, which joined the inflight fetch rather + // than starting its own. Before the fix, an inflight joiner's per-key + // promise resolved by looking up the requested (already-normalized) key + // in the batch's raw, differently-cased response object and found + // nothing, so context2 silently received no price at all for this asset. + const normalizedTokenAsset = normalizeAssetId(MOCK_TOKEN_ASSET); + expect(context1.response.assetsPrice?.[normalizedTokenAsset]).toStrictEqual( + expect.objectContaining({ price: 1.0 }), + ); + expect(context2.response.assetsPrice?.[normalizedTokenAsset]).toStrictEqual( + expect.objectContaining({ price: 1.0 }), + ); controller.destroy(); }); @@ -1179,7 +1199,7 @@ describe('PriceDataSource', () => { // 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], + [normalizeAssetId(MOCK_TOKEN_ASSET)], expect.anything(), ); @@ -1315,4 +1335,270 @@ describe('PriceDataSource', () => { controller.destroy(); }, ); + + describe('regression: checksummed vs lowercase asset ID casing', () => { + // Real USDC address. The Price API's response echoes back whatever + // casing it normalizes addresses to (lowercase in practice), which does + // not match the app's own checksummed request form — this must not + // desync the deduper's key from the batch's response key. + const CHECKSUMMED_TOKEN_ASSET = + 'eip155:1/erc20:0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48' as Caip19AssetId; + const LOWERCASE_TOKEN_ASSET = normalizeAssetId( + CHECKSUMMED_TOKEN_ASSET, + ).toLowerCase() as Caip19AssetId; + + it('an inflight joiner receives the price when the API response key casing differs from the request casing', async () => { + expect(LOWERCASE_TOKEN_ASSET).not.toBe(CHECKSUMMED_TOKEN_ASSET); + + let resolveApi: ((value: Record) => void) | undefined; + const apiPromise = new Promise>((resolve) => { + resolveApi = resolve; + }); + + const { controller, apiClient } = setupController({}); + apiClient.prices.fetchV3SpotPrices.mockReturnValue(apiPromise); + + const next = jest.fn().mockResolvedValue(undefined); + + // Both callers request the CHECKSUMMED form (the app's normalized + // form); the API mock will respond with the LOWERCASE form, matching + // how the real Price API actually echoes back addresses. + const starter = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + }), + response: {}, + }); + const joiner = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + }), + response: {}, + }); + + const starterPromise = controller.assetsMiddleware(starter, next); + const joinerPromise = controller.assetsMiddleware(joiner, next); + + // Only one API call: the joiner coalesced onto the starter's inflight + // fetch instead of issuing its own. + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + expect(resolveApi).toBeDefined(); + resolveApi?.({ + [LOWERCASE_TOKEN_ASSET]: createMockPriceData(1.0), + }); + await Promise.all([starterPromise, joinerPromise]); + + const expectedPrice = expect.objectContaining({ price: 1.0 }); + // Before the fix: the starter's own result was rescued by downstream + // (AssetsController-level) key re-normalization, but the joiner's + // per-key inflight promise looked up its checksummed key in a + // lowercase-keyed raw response and got `undefined` — the price was + // silently dropped for the joiner only. + expect( + starter.response.assetsPrice?.[CHECKSUMMED_TOKEN_ASSET], + ).toStrictEqual(expectedPrice); + expect( + joiner.response.assetsPrice?.[CHECKSUMMED_TOKEN_ASSET], + ).toStrictEqual(expectedPrice); + + controller.destroy(); + }); + + it('a caller requesting the lowercase form coalesces onto an inflight fetch started for the checksummed form', async () => { + const { controller, apiClient } = setupController({ + priceResponse: { + [LOWERCASE_TOKEN_ASSET]: createMockPriceData(1.0), + }, + }); + + const next = jest.fn().mockResolvedValue(undefined); + + const checksummedCaller = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + }), + response: {}, + }); + const lowercaseCaller = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [LOWERCASE_TOKEN_ASSET], + }), + response: {}, + }); + + await Promise.all([ + controller.assetsMiddleware(checksummedCaller, next), + controller.assetsMiddleware(lowercaseCaller, next), + ]); + + // One real-world asset requested under two different casings still + // produces a single deduper key (both callers' keys normalize to the + // same value), so only one API call is made. + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + const expectedPrice = expect.objectContaining({ price: 1.0 }); + expect( + checksummedCaller.response.assetsPrice?.[CHECKSUMMED_TOKEN_ASSET], + ).toStrictEqual(expectedPrice); + expect( + lowercaseCaller.response.assetsPrice?.[CHECKSUMMED_TOKEN_ASSET], + ).toStrictEqual(expectedPrice); + + controller.destroy(); + }); + }); + + describe('regression: forceUpdate invalidation uses the normalized deduper key', () => { + it('forceUpdate on an unnormalized asset ID actually forces a re-fetch, not a silent no-op', async () => { + const CHECKSUMMED_TOKEN_ASSET = + 'eip155:1/erc20:0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48' as Caip19AssetId; + const LOWERCASE_TOKEN_ASSET = normalizeAssetId( + CHECKSUMMED_TOKEN_ASSET, + ).toLowerCase() as Caip19AssetId; + + const { controller, apiClient } = setupController({ + priceResponse: { + [LOWERCASE_TOKEN_ASSET]: createMockPriceData(1.0), + }, + }); + + const next = jest.fn().mockResolvedValue(undefined); + + // First fetch: populates the freshness cache under the normalized key. + await controller.assetsMiddleware( + createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + }), + response: {}, + }), + next, + ); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Second call, same asset, no forceUpdate: freshness cache skips it — + // still within TTL, no new API call. + await controller.assetsMiddleware( + createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + }), + response: {}, + }), + next, + ); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(1); + + // Third call, forceUpdate: must invalidate the SAME (normalized) key + // that was populated in the first fetch, or invalidateKeys silently + // targets a key that was never in the freshness cache in the first + // place, and the "forced" refetch is actually skipped. + await controller.assetsMiddleware( + createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [CHECKSUMMED_TOKEN_ASSET], + forceUpdate: true, + }), + response: {}, + }), + next, + ); + expect(apiClient.prices.fetchV3SpotPrices).toHaveBeenCalledTimes(2); + + controller.destroy(); + }); + }); + + describe('regression: currency switch racing an inflight fetch', () => { + it('a request under a new currency does not join an inflight fetch started under the previous currency', async () => { + let currentCurrency: SupportedCurrency = 'usd'; + const { controller, apiClient } = setupController({ + getSelectedCurrency: () => currentCurrency, + }); + + // The FIRST call (the original USD fetch) hangs until manually + // resolved. Every later call resolves immediately — including the EUR + // fetch's own internal USD-baseline leg (PriceDataSource always fetches + // a USD baseline alongside a non-USD currency), which must NOT be + // confused with the original hanging USD call just because it also + // requests `currency: 'usd'`; only call order distinguishes them here. + let resolveUsd: ((value: Record) => void) | undefined; + const usdPromise = new Promise>((resolve) => { + resolveUsd = resolve; + }); + let callCount = 0; + apiClient.prices.fetchV3SpotPrices.mockImplementation( + (_assetIds: string[], options: { currency: SupportedCurrency }) => { + callCount += 1; + if (callCount === 1) { + return usdPromise; + } + if (options.currency === 'usd') { + return Promise.resolve({ + [MOCK_TOKEN_ASSET]: createMockPriceData(2419.66), // EUR fetch's own USD baseline + }); + } + return Promise.resolve({ + [MOCK_TOKEN_ASSET]: createMockPriceData(2087.78), // real EUR price + }); + }, + ); + + const next = jest.fn().mockResolvedValue(undefined); + + // Start a fetch under USD; it hangs (join point for the race). + const usdContext = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [MOCK_TOKEN_ASSET], + }), + response: {}, + }); + const usdMiddlewarePromise = controller.assetsMiddleware( + usdContext, + next, + ); + + // Switch currency mid-flight, mirroring AssetsController's real + // currency-change handler: invalidate the freshness cache (never the + // inflight map — invalidate() is documented not to touch it), then + // re-request the same asset under the new currency. + currentCurrency = 'eur'; + controller.invalidatePriceCache(); + + const eurContext = createMiddlewareContext({ + request: createDataRequest({ + assetsForPriceUpdate: [MOCK_TOKEN_ASSET], + }), + response: {}, + }); + // Before the fix: the deduper key had no currency component, so this + // EUR request would see the USD fetch still registered in `#inflight` + // under the same bare asset-ID key and join it — receiving the (still + // pending) USD result once resolved, mislabeled as EUR. + await controller.assetsMiddleware(eurContext, next); + + const normalizedTokenAsset = normalizeAssetId(MOCK_TOKEN_ASSET); + expect( + eurContext.response.assetsPrice?.[normalizedTokenAsset], + ).toStrictEqual(expect.objectContaining({ price: 2087.78 })); + + // Resolve the now-stale USD fetch. Its result is discarded rather than + // delivered to the caller that originally requested it — by the time + // it settles, the currency has already moved on to EUR, so returning + // it would let outdated-currency data reach state just as easily as + // joining the wrong inflight promise would have. Key isolation alone + // stops a *new* request from joining a stale fetch; discarding a stale + // fetch's own result on settlement closes the other half of the race. + resolveUsd?.({ + [MOCK_TOKEN_ASSET]: createMockPriceData(2419.66), // real USD price + }); + await usdMiddlewarePromise; + expect( + usdContext.response.assetsPrice?.[normalizedTokenAsset], + ).toBeUndefined(); + + controller.destroy(); + }); + }); }); diff --git a/packages/assets-controller/src/data-sources/PriceDataSource.ts b/packages/assets-controller/src/data-sources/PriceDataSource.ts index 56a263b4294..1d9928bf957 100644 --- a/packages/assets-controller/src/data-sources/PriceDataSource.ts +++ b/packages/assets-controller/src/data-sources/PriceDataSource.ts @@ -16,7 +16,11 @@ import type { AssetsControllerStateInternal, } from '../types.js'; import { DedupingBatchFetcher } from '../utils/dedupingBatchFetcher.js'; -import { fetchWithTimeout, normalizeAssetId } from '../utils/index.js'; +import { + fetchWithTimeout, + normalizeAssetId, + safeNormalizeAssetId, +} from '../utils/index.js'; import type { SubscriptionRequest } from './AbstractDataSource.js'; import { reduceInBatchesSerially } from './evm-rpc-services/index.js'; @@ -109,6 +113,50 @@ type SpotPriceMarketData = Omit< 'lastUpdated' | 'assetPriceType' >; +/** + * Deduper cache/inflight key: the currency a price was requested in, prefixed + * onto the asset ID. Without this, a currency switch racing an in-flight + * fetch for the same asset ID can join a promise fetched under the previous + * currency (see {@link makeDeduperKey}). + */ +type PriceDeduperKey = `${SupportedCurrency}:${Caip19AssetId}`; + +/** + * Build a currency-scoped deduper key so requests for the same asset under + * different currencies never collide in the deduper's per-key cache/inflight + * maps — a currency switch always produces a key the deduper has never seen, + * so it can never join a promise fetched under the previous currency. + * + * @param currency - The currency the price is requested in. + * @param assetId - The CAIP-19 asset ID. + * @returns The composite deduper key. + */ +function makeDeduperKey( + currency: SupportedCurrency, + assetId: Caip19AssetId, +): PriceDeduperKey { + return `${currency}:${assetId}`; +} + +/** + * Split a composite deduper key back into its currency and asset ID. Splits + * on the first colon only — currency codes never contain one, and CAIP-19 + * asset IDs always do. + * + * @param key - The composite deduper key. + * @returns The currency and asset ID that made up the key. + */ +function parseDeduperKey(key: PriceDeduperKey): { + currency: SupportedCurrency; + assetId: Caip19AssetId; +} { + const separatorIndex = key.indexOf(':'); + return { + currency: key.slice(0, separatorIndex) as SupportedCurrency, + assetId: key.slice(separatorIndex + 1) as Caip19AssetId, + }; +} + /** * Type guard to check if market data has a valid price * @@ -159,7 +207,7 @@ export class PriceDataSource { * overlapping triggers (middleware + subscription poll) don't issue duplicate * API requests. */ - readonly #deduper: DedupingBatchFetcher; + readonly #deduper: DedupingBatchFetcher; /** Active subscriptions by ID */ readonly #activeSubscriptions: Map< @@ -179,9 +227,9 @@ export class PriceDataSource { this.#fetchTimeoutMs = options.fetchTimeoutMs ?? DEFAULT_FETCH_TIMEOUT_MS; this.#deduper = new DedupingBatchFetcher({ fetchBatch: ( - assetIds, - ): Promise> => - this.#executeBatchFetch(assetIds), + keys, + ): Promise> => + this.#executeBatchFetch(keys), freshnessTtlMs: options.priceFreshnessTtlMs ?? this.#pollInterval, }); } @@ -254,7 +302,12 @@ export class PriceDataSource { } if (request.forceUpdate) { - this.#deduper.invalidateKeys(priceableAssetIds); + const currentCurrency = this.#getSelectedCurrency(); + this.#deduper.invalidateKeys( + priceableAssetIds.map((assetId) => + makeDeduperKey(currentCurrency, safeNormalizeAssetId(assetId)), + ), + ); } try { @@ -325,18 +378,65 @@ export class PriceDataSource { } /** - * Execute the actual batched API call for a set of asset IDs and return + * Execute the actual batched API call for a set of deduper keys and return * parsed price results. Used as the `fetchBatch` callback for the deduper, * so it does NOT check freshness or inflight state — that is handled by * {@link DedupingBatchFetcher}. * - * @param assetIds - Asset IDs to fetch (already filtered/deduplicated). - * @returns Parsed prices keyed by CAIP-19 asset ID. + * Every key in one call shares the same currency (they all originate from a + * single {@link #fetchSpotPrices} call, which snapshots the currency once) + * — so the currency is decoded from the keys themselves rather than + * re-reading {@link #getSelectedCurrency}, which could have moved on to a + * different currency by the time this async callback actually runs. + * + * The returned record is keyed by the *requested* deduper key (asset ID + * normalized the same way the caller's key was), not by whatever casing the + * Price API's response happens to use — otherwise a caller joining this + * fetch as an inflight promise looks up its own (normalized) key against a + * differently-cased response key and gets nothing back. + * + * Asset IDs from response bodies are treated as untrusted: normalization + * uses {@link safeNormalizeAssetId}, which cannot throw, so one + * unexpected/malformed key in a large batch response can't discard every + * other (valid) asset's price in the same batch. + * + * If the currently-selected currency has moved on from the currency this + * batch was fetched under by the time it settles (a currency switch raced + * this fetch), the result is discarded entirely rather than returned — + * closing the race one step earlier than deduper-key isolation alone does: + * key isolation stops a *new* request from joining a stale-currency + * fetch, but a slow stale-currency fetch that was already independently + * in flight would otherwise still complete normally and let its + * now-superseded values reach the caller (and, from there, state). + * + * @param keys - Deduper keys to fetch (already filtered/deduplicated). + * @returns Parsed prices keyed by the same deduper key that was requested, + * or an empty record if the batch's currency is no longer selected. */ async #executeBatchFetch( - assetIds: Caip19AssetId[], - ): Promise> { - const selectedCurrency = this.#getSelectedCurrency(); + keys: PriceDeduperKey[], + ): Promise> { + if (keys.length === 0) { + return {}; + } + + // All keys in one batch share a currency (see doc comment above). + const { currency: selectedCurrency } = parseDeduperKey(keys[0]); + const assetIdToKey = new Map(); + for (const key of keys) { + const { currency, assetId } = parseDeduperKey(key); + if (currency !== selectedCurrency) { + // Should be unreachable: #fetchSpotPrices only ever builds keys for + // one currency per call. Guard against it anyway rather than fetch + // some assets under the wrong currency if this invariant is ever + // broken by a future change. + throw new Error( + `PriceDataSource: batch contains mixed currencies (${selectedCurrency} and ${currency})`, + ); + } + assetIdToKey.set(assetId, key); + } + const assetIds = [...assetIdToKey.keys()]; type BatchResult = { selectedCurrencyPrices: V3SpotPricesResponse; @@ -356,14 +456,42 @@ export class PriceDataSource { initialResult: [], }); + if (this.#getSelectedCurrency() !== selectedCurrency) { + // The currency changed while this batch was in flight. Its values are + // for a currency nobody is displaying anymore — discard rather than + // let them flow into the caller (and from there, state) mislabeled + // under a currency they were never fetched in. + return {}; + } + const fetchedAt = Date.now(); - const prices: Record = {}; + const prices: Record = {}; for (const { selectedCurrencyPrices, usdPrices } of batchResults) { - for (const [assetId, marketData] of Object.entries( + // Index the USD companion response by normalized asset ID too — the + // two responses come from separate API calls, and nothing guarantees + // they use identical casing for the same asset. + const normalizedUsdPrices = new Map(); + for (const [rawAssetId, marketData] of Object.entries(usdPrices)) { + normalizedUsdPrices.set( + safeNormalizeAssetId(rawAssetId as Caip19AssetId), + marketData, + ); + } + + for (const [rawAssetId, marketData] of Object.entries( selectedCurrencyPrices, )) { - const usdMarketData = usdPrices[assetId]; + // The Price API's response key casing does not necessarily match the + // requested (normalized) casing — normalize it back so the result is + // keyed the same way the request was. Untrusted input: falls back to + // the raw ID (which then simply won't match anything in + // `assetIdToKey`) rather than throwing and discarding the rest of + // the batch. + const normalizedAssetId = safeNormalizeAssetId( + rawAssetId as Caip19AssetId, + ); + const usdMarketData = normalizedUsdPrices.get(normalizedAssetId); if ( !isValidMarketData(marketData) || @@ -372,7 +500,13 @@ export class PriceDataSource { continue; } - prices[assetId as Caip19AssetId] = { + const key = assetIdToKey.get(normalizedAssetId); + if (key === undefined) { + // Response contains an asset we didn't ask for; ignore it. + continue; + } + + prices[key] = { ...marketData, assetPriceType: 'fungible', usdPrice: usdMarketData.price, @@ -386,16 +520,43 @@ export class PriceDataSource { /** * Fetch spot prices for all provided asset IDs, deduplicating via the - * deduper (freshness TTL + per-asset inflight coalescing). + * deduper (freshness TTL + per-asset, per-currency inflight coalescing). + * + * Both the deduper key and the returned record are built from the + * normalized asset ID, even if the caller passed an unnormalized one — + * so (a) two callers that mean the same real-world asset but formatted the + * address differently (e.g. one checksummed, one lowercase) join the same + * cache/inflight entry instead of silently missing each other, and (b) the + * result is keyed the same way state (`assetsPrice`) is, which is always + * normalized (see `AssetsController`'s response normalization). * * @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). + * @returns Spot prices response, keyed by the *normalized* asset ID — + * only contains entries for assets that were actually fetched or joined + * from inflight. */ async #fetchSpotPrices( assetIds: Caip19AssetId[], ): Promise> { - return this.#deduper.fetch(assetIds); + const currency = this.#getSelectedCurrency(); + const keyToAssetId = new Map(); + const keys = assetIds.map((assetId) => { + const normalizedAssetId = safeNormalizeAssetId(assetId); + const key = makeDeduperKey(currency, normalizedAssetId); + keyToAssetId.set(key, normalizedAssetId); + return key; + }); + + const results = await this.#deduper.fetch(keys); + + const prices: Record = {}; + for (const [key, price] of Object.entries(results)) { + const assetId = keyToAssetId.get(key as PriceDeduperKey); + if (assetId !== undefined) { + prices[assetId] = price; + } + } + return prices; } /** From 363f6cbb3fc96e64a97a43d6d7b15c9d7329a929 Mon Sep 17 00:00:00 2001 From: gomes <17035424+gomesalexandre@users.noreply.github.com> Date: Tue, 1 Sep 2026 23:42:46 +0200 Subject: [PATCH 2/2] chore: backfill real PR number in changelog links --- packages/assets-controller/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 8ce2ae36a9c..694385a5b31 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -13,8 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fix `PriceDataSource` losing ERC-20 prices for callers that joined an in-flight fetch, caused by the deduper matching the Price API's response (lowercase-cased addresses) against the caller's checksummed request key ([#10061](https://github.com/MetaMask/core/pull/10061)) -- Fix a currency switch racing an in-flight price fetch, which could let a caller join (or receive) a price fetched under the previously-selected currency ([#10061](https://github.com/MetaMask/core/pull/10061)) +- Fix `PriceDataSource` losing ERC-20 prices for callers that joined an in-flight fetch, caused by the deduper matching the Price API's response (lowercase-cased addresses) against the caller's checksummed request key ([#10063](https://github.com/MetaMask/core/pull/10063)) +- Fix a currency switch racing an in-flight price fetch, which could let a caller join (or receive) a price fetched under the previously-selected currency ([#10063](https://github.com/MetaMask/core/pull/10063)) ## [14.0.3]