MetaMask / MetaMask/metamask-mobile
Replace JSON.stringify(params) effect dep in useSimulationMetrics
- 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
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