fix(assets-controller): clean up unused assetsInfo/assetsPrice entries on startup

[Prithpal-Sooriya]

Explanation

AssetsController's assetsInfo and assetsPrice state slices have never had a delete path: metadata and prices are written whenever assets are fetched or detected, but entries are never removed when the assets stop being referenced. Since both slices are persisted, they grow unbounded over time.

This PR adds a cleanupUnusedMetadata utility (src/utils/cleanupUnusedMetadata.ts) that deletes every assetsInfo / assetsPrice entry whose asset ID is not:

• a key in state.assetsBalance[accountId] for any account in state (key presence is what counts — addCustomAsset and the native / default-asset seeders write { amount: '0' } entries for held assets, and scanning all accounts avoids wiping metadata for non-selected accounts),
• listed in state.customAssets for any account,
• a native asset ID (recognized via isNativeAssetId in utils/native-assets.ts: the slip44 namespace, the zero-address ERC-20 convention, and the hardcoded native asset registry that covers non-standard natives like METIS/MNT),
• a default tracked asset (DEFAULT_TRACKED_ASSETS_BY_CHAIN) — these often have metadata but no balance (e.g. mUSD on Monad is pre-seeded into assetsInfo while its zero balance is only written once the chain is enabled), and deleting them would be permanent since re-seeding is gated behind chain enablement.

