MetaMask / MetaMask/metamask-mobile
Narrow getStateForAssetSelector inputs: whole-controller spreads over-subscribe selectAssetsBySelectedAccountGroup, re-deriving all assets on asset-irrelevant controller churn
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
> **Performance audit finding** · Severity: **Sev3** (corrected from High — see below) · Effort: Low–Medium · Fix risk: Low (behavior-preserving input narrowing) · Test safety net: Partial
> Owner: `@MetaMask/metamask-assets (suggested)`
> File: `app/selectors/assets/assets-list.ts:107,157`
### What is this about?
**Correction to the original framing.** This ticket previously claimed the cost was a per-check **deep compare of the entire asset state** that "scales with power-user data." That mechanism does not hold up under inspection:
- `createDeepEqualSelector` uses **`fast-equals` `deepEqual`** (`app/selectors/util.ts:30`), not lodash. `deepEqual` **short-circuits on reference-equal sub-values** (an `a[key] === b[key]` fast path per key) and **bails on the first difference**. A fresh wrapper object does *not* force a full traversal.
- The bulk slices in the composite — `allTokens`, `tokenBalances`, `marketData`, `balances`, `conversionRates`, `assetsMetadata`, `accountsAssets`, … — are each produced by a `createDeepEqualSelector` in `assets-migration.ts`, so they are **reference-stable** when unchanged. `deepEqual` hits `===` on those keys and never descends into the thousands-of-entries token/balance subtrees.
So the per-check compare is ~O(number of top-level composite keys) pointer comparisons, **not** O(asset count). The deep-compare cost is not the problem.
**The real issue is input over-subscription → unnecessary result recomputation.** `getStateForAssetSelector` (`:107`) spreads three *whole* controllers into the composite:
```ts
return {
...AccountTreeController, // 5 fields; consumer reads only `accountTree`
...AccountsController, // ≈ internalAccounts (consumed)
/* … stabilized get* slices: allTokens, tokenBalances, marketData, … … */
...NetworkController, // 3 fields; consumer reads only `networkConfigurationsByChainId`
accountsByChainId,
};
```
But the upstream consumer `_selectAssetsBySelectedAccountGroup` reads a narrow `AssetListState` (`@metamask/assets-controllers`, `selectors/token-selectors.ts:94`) — from those three controllers it uses **only** `accountTree`, `internalAccounts`, and `networkConfigurationsByChainId`. The spreads drag every other field into the deep-equal'd composite, where **any change to an extraneous field flips `deepEqual` to `false` and re-runs the full asset derivation** — producing a new output reference that cascades to the 15+ dependent selectors and the per-row `selectAsset` (#31492).
The extraneous fields are exactly the ones that churn on background timers, unrelated to any balance/price change:
- `NetworkController.networksMetadata` — network availability / EIP-1559 status polling (per enabled network).
- `NetworkController.selectedNetworkClientId` — changes on every network switch.
- `AccountTreeController.isAccountTreeSyncingInProgress` — a boolean that **toggles on every account-tree sync cycle** (≥2 `deepEqual=false` flips per sync).
- `AccountTreeController.hasAccountTreeSyncingSyncedAtLeastOnce`, `accountGroupsMetadata`, `accountWalletsMetadata` — sync / metadata churn.
Each of these, while an asset surface is mounted, triggers a full asset re-derivation + a per-row `selectAsset` cascade across every visible token-list row — even though no token, balance, or price changed.
**Component footprint** (the cascade blast radius — GitHub code search, two hops from the root):
- Direct consumers: `useTokensWithBalance` (Bridge), `useTokenAtomicActions` (TokenDetails), and the send flow's `useAccountTokens`/`useRouteParams`.
- Via `selectSortedAssetsBySelectedAccountGroup`: the wallet token list itself (`app/components/UI/Tokens/index.tsx` — which renders `TokenListItem` per row, where `selectAsset` adds the per-row multiplier of #31492; 26 component-side `selectAsset` references) and the Homepage tokens section.
- Via the `useTokensWithBalance` hook alone: 29 files — Bridge token selection and batch-sell, Card (asset balances, swaps, spending limit), Perps payment tokens, and the leaderboard QuickBuy flow.
Net: roughly **50 unique non-test files across 8 product surfaces** (wallet home, homepage, token details, send, bridge, card, perps, quick-buy). On each spurious recompute the token list multiplies the per-row `selectAsset` re-derivation by every visible row.
### Scenario
N/A — see Technical Details.
### Design
N/A — internal performance change; no UI/design impact.
### Technical Details
**Fix direction** — narrow the three spreads to the exact consumed fields; leave the already-stabilized `get*` slices untouched:
```ts
const getStateForAssetSelector = (state: RootState) => {
const { AccountTreeController, AccountsController, NetworkController } =
state.engine.backgroundState;
return {
accountTree: AccountTreeController.accountTree,
internalAccounts: AccountsController.internalAccounts,
allTokens: getTokensControllerAllTokens(state),
allIgnoredTokens: getTokensControllerAllIgnoredTokens(state),
tokenBalances: getTokenBalancesControllerTokenBalances(state),
marketData: getTokenRatesControllerMarketData(state),
assetsMetadata: getMultiChainAssetsControllerAssetsMetadata(state),
accountsAssets: getMultiChainAssetsControllerAccountsAssets(state),
allIgnoredAssets: getMultiChainAssetsControllerAllIgnoredAssets(state),
balances: getMultiChainBalancesControllerBalances(state),
conversionRates: getMultichainAssetsRatesControllerConversionRates(state),
currencyRates: getCurrencyRateControllerCurrencyRates(state),
currentCurrency: getCurrencyRateControllerCurrentCurrency(state),
networkConfigurationsByChainId:
NetworkController.networkConfigurationsByChainId,
accountsByChainId: getAccountTrackerControllerAccountsByChainId(state) as /* … */,
};
};
```
After this, the composite's only recompute triggers are genuine asset-relevant changes; `networksMetadata` / `selectedNetworkClientId` / account-tree sync-flag churn no longer invalidates it.
**Keep `createDeepEqualSelector` — do *not* switch to a plain `createSelector`.** The three narrowed fields (`accountTree`, `internalAccounts`, `networkConfigurationsByChainId`) are raw state reads, not deep-equal-stabilized, so a reducer that replaces one with a new-but-deeply-equal reference would force a recompute under a plain selector; `deepEqual` absorbs that. (This is the opposite of the original ticket's suggestion to drop the deep-equal wrapper, which would *regress* recompute frequency on these surfaces.)
**Verify before narrowing** that `_selectAllAssets` (via `selectAllAssetsGrouped`) and the Tron path (`selectTronSpecialAssetsBySelectedAccountGroup`) consume the same `AssetListState` and read no field beyond it.
### Threat Modeling Framework
N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.
### Acceptance Criteria
- Dispatching a `NetworkController.networksMetadata` update (or toggling `AccountTreeController.isAccountTreeSyncingInProgress`) with token/balance/price data unchanged does **not** increment `selectAssetsBySelectedAccountGroup.recomputations()`.
- Recompute count on `selectAssetsBySelectedAccountGroup` increments ~once per actual asset-state change (token add/remove, balance, price, account-tree structure), **not** per background poll/sync.
- Profiler on a power-user profile: token list / asset surfaces stop re-deriving during network-status polling and account-tree sync; re-measure #31492 after this lands.
### References
- Files: `app/selectors/assets/assets-list.ts:107` (input), `:157` (wrapper); `app/selectors/util.ts:30` (`createDeepEqualSelector` = `fast-equals` `deepEqual`); `@metamask/assets-controllers` `selectors/token-selectors.ts:94` (`AssetListState` — the narrow consumed-field set)
- Downstream: #31492 (per-row `selectAsset` — the recompute-cascade target, not a deep-compare cost)
- Pattern: **input over-subscription** (selector subscribes to whole controllers but reads a few fields), *not* the deep-compare cost originally claimed. Distinct failure shape from #31499.
- Source: `mms-performance` dependency-tree mapping (MetaMask/skills#49, `mm-selector-cascade`)
- Status: **UNVALIDATED** — mechanism corrected and verified against source; magnitude (background-trigger frequency × re-derivation cost while asset surfaces are mounted) still needs profiler confirmation.
Contributor guide
Research direction
Start in app/selectors/assets/assets-list.ts at getStateForAssetSelector and inspect AssetListState in @metamask/assets-controllers selectors/token-selectors.ts:94, including the _selectAllAssets and Tron paths. Verify the narrowed inputs against those consumers, then measure selector recomputations and profiler behavior while changing networksMetadata or the account-tree sync flag; done means those irrelevant updates no longer trigger recomputation while genuine asset changes still do.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- mobile-dev, performance
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 52/100