MetaMask / MetaMask/metamask-mobile

Memoize useBalanceChanges balanceChanges return array

Open Beginner friendly
#31,374 1 comment 0 reactions 0 assignees View on GitHub
area-performance Sev3 size-S 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: **High** · 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:307`

### What is this about?

`useBalanceChanges` constructs a fresh `balanceChanges` array (and fresh `{ pending, value }` wrapper) on every render and returns it without `useMemo`. Any consumer that depends on the returned `value` array reference — including `useSimulationMetrics`, which derives a stringified-properties effect from it — sees a new reference each render.

**Why it matters**

SimulationDetails renders on every confirmation and re-renders as fiat rates, decimals, and currency selectors update. Because `value` is a new array each render, downstream memoization (e.g. `useMemo`/`React.memo` over balance changes, and the `JSON.stringify(params)` effect in `useSimulationMetrics`) cannot short-circuit, causing repeated recomputation and metric dispatches.

### 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:307`
```ts
const balanceChanges: BalanceChange[] = [
...(nativeChange ? [nativeChange] : []),
...tokenChanges,
];

return { pending: false, value: balanceChanges };
```
`nativeChange` and `tokenChanges` are themselves recomputed inline each render (`getNativeBalanceChange`, `getTokenBalanceChanges` at lines 292 and 299) outside any memo.

**Fix**

Compute `balanceChanges` (and the returned object) inside a `useMemo` keyed on the resolved async values and inputs: `nativeBalanceChange`, `tokenBalanceChanges`, `erc20Decimals.value`, `erc20FiatRates.value`, `erc20UsdRates.value`, `nativeFiatRate`, `nativeUsdRate`, `chainId`. Return a stable `{ pending, value }` object. Note the early-return `{ pending: true, value: [] }` should also use a shared frozen empty-array constant so the pending state is reference-stable too.

### 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`.
- Add a test that renders the hook twice with identical inputs and asserts `result.value` is `===` across renders.

### References

- File: `app/components/UI/SimulationDetails/useBalanceChanges.ts:307`
- Source: MetaMask Mobile performance audit — finding `unstablehook-usebalancechanges-fresh-array`
- Owner (CODEOWNERS / best-effort): @MetaMask/confirmations (suggested)
- Status: **UNVALIDATED**

Contributor guide

Open the contributing guide

Research direction

Start with app/components/UI/SimulationDetails/useBalanceChanges.ts at the referenced lines, then read app/components/UI/SimulationDetails/useBalanceChanges.test.ts. Run the named Jest test before changing anything and add the specified identical-input rerender assertion. Done means the hook returns the same value reference across unchanged renders while the existing tests continue to pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
react-native, typescript
Domain
frontend, mobile, performance
Issue type
Refactor
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.