All comparisons are case-insensitive because state keys are checksummed while some sources emit lowercase IDs. assetPreferences is deliberately left untouched so an asset a user hid stays hidden if it ever comes back (the cleanup's parameter type omits that slice entirely, enforcing this at compile time).

The controller invokes the cleanup at the end of #runStartupRefresh via this.update((state) => cleanupUnusedMetadata(state)), only after a successful refresh — on failure the balance picture may be incomplete, so no pruning happens. Stale entries are therefore pruned once per successful startup/unlock after balances have been refreshed and defaults re-seeded. BaseController.update skips publishing when nothing changed, so a no-op cleanup does not emit a state change.

References

N/A

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've communicated my changes to consumers by updating changelogs for packages I've changed
• I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

  

[Prithpal-Sooriya]

@cursor please remove type assetion

[cursor]

The type assertion is removed — pushed as d05dff6 on the PR. Both call sites now read exactly this.update((state) => cleanupUnusedMetadata(state)); with no cast.

Rather than moving the assertion somewhere else, I removed the need for it. The root cause was that immer's WritableDraft<AssetsControllerState> isn't assignable to the full state type (the Json-typed metadata values diverge under Draft). But cleanupUnusedMetadata only ever inspects keys — it never reads a metadata, price, or balance value — so I retyped its parameter as a minimal structural contract, CleanupUnusedMetadataState, whose slice values are unknown:

export type CleanupUnusedMetadataState = {
  /** Shared metadata for all assets, keyed by CAIP-19 asset ID. */
  assetsInfo: Record<string, unknown>;
  /** Per-account balances, keyed by account ID then CAIP-19 asset ID. */
  assetsBalance: Record<string, Record<string, unknown>>;
  /** Price data for assets, keyed by CAIP-19 asset ID. */
  assetsPrice: Record<string, unknown>;
  /** Custom assets added by users per account (CAIP-19 asset IDs). */
  customAssets: Record<string, readonly string[]>;
};

Both AssetsControllerState and the update callback's draft satisfy this directly, so the controller passes the draft as-is. Two bonus effects: the two internal delete state.assetsInfo[assetId as Caip19AssetId] casts became plain delete state.assetsInfo[assetId] (three assertions removed in total, zero remain in the util or its call sites), and omitting assetPreferences from the type now enforces at compile time that the cleanup can't touch it — previously that was only a documented convention.

Verified after the change: the package build (its authoritative typecheck) passes, the full package test suite passes with no failures, and ESLint plus formatting checks are clean on the touched files. No behavior changed, so the changelog entry and PR description remain accurate.

  

[Prithpal-Sooriya]

@cursor, we don't ned to cleanup on failures thats overkill

[cursor]

Done — the cleanup no longer runs on the failure path. Pushed as 2dbf926 on PR #9806.

#runStartupRefresh now calls cleanupUnusedMetadata only at the end of the try branch, after a successful refresh; the catch branch still re-seeds natives/defaults and re-subscribes but does no pruning. Beyond avoiding needless work, this is also the safer ordering: if the startup fetch failed, the balance picture in state may be incomplete, and pruning against it could delete metadata for assets that are actually still held — I left a short comment in the code capturing that rationale.

Verification: the full package test suite passes (the controller integration test, which asserts cleanup happens after startup, goes through the success path and still passes), and ESLint plus formatting checks are clean. The changelog entry ("clean up … on startup, at the end of the startup refresh") is still accurate as written, so only the PR description needed updating — it previously said the cleanup ran "in both the try and catch branches", and now reflects success-only behavior.

  

[Prithpal-Sooriya]

@cursor we don't need these comments

[cursor]

Done — the three-line comment above the cleanup call is removed, pushed as 26d01e4. The try branch of #runStartupRefresh now ends with the bare this.update((state) => cleanupUnusedMetadata(state)); call, and the success-only rationale remains documented in the PR description instead of the code. The pre-existing "Seed before subscribe…" comment a few lines up predates this PR, so I left it alone.

ESLint, the formatting check, and the controller integration test covering the startup cleanup all pass after the change.

With this, every open review comment has been addressed across the seven threads: type assertion removal, success-path-only cleanup, the cleanupUnusedMetadata.ts condensing (comments, deduped types, flatMap, combined sets), both test files converted to parameterized tables, the shortened changelog entry, the native-assets.ts comment trim, and this comment removal.

  

[Prithpal-Sooriya]

@cursor keep change log short

[cursor]

Done — the changelog entry is now a single line, pushed as 06154e5.

The nested detail bullet listing all the keep-rules is gone, and the main line was tightened to:

- Clean up unused `assetsInfo` and `assetsPrice` entries after a successful startup refresh so those persisted state slices no longer grow unbounded ([#9806](https://github.com/MetaMask/core/pull/9806))

While trimming I also updated "on startup" to "after a successful startup refresh", so the shortened entry reflects your earlier change that removed cleanup from the failure path. The full keep-rule details remain available in the PR description and the function's JSDoc for anyone who needs them. yarn changelog:validate and the formatting check both pass.

  

[Prithpal-Sooriya]

@cursor consider a testing table it.each([{}])

[cursor]

Done — the cleanup tests are now table-driven, pushed as 06bda7f (net −75 lines on the file).

The core of the suite is one it.each(cleanupCases) over 15 typed rows of { description, assetId, references?, expectKept }. Each row seeds the asset into assetsInfo and assetsPrice, applies its reference slices (balance entries, customAssets, or none), runs the cleanup, and strict-asserts both slices are either exactly the seeded entry or empty. The rows cover: unreferenced, malformed, and NFT IDs (removed); non-zero balance, zero-amount balance, non-selected-account balance, customAssets-only, lowercase balance key, lowercase customAssets entry, referenced-malformed ID, the four native forms, and mUSD-on-Monad (all kept). Flattening into rows actually added cases — "non-zero balance" and the two casing variants were previously bundled inside multi-assertion tests.

Four scenarios don't fit the one-asset row shape and stayed as standalone tests: selective removal within a mixed state (referenced and unreferenced entries side by side), removal of a price entry that has no assetsInfo counterpart, assetPreferences staying untouched, and the empty-state no-op.

While in the file I also trimmed the constant and helper docblocks to match the terser style you've asked for elsewhere, and the lowercase custom-asset cast now uses the named Caip19AssetId type instead of the inline template-literal type.

Verified: 19 tests pass (up from 12) with 100% statement and branch coverage on cleanupUnusedMetadata.ts, the full package suite passes, and ESLint plus formatting checks are clean.

  

[Prithpal-Sooriya]

@cursor

• remove comments
• remove duplicate types (promote reuse)
• consider higher order array functions (e.g. flatmap) to make code more consise

[cursor]

All three bullets (plus the same-file "combine sets" note) are addressed — pushed as 5029578. The file shrank from 66 lines to 46, and the diff is net −38 lines.

Remove comments. All four property comments, the module-constant comment, and the body comment are gone. The two JSDoc blocks were cut to a fraction of their size: the function doc is now one sentence plus the @param, and the type doc is three lines. I kept that type doc deliberately — it explains why the slice values are unknown, which is the mechanism that keeps the call sites assertion-free per your earlier comment; without it, a future cleanup would likely "fix" the type back to the real state shape and reintroduce the cast.

Remove duplicate types. The three identical inline Record<string, unknown> types are deduped into a single AssetIdKeyedRecord alias, and customAssets now reuses the real state type via AssetsControllerStateInternal['customAssets'] instead of a hand-rolled Record<string, readonly string[]>.

Higher-order array functions + combined sets. The two nested for-loop set builders and the separate module-level default-tracked set are now one expression building a single keptAssetIds set — defaults via .flat(), balances via .flatMap(Object.keys), custom assets via .flat(), all lowercased in one .map. That also resolves your line-71 note: isUnused is down to two checks (!keptAssetIds.has(...) && !isNativeAssetId(...)). The two deletion loops are merged into one loop over [state.assetsInfo, state.assetsPrice] using .filter(isUnused).

Behavior is unchanged: all 12 utility tests pass with 100% coverage on the file, the package build (its typecheck, which also proves the cast-free controller call sites still compile against the reworked type) passes, the full package suite passes, and ESLint plus formatting are clean.

Per the thread focus I only touched cleanupUnusedMetadata.ts here — your comments on the test tables, changelog length, and the comments in native-assets.ts / AssetsController.ts are separate threads I'll handle in their own turns.

  

[Prithpal-Sooriya]

@cursor just combine sets to reduce Boolean check noise

[cursor]

Already done — this comment was folded into commit 5029578 from the earlier round of feedback on this file, and it's pushed on the branch. No further edits were needed this turn.

The line this comment anchored to (!DEFAULT_TRACKED_ASSET_IDS.has(assetId.toLowerCase()) &&) no longer exists. The separate module-level default-tracked set was merged with the balance keys and custom asset IDs into a single combined set, all lowercased in one pass:

  const keptAssetIds = new Set(
    [
      ...[...DEFAULT_TRACKED_ASSETS_BY_CHAIN.values()].flat(),
      ...Object.values(state.assetsBalance).flatMap((accountBalances) =>
        Object.keys(accountBalances),
      ),
      ...Object.values(state.customAssets).flat(),
    ].map((assetId) => assetId.toLowerCase()),
  );
  const isUnused = (assetId: string): boolean =>
    !keptAssetIds.has(assetId.toLowerCase()) && !isNativeAssetId(assetId);

isUnused is down from three boolean checks to two. That's the floor: the native check can't be folded into the set because it's rule-based rather than enumerable — the slip44 namespace and zero-address conventions match IDs on unbounded chain IDs, so there's no finite set of native IDs to precompute (only the hardcoded registry portion lives in a set, inside isNativeAssetId).

All 12 utility tests pass with 100% coverage on this file, and the combined-set version is what the full-suite, build, lint, and formatting checks already validated when it was pushed.

  

[Prithpal-Sooriya]

@cursor Consider test table

[cursor]

Done — the isNativeAssetId tests are now a parameterized table, pushed as 684370c.

The five it blocks became a single it.each with eight rows in the { description, assetId, expected } object style, titled returns $expected for $description. Each row corresponds to a distinct branch of the implementation, so nothing was lost in the flattening: slip44 on a registry chain, slip44 outside the registry (Solana, which exercises the parse path rather than the registry short-circuit), zero-address ERC-20 on an unregistered chain, the METIS dead-address registry case in both casings (pinning case-insensitivity), a regular ERC-20, a malformed ID, and an NFT ID with a token ID.

Verified: the suite now reports 22 passing tests for the file (8 table rows plus the 14 pre-existing builder tests) with 100% statement and branch coverage on native-assets.ts, and ESLint plus formatting checks are clean.

Per the thread focus this turn only touched native-assets.test.ts; the analogous table suggestion on cleanupUnusedMetadata.test.ts is a separate thread I'll handle in its own turn.

  

[Prithpal-Sooriya]

@cursor too many comments, remove

[cursor]

Done — pushed as f6967a4. The isNativeAssetId section went from 55 lines to 29 (net −25 on the file).

Rather than only deleting comment text, I removed what the comments were compensating for. The lazy-init machinery — a documented let knownNativeAssetIds plus a documented getKnownNativeAssetIds() helper — collapsed into one uncommented, self-explanatory const KNOWN_NATIVE_ASSET_IDS (eager init is harmless here; the controller already builds the same registry at construction). That eliminated two JSDoc blocks outright. The remaining isNativeAssetId doc shrank from thirteen lines to a one-sentence contract naming the three recognition rules, plus the @param/@returns tags the lint setup expects on existing blocks. I also condensed the body itself: the parse result is destructured and the trailing two checks merged into a single return expression. The pre-existing docs on the two builder functions weren't touched since they're outside this PR's diff.

Verification: all 34 tests across native-assets.test.ts and cleanupUnusedMetadata.test.ts pass with 100% statements, branches, functions, and lines on native-assets.ts. One note for transparency: the first coverage run after the edit briefly reported phantom uncovered lines — I had run the auto-formatter in parallel with jest, so the two suites read different versions of the file mid-rewrite; a rerun on the stable file is fully green. The package build (typecheck), the full package suite, ESLint, and formatting checks all pass.