diff --git a/@shared/api/helpers/getIconFromTokenList.ts b/@shared/api/helpers/getIconFromTokenList.ts index ae97610db8..aa42c39169 100644 --- a/@shared/api/helpers/getIconFromTokenList.ts +++ b/@shared/api/helpers/getIconFromTokenList.ts @@ -20,7 +20,11 @@ export const getIconFromTokenLists = async ({ }) => { let verifiedToken = {} as AssetListReponseItem; let canonicalAsset = undefined as string | undefined; - for (const data of assetsListsData) { + // The lists arrive in the user's configured order, which is a priority order: + // the first list carrying the asset wins. Without the labeled break below the + // outer loop runs to completion and the LAST matching list silently overwrites + // earlier, higher-priority entries. + listLoop: for (const data of assetsListsData) { const list = data.assets; if (list) { for (const record of list) { @@ -29,7 +33,7 @@ export const getIconFromTokenLists = async ({ if (record.contract && record.contract.match(regex) && record.icon) { verifiedToken = record; canonicalAsset = getCanonicalFromAsset(code, contractId); - break; + break listLoop; } } @@ -42,7 +46,7 @@ export const getIconFromTokenLists = async ({ ) { verifiedToken = record; canonicalAsset = getCanonicalFromAsset(code, issuerId); - break; + break listLoop; } } } diff --git a/extension/src/background/messageListener/handlers/__tests__/getCachedAssetIconList.test.ts b/extension/src/background/messageListener/handlers/__tests__/getCachedAssetIconList.test.ts new file mode 100644 index 0000000000..3dbbf4b402 --- /dev/null +++ b/extension/src/background/messageListener/handlers/__tests__/getCachedAssetIconList.test.ts @@ -0,0 +1,45 @@ +import { getCachedAssetIconList } from "../getCachedAssetIconList"; + +const USDT0 = "USDT0:GATISXX6BZ6NC7IKQBY37CJD4SOZL3CYZJWXEDG6JVIY4WBS6KXJHN6Q"; +const USDC = "USDC:GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN"; +const USDC_ICON = "https://centre.io/usdc.png"; + +const makeLocalStore = (assetIconCache: unknown) => + ({ + getItem: jest.fn(async () => assetIconCache), + setItem: jest.fn(), + remove: jest.fn(), + clear: jest.fn(), + }) as any; + +describe("getCachedAssetIconList", () => { + it("returns the icons it has", async () => { + const result = await getCachedAssetIconList({ + localStore: makeLocalStore({ [USDC]: USDC_ICON }), + }); + + expect(result.icons).toEqual({ [USDC]: USDC_ICON }); + }); + + it("leaves out assets recorded as having no icon", async () => { + // A null here means an earlier lookup came up empty, and getAssetIcons + // reads it as "already tried, don't look again". Because this cache is on + // disk that verdict outlived the session, so an asset whose icon failed + // once — USDT0, whose LOBSTR url 403s browsers — stayed iconless forever. + // Dropping nulls turns it back into an ordinary cache miss, and the fresh + // lookup overwrites the stale entry. + const result = await getCachedAssetIconList({ + localStore: makeLocalStore({ [USDC]: USDC_ICON, [USDT0]: null }), + }); + + expect(result.icons).toEqual({ [USDC]: USDC_ICON }); + }); + + it("returns an empty map when nothing is cached", async () => { + const result = await getCachedAssetIconList({ + localStore: makeLocalStore(undefined), + }); + + expect(result.icons).toEqual({}); + }); +}); diff --git a/extension/src/background/messageListener/handlers/getCachedAssetIconList.ts b/extension/src/background/messageListener/handlers/getCachedAssetIconList.ts index e27161ffb5..000c55f196 100644 --- a/extension/src/background/messageListener/handlers/getCachedAssetIconList.ts +++ b/extension/src/background/messageListener/handlers/getCachedAssetIconList.ts @@ -10,6 +10,52 @@ export const getCachedAssetIconList = async ({ (await localStore.getItem(CACHED_ASSET_ICONS_ID)) || {}; return { - icons: assetIconCache, + // A null entry records that a lookup came up empty, and getAssetIcons reads + // one as "already tried, don't look again". That was only ever meant to last + // a session, but this cache is on disk, so the verdict outlived it: an asset + // whose icon failed once stayed iconless for good, with no way back — no + // icon means no , so nothing fires the error handler that would have + // retried. Dropping nulls here turns them back into ordinary cache misses, + // and the fresh lookup overwrites the stale entry. + // + // TODO: this is a read-side workaround, not the real fix. It hides bad data + // rather than stopping it being written, and it rules out ever storing a + // meaningful null here. + // + // Retrying is the point: it is how an asset that got stuck gets its icon + // back. But the retry is not free, and for an asset with no icon anywhere + // it repeats forever without ever succeeding. Each attempt costs a + // token-list scan and, when that finds nothing, one batched Horizon call + // covering every such issuer plus a stellar.toml fetch for each issuer that + // publishes a home domain. (USDT0 stops at the Horizon call: its issuer + // publishes no home domain and, with its master key at weight 0, never + // can.) + // + // That cost is newly paid by the flows that load balances with icons — + // swap, send, manage assets, history — which previously skipped a + // null-marked asset outright. The Account view's own icon hook already + // retried regardless, since it passes an empty cache to its lookup pass, so + // nothing changes there. And nothing suppresses the retry from one popup to + // the next: closing the popup tears it down, taking Redux — the only record + // of what this session already resolved — with it, so every open starts + // from nothing. + // + // A negative cache that survives the popup, with a TTL so it expires rather + // than latching, is the sane way to stop the endless retry — and this + // filter would silently swallow one. + // + // To fix properly, in order: + // 1. Stop persisting nulls: retryAssetIcon sends `iconUrl: null` meaning + // "clear this", so cacheAssetIcon should delete the entry rather than + // store the null. + // 2. Add a migration to clear the nulls already on disk. It cannot be + // done by (1) alone, since nothing rewrites an entry the lookup skips. + // 3. Drop this filter, so the read path goes back to being a plain + // accessor and null is free to mean something again. + // Doing (2) without (1) is not enough on its own: every later icon failure + // writes a fresh null and puts that user straight back into the bug. + icons: Object.fromEntries( + Object.entries(assetIconCache).filter(([, iconUrl]) => iconUrl), + ), }; }; diff --git a/extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js b/extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js index 49ffe0d371..5974ffb2cb 100644 --- a/extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js +++ b/extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js @@ -11,7 +11,7 @@ const VERIFIED_TOKEN_ISSUER = validAssetList.assets[0].issuer; const VERIFIED_TOKEN_CODE = validAssetList.assets[0].code; const EXPECTED_ICON_URL = validAssetList.assets[0].icon; -jest +(jest .spyOn(ExtensionMessaging, "sendMessageToBackground") .mockImplementation(() => Promise.resolve({ @@ -20,7 +20,7 @@ jest ), jest .spyOn(TokenListHelpers, "getCombinedAssetListData") - .mockImplementation(() => Promise.resolve([validAssetList])); + .mockImplementation(() => Promise.resolve([validAssetList]))); describe("getIconFromTokenLists", () => { it("should return an icon if an asset is in a token list by contract ID", async () => { @@ -55,3 +55,40 @@ describe("getIconFromTokenLists", () => { expect(canonicalAsset).toBeUndefined(); }); }); + +describe("getIconFromTokenLists list priority", () => { + // USDT0 is on the Soroswap list with a direct https icon and on the LOBSTR + // list with an ipfs.io gateway url. That gateway answers curl but returns 403 + // to a browser User-Agent, so an can never load it. Keeping whichever + // list was read last handed the browser the one url it cannot fetch. + const laterList = { + ...validAssetList, + provider: "Later Provider", + assets: [ + { + ...validAssetList.assets[0], + icon: "https://later-list.example/icon.png", + }, + ], + }; + + it("uses the icon from the first matching list, matched by issuer", async () => { + const { icon } = await getIconFromTokenLists({ + issuerId: VERIFIED_TOKEN_ISSUER, + code: VERIFIED_TOKEN_CODE, + assetsListsData: [validAssetList, laterList], + }); + + expect(icon).toEqual(EXPECTED_ICON_URL); + }); + + it("uses the icon from the first matching list, matched by contract ID", async () => { + const { icon } = await getIconFromTokenLists({ + contractId: VERIFIED_TOKEN_CONTRACT, + code: VERIFIED_TOKEN_CODE, + assetsListsData: [validAssetList, laterList], + }); + + expect(icon).toEqual(EXPECTED_ICON_URL); + }); +});