MetaMask / MetaMask/metamask-mobile

Replace O(n^2) reduce-with-spread over address book in useExistingAddress

Open Beginner friendly
#31,378 1 comment 0 reactions 0 assignees View on GitHub
area-performance Sev3 size-XS ta-triaged team-confirmations team-mobile-platform
Dominant language
TypeScript
Stars
3k
Forks
1.7k
Avg merge
1d 14h
Merged PRs (30d)
669

Description

> **Performance audit finding** · Severity: **Low** · Effort: Easy · Fix risk: Simple · Test safety net: Covered (app/components/hooks/useExistingAddress.test.ts)
> Owner: `@MetaMask/confirmations (suggested)`
> File: `app/components/hooks/useExistingAddress.ts:20`

### What is this about?

`useExistingAddress` flattens the per-network address book with `Object.values(addressBook).reduce((acc, networkAddressBook) => ({ ...acc, ...networkAddressBook }), {})`. Spreading `...acc` on every iteration reallocates and recopies the entire accumulator each step, making the flatten O(n^2) in the total number of address-book entries.

**Why it matters**

The address book grows with user contacts across networks. For users with many saved addresses this quadratic copy runs whenever `addressBook` changes (inside `useMemo`). While memoized, the per-recompute cost is unnecessarily high and the pattern is a known anti-pattern flagged by the audit.

### Scenario

N/A — see Technical Details.

### Design

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

### Technical Details

**Evidence**

`app/components/hooks/useExistingAddress.ts:18`
```ts
const filteredAddressBook = useMemo(
() =>
Object.values(addressBook).reduce(
(acc, networkAddressBook) => ({
...acc,
...networkAddressBook,
}),
{},
),
[addressBook],
);
```

**Fix**

Mutate a single accumulator instead of spreading: `Object.values(addressBook).reduce((acc, n) => Object.assign(acc, n), {})`, or `Object.assign({}, ...Object.values(addressBook))`. Both are O(n) and produce the same flattened map. Keep the `useMemo` and dependency unchanged.

### 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/hooks/useExistingAddress.test.ts` to confirm matching/lookup behavior is unchanged.

### References

- File: `app/components/hooks/useExistingAddress.ts:20`
- Source: MetaMask Mobile performance audit — finding `unstablehook-useexistingaddress-reduce-spread`
- Owner (CODEOWNERS / best-effort): @MetaMask/confirmations (suggested)
- Status: **UNVALIDATED**

Contributor guide

Open the contributing guide

Research direction

Start with app/components/hooks/useExistingAddress.ts:18-20 and inspect the existing useMemo flattening logic. Run yarn jest app/components/hooks/useExistingAddress.test.ts; the work is done when the address book is flattened without repeated accumulator spreading and the existing matching and lookup tests pass.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.