fix(icons): show USDT0's icon, for new and existing users - #2994
Conversation
USDT0 is carried by two of the default asset lists: Soroswap, with a direct https icon, and LOBSTR, with an ipfs.io gateway url. That gateway serves the image to curl but returns 403 to a browser User-Agent, so an <img> can never load it. Two things then went wrong. Icon lookup only broke out of its inner record loop, so the outer loop ran to completion and the LAST matching list won — handing the browser the one url it cannot fetch. The lists arrive in the user's configured order, which is meant to be a priority order, so the first match should win. When that image failed, the retry recorded "no icon" in the cache as a null, and lookup reads a null as "already tried, don't look again". That was only ever meant to last a session, but the cache is on disk, so the verdict outlived it: the asset stayed iconless for good, with no way back — no icon means no <img>, so nothing fires the error handler that would have retried. Reading the cache now skips nulls, which turns them into ordinary cache misses and lets the fresh lookup overwrite the stale entry. Users already stuck recover on first load. The in-session memo is untouched: it lives in Redux, which is where the comment in getAssetIcons always said it lived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-05f821b2b89f6f22d0ec |
There was a problem hiding this comment.
Pull request overview
Fixes missing USDT0 icons by respecting token-list priority and retrying persisted icon misses.
Changes:
- Uses the first matching token-list icon.
- Filters persisted null icon entries.
- Adds regression coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
extension/src/popup/helpers/__tests__/getIconFromTokenLists.test.js |
Tests issuer and contract list priority. |
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts |
Excludes persisted null icon entries. |
extension/src/background/messageListener/handlers/__tests__/getCachedAssetIconList.test.ts |
Tests cache filtering behavior. |
@shared/api/helpers/getIconFromTokenList.ts |
Stops lookup after the first matching list. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Records what the real fix is and why it needs three steps in order: stop persisting nulls, migrate the ones already on disk, then drop the filter. Notes that migrating alone would not hold, since every later icon failure writes a fresh null, and that the filter currently rules out a durable negative cache we may well want. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts:21
- Please reference a tracking issue in this TODO (for example,
TODO(#1234): ...) so the planned cache cleanup cannot be silently forgotten.
// TODO: this is a read-side workaround, not the real fix. It hides bad data
The Account view re-runs the icon hook every time balances change, and the lookup pass was handed an empty cache, so it ignored what the session had already worked out. An asset with no icon anywhere went through the whole chain again on every pass — token lists, then Horizon, then the issuer's toml — rather than once. Passing the session's icon map means the nulls it holds do what they were meant to: mark an asset as settled for this session. Behaviour on a fresh popup is unchanged, since that map starts empty. Raised in review of the persisted-null fix. It is not caused by that change: getAssetIcons records nothing for an asset it skips, so a cached null and an absent entry both left the asset looking unresolved to this hook and both triggered the same repeat lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts:21
- This TODO has no tracking reference, so the deferred storage migration/cache-contract work can be lost after this hotfix. Freighter's review guidance requires TODOs to reference an issue; please link the issue that owns these follow-up steps (for example,
TODO(#…):).
// TODO: this is a read-side workaround, not the real fix. It hides bad data
…up again" This reverts commit 676006d3b0f6a1f0e0e5e1e0f8a4f5d5f3c8e7a1. Honouring the session's icon map here is unsafe, because that map is shared with a path that looks less hard. History's getIconUrl consults the token lists and, when they are cached — which the Account view guarantees — never reaches the issuer-toml fallback, then records `iconUrl || null` for whatever it did not find. That null goes into the same Redux map. So an asset whose icon exists only in its issuer's stellar.toml gets marked "no icon" by a lookup that never tried the toml, and this change made the Account view believe it. The asset then renders iconless for the rest of the session, which is a session-scoped version of exactly the latched failure this PR exists to fix. A transient error inside getIconUrlFromIssuer latches the same way. The repeat lookups this was meant to avoid are pre-existing and not introduced by this PR: getAssetIcons records nothing for an asset it skips, so a cached null and an absent entry are indistinguishable to this hook and both already triggered the same lookup. Leaving that as it was. Doing this properly means stopping the history path from recording a negative verdict for a lookup it deliberately skipped, which is a change to another view and belongs outside a hotfix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts:21
- This new TODO is not linked to a tracked issue, so the acknowledged cache-design follow-up can be lost after this PR merges. Please reference a GitHub issue in the TODO (for example,
TODO(#1234): ...) or remove the TODO and track the work elsewhere.
// TODO: this is a read-side workaround, not the real fix. It hides bad data
| // TODO: this is a read-side workaround, not the real fix. It hides bad data | ||
| // rather than stopping it being written, and it permanently rules out ever | ||
| // storing a legitimate null here — which we may well want, since an asset | ||
| // with genuinely no icon currently re-runs the whole lookup chain (token | ||
| // lists, then Horizon, then the issuer's stellar.toml) on every cold popup | ||
| // open. A durable negative cache with a TTL is the sane answer to that, and | ||
| // this filter would silently swallow it. |
There was a problem hiding this comment.
What is a "cold popup open"? What is the consequence of re-running the lookup chain for nulls besides USDT0?
There was a problem hiding this comment.
"cold popup open" just means opening the extension for the first time (so no ephemeral data, like balances, has been cached). I can make this wording a bit more clear.
The consequence of re-running the lookup chain is just a couple of seconds of the loading icon state as we retry the icon (non-blocking). Previously, we wouldn't bother retrying and just say "this previously was not found" and show the default icon
The earlier note claimed the repeat lookup "already happens today via useGetIcons, so filtering here does not add it". That is true of the Account view's icon hook, which passes an empty cache to its lookup pass and so retried either way — and false of the flows that load balances with icons (swap, send, manage assets, history), where a null-marked asset was skipped outright and now is not. Says instead that retrying is the point, since it is how a stuck asset recovers, and is honest that it is newly paid in those flows and never terminates for an asset with no icon anywhere. Names the per-attempt cost, and why a negative cache wants a TTL rather than the latch we just removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Every popup open" only explains itself if you already know the popup is destroyed when it closes. Spell that out: closing it takes Redux with it, and Redux is the only record of what the session had already resolved, so each open starts from nothing. Follows removing "cold popup open", which was invented jargon and read as though there were some warmer kind of open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts:21
- Please link this substantial follow-up to a tracking issue (for example,
TODO(#NNNN): ...). The repository guidance requires TODOs to carry an issue reference so the acknowledged persistent retry cost and cache redesign are not lost.
// TODO: this is a read-side workaround, not the real fix. It hides bad data
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
extension/src/background/messageListener/handlers/getCachedAssetIconList.ts:58
- This removes negative caching on every read, not just for stale upgrade data. As the new comment above notes, any genuinely iconless asset will now repeat token-list, Horizon, and possibly
stellar.tomllookups each time a popup flow loads icons. That adds recurring latency and third-party traffic across swap, send, manage-assets, and history. Please retain an expiring negative-cache value (and separately invalidate legacy permanent nulls) so transient failures recover without making permanent misses retry on every popup open.
icons: Object.fromEntries(
Object.entries(assetIconCache).filter(([, iconUrl]) => iconUrl),
* v5.48.0 * fix(icons): show USDT0's icon, for new and existing users (v5.48.0) (#2998) Backport of #2994 onto the v5.48.0 release branch. Cherry-picked from bbcd41d; the four touched files are byte-identical to master. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * feat: add USDT0 launch banner and promo sheet to Home (v5.48.0 backport) (#2997) * feat: add USDT0 launch banner and promo sheet to Home (v5.48.0) Backport of #2990 onto the v5.48.0 release branch so the promo can ship with that release. Applied as a single squashed commit; the tree for the 17 touched files is identical to the master-based branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: fail closed on USDT0 banner dismissal lookup, announce the dialog Backport of #2999 onto the v5.48.0 release branch, addressing the review comments left on #2990 after it merged. The three touched files match the master-based fix branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Cássio Marcos Goulart <3228151+CassioMG@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
TL;DR
USDT0 shows no icon in the wallet, and once it has failed for a user it never comes back — not on reload, not on restart.
Two things cause it. USDT0 is carried by two of the default asset lists: Soroswap, with a direct HTTPS icon, and LOBSTR, with an
ipfs.iogateway URL. That gateway serves the image tocurlbut returns 403 to a browser User-Agent, so an<img>can never load it. Icon lookup was keeping whichever list it read last, which handed the browser the one URL it cannot fetch. And when that image failed, the failure was recorded to disk as "this asset has no icon", which lookup reads as "already tried, don't look again" — a verdict that was only ever meant to last a session.This makes the first matching list win, since the list order is the user's priority order, and makes the cache stop reporting those stale "no icon" records. Users already stuck recover on their first load after upgrading — no migration needed: the fresh lookup overwrites the old entry.
Four lines of production code.
Verifying
master, holdUSDT0:GATISXX6BZ6NC7IKQBY37CJD4SOZL3CYZJWXEDG6JVIY4WBS6KXJHN6Qon Mainnet and open the account view — no icon, and it stays missing across reloads.null"https://docs.usdt0.to/downloads/usdt0/Symbol_USDT0_Secondary.png"— the stale record is gone from disk.Confirmed end to end in a local build against the prod backend.
Implementation details (for agents)
Root cause
getIconFromTokenListsiterated the lists in order, but itsbreakonly exited the inner record loop. The outer loop over lists ran to completion, so the last matching list overwrote every earlier match. Ordering is meant to be priority (stellar.expert → Soroswap → LOBSTR), so USDT0 resolved to LOBSTR'sipfs.ioURL instead of Soroswap'sdocs.usdt0.to.https://ipfs.io/ipfs/bafkreifaalohkkikosp27qp6qczwaffwseejvoaafkgv7ahyrqvejmpqoureturns 200 to curl and 403 to a browser User-Agent — isolated to the UA alone, other headers make no difference. So the<img>always failed.onError→retryAssetIconwritesiconUrl: nulltoCACHED_ASSET_ICONS_ID, then consults onlygetIconUrlFromIssuer(home domain → SEP-1 TOML).home_domain: nullfor the issuer, withthresholds {0,0,0}and a single signer at weight 0 — the account is permanently locked and can never set one. A validstellar.tomlwith the rightimagedoes exist atusdt0.to, but the wallet has no way to discover that domain.getAssetIconsskips any asset whose cached value isnull, with a comment saying the null "is only stored in Redux, so we will re-try on next app reload". It wasn't —retryAssetIconpersisted it, so the skip outlived the session. With no icon there is no<img>, so nothing fires theonErrorthat would have retried: the asset could not recover on its own.What changed
Label the outer loop and break out of it on the first match:
freighter/@shared/api/helpers/getIconFromTokenList.ts
Lines 20 to 55 in 6701092
Leave null entries out when reading the persisted icon cache:
freighter/extension/src/background/messageListener/handlers/getCachedAssetIconList.ts
Lines 1 to 24 in 6701092
Filtering on read rather than deleting on write is what makes this a four-line fix instead of a storage migration: a stale entry becomes an ordinary cache miss, and
getIconFromTokenListsalready writes what it finds, so the fresh lookup overwrites it on disk. It also makesretryAssetIcon's existingiconUrl: nullmessage finally mean what it intended — clear this entry — with no change to that handler.The in-session memo is deliberately untouched. It lives in Redux (
saveIconsForBalances), which is wheregetAssetIcons' comment always claimed it lived; filtering at the storage boundary separates the two cleanly.Verification
tsc --noEmitclean — identical tomaster.null→ icon renders → stored value replaced with the working URL.Follow-ups / out of scope
ipfs.ioURL that no browser can load. Worth reporting upstream — ordering saves us here only because Soroswap happens to list USDT0 too. It is the only such entry across all three default lists today (stellar.expert serves everything frommeta.stellar.expert, all browser-loadable).AssetIconlatches its error state. Once an image fails, the row keeps the broken-image glyph for the life of the component even if a working URL arrives, so a successful retry is invisible until the popup is reopened. Pre-existing and unrelated to these two lines, but it is why the existing retry path appears to do nothing.