MetaMask / MetaMask/metamask-mobile

Stabilize erc20TokenAddresses instead of JSON.stringify in async deps

Open Beginner friendly
#31,296 1 comment 0 reactions 0 assignees View on GitHub
area-performance Sev3 size-S ta-ai-fixable ta-triaged team-confirmations
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.