MetaMask / MetaMask/metamask-mobile
Stabilize erc20TokenAddresses instead of JSON.stringify in async deps
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
> **Performance audit finding** · Severity: **Medium** · Effort: Easy · Fix risk: Simple · Test safety net: Covered (app/components/UI/SimulationDetails/useBalanceChanges.test.ts)
> Owner: `@MetaMask/confirmations (suggested)`
> File: `app/components/UI/SimulationDetails/useBalanceChanges.ts:262`
### What is this about?
`erc20TokenAddresses` is recomputed inline each render (`.filter(...).map(...)` at lines 252-258) producing a new array reference, then three `useAsyncResultOrThrow` calls pass `JSON.stringify(erc20TokenAddresses)` as their dependency. The array is serialized up to three times every render purely to derive stable deps.
**Why it matters**
`useBalanceChanges` re-renders frequently during confirmations (fiat rate, currency, decimals updates). Each render performs the filter/map plus up to three `JSON.stringify` passes over the address list. This is wasted JS-thread work and is the classic stringify-as-dep band-aid; the addresses array itself should be memoized so the async hooks can depend on a stable reference.
### Scenario
N/A — see Technical Details.
### Design
N/A — internal performance change; no UI/design impact.
### Technical Details
**Evidence**
`app/components/UI/SimulationDetails/useBalanceChanges.ts:260`
```ts
const erc20Decimals = useAsyncResultOrThrow(
() => fetchAllErc20Decimals(erc20TokenAddresses, networkClientId),
[JSON.stringify(erc20TokenAddresses)],
);
const erc20FiatRates = useAsyncResultOrThrow(
() => fetchTokenFiatRates(fiatCurrency, erc20TokenAddresses, chainId),
[JSON.stringify(erc20TokenAddresses), chainId, fiatCurrency],
);
```
(plus the third call at line 270 with the same pattern.)
**Fix**
Memoize `erc20TokenAddresses` once with `useMemo(() => tokenBalanceChanges.filter(...).map(...), [tokenBalanceChanges])` and a stable join key (e.g. `useMemo(() => addresses.join(','), [addresses])`). Use that join-key string (or the memoized array) as the dependency for all three async hooks, computing the key a single time per change instead of stringifying on every render.
### Threat Modeling Framework
N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.
### Acceptance Criteria
- - Run `yarn jest app/components/UI/SimulationDetails/useBalanceChanges.test.ts` to confirm fetches still re-run when the address set changes and not otherwise.
- Add an assertion that `fetchAllErc20Decimals` is not re-invoked across renders with an identical address set.
### References
- File: `app/components/UI/SimulationDetails/useBalanceChanges.ts:262`
- Source: MetaMask Mobile performance audit — finding `hookdeps-usebalancechanges-jsonstringify`
- Owner (CODEOWNERS / best-effort): @MetaMask/confirmations (suggested)
- Status: **UNVALIDATED**
Contributor guide
Research direction
Start in app/components/UI/SimulationDetails/useBalanceChanges.ts around lines 252-270 and inspect how erc20TokenAddresses feeds the three async hooks. Run yarn jest app/components/UI/SimulationDetails/useBalanceChanges.test.ts, then add the requested assertion for identical address sets. Done means fetches rerun when the address set changes but not across identical renders, with the existing tests passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react-native, typescript
- Domain
- performance, testing-qa
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100