MetaMask / MetaMask/metamask-mobile

Memoize new Set result in selectRequiredTransactionIds

Open
#31,355 1 comment 0 reactions 0 assignees View on GitHub
area-performance Sev3 size-S ta-triaged team-confirmations team-transactions
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: Partial
> Owner: `@MetaMask/confirmations`
> File: `app/selectors/transactionController.ts:98`

### What is this about?

`selectRequiredTransactionIds` is a plain `createSelector` whose result function builds `new Set(transactions.flatMap(...))`. A `Set` is a new reference every recompute, and two Sets with identical contents are never `===`. So whenever the transactions array recomputes, this Set changes identity, and so does the downstream chain (`selectRequiredTransactions`, `selectRequiredTransactionHashes`).

**Why it matters**

The `transactions` array changes on every transaction status update. Each rebuild yields a new Set, breaking memoization for the whole required-transactions selector chain even when the actual set of required IDs is unchanged. `createDeepEqualSelector`'s default deep-equal does not compare `Set` contents reliably, so a stable-content guard is needed.

### Scenario

N/A — see Technical Details.

### Design

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

### Technical Details

**Evidence**

`app/selectors/transactionController.ts:98`
```ts
export const selectRequiredTransactionIds = createSelector(
selectTransactionsStrict,
(transactions) =>
new Set(transactions.flatMap((tx) => tx.requiredTransactionIds ?? [])),
);
```

**Fix**

Return a sorted array (deep-equal comparable) and build the Set at the call site, or add a custom `memoize` with a Set-aware equality. Simplest: produce a stable array and let `createDeepEqualSelector` memoize it:
```ts
export const selectRequiredTransactionIds = createDeepEqualSelector(
selectTransactionsStrict,
(transactions) =>
[...new Set(transactions.flatMap((tx) => tx.requiredTransactionIds ?? []))].sort(),
);
```
(Adjust downstream `.has(...)` usages to array `.includes(...)`, or reconstruct a Set in a `useMemo`.)

### Threat Modeling Framework

N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.

### Acceptance Criteria

- `app/selectors/transactionController.test.ts:146` asserts contents (`toStrictEqual`) but not referential stability. Add a test asserting `toBe` across two equal-content transaction arrays. Profiler: confirm `selectRequiredTransactions`/`selectRequiredTransactionHashes` consumers stop re-rendering on unrelated transaction updates.

### References

- File: `app/selectors/transactionController.ts:98`
- Source: MetaMask Mobile performance audit — finding `selector-required-transaction-ids`
- Owner (CODEOWNERS / best-effort): @MetaMask/confirmations
- Status: **UNVALIDATED**

Contributor guide

Open the contributing guide

Research direction

Start in app/selectors/transactionController.ts at selectRequiredTransactionIds and read the downstream selectors that consume it. Run the focused cases in app/selectors/transactionController.test.ts, including the existing contents assertion. Done means equal-content transaction arrays preserve the selector result reference and downstream behavior remains correct.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
mobile-dev
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.