MetaMask / MetaMask/metamask-mobile

Replace JSON.stringify(params) effect dep in useSimulationMetrics

Open Beginner friendly
#31,299 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: **Medium** · Effort: Easy · Fix risk: Simple · Test safety net: Covered (app/components/UI/SimulationDetails/useSimulationMetrics.test.ts)
> Owner: `@MetaMask/confirmations (suggested)`
> File: `app/components/UI/SimulationDetails/useSimulationMetrics.ts:113`

### What is this about?

The metrics-dispatch effect uses `JSON.stringify(params)` as a dependency. `params` is rebuilt inline every render (`const params = { properties }` at line 97, where `properties` spreads several freshly-computed objects), so `JSON.stringify` runs on every render to produce the dep value, and the cost scales with the number/size of balance-change assets.

**Why it matters**

`JSON.stringify` over the full properties object executes on every render of the confirmation screen, regardless of whether anything changed. On large simulations (many assets) this is a non-trivial per-render serialization cost on the JS thread, and it is the kind of "stringify-as-dep" band-aid that hides the real fix (memoizing `params`/`properties`).

### Scenario

N/A — see Technical Details.

### Design

N/A — internal performance change; no UI/design impact.

### Technical Details

**Evidence**

`app/components/UI/SimulationDetails/useSimulationMetrics.ts:106`
```ts
useEffect(() => {
if (shouldSkipMetrics) {
return;
}
dispatch(updateConfirmationMetric({ id: transactionId, params }));
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [shouldSkipMetrics, transactionId, JSON.stringify(params), dispatch]);
```
`params` source at line 97:
```ts
const params = { properties };
```

**Fix**

Memoize `properties` (and thus `params`) with `useMemo` keyed on `simulationResponse`, `simulationLatency`, `receivingAssets`, and `sendingAssets`, then depend on the memoized `params` object reference directly in the effect. This removes the per-render `JSON.stringify` while keeping the effect firing only when the underlying metrics actually change.

### 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/useSimulationMetrics.test.ts` and confirm `updateConfirmationMetric` dispatch count/payload assertions are unchanged.
- Confirm with a render-count probe that the effect no longer re-runs on renders where the metrics inputs are identical.

### References

- File: `app/components/UI/SimulationDetails/useSimulationMetrics.ts:113`
- Source: MetaMask Mobile performance audit — finding `hookdeps-usesimulationmetrics-jsonstringify`
- Owner (CODEOWNERS / best-effort): @MetaMask/confirmations (suggested)
- Status: **UNVALIDATED**

Contributor guide

Open the contributing guide

Research direction

Start with app/components/UI/SimulationDetails/useSimulationMetrics.ts, especially the params construction and metrics-dispatch effect. Run yarn jest app/components/UI/SimulationDetails/useSimulationMetrics.test.ts before and after the change. Done means the test assertions remain unchanged and the effect does not rerun when its metrics inputs are identical.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.