MetaMask / MetaMask/metamask-extension

[P2] Consolidate controller state registration into a single declaration source

Open
#44,680 1 comment 0 reactions 0 assignees View on GitHub
exalate-duplicate INVALID-ISSUE-TEMPLATE team-extension-platform
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

**File:** `app/scripts/metamask-controller.js`, `app/scripts/messenger-client-init/utils.ts`
**Size:** M | **Hours:** 16-24

---

## Problem

- Both `ComposableObservableStore` configs, persisted and memStore, are assembled from three mechanisms: the hand-written literals, the `resetOnRestartStore` spread, and the `controllerPersistedState`/`controllerMemState` spreads built by messenger-client-init. Nothing checks the three against each other, in either config.
- https://github.com/MetaMask/metamask-extension/blob/477be3b0bc9c2a7eb0f74df2a38d60618825c19e/app/scripts/metamask-controller.js#L1477-L1478
- https://github.com/MetaMask/metamask-extension/blob/477be3b0bc9c2a7eb0f74df2a38d60618825c19e/app/scripts/metamask-controller.js#L1548-L1549
- Registration mistakes have historically failed loudly, since a controller missing from memStore breaks its UI visibly. Surfaces consuming state through `subscribeToMessengerEvent` (the perps stream bridge, the hardware-wallet sign tracker) work with no memStore membership, so a missed literal entry or a stale `persistedStateKey: null` opt-out can only fail silently: the feature works all session, then resets on restart, and the loss is invisible until then.
- https://github.com/MetaMask/metamask-extension/blob/477be3b0bc9c2a7eb0f74df2a38d60618825c19e/app/scripts/metamask-controller.js#L6809-L6818
- https://github.com/MetaMask/metamask-extension/blob/477be3b0bc9c2a7eb0f74df2a38d60618825c19e/ui/hooks/hardware-wallets/useHwSignTracker.ts#L420-L432
- Any additional consumer that needs to define coverage over controller state (for example a backup writer) becomes a fourth hand-assembled list with the same silent failure mode.

---

## Solution

One declaration source for which controllers' state is persisted and surfaced to the UI. The messenger-client-init registration model (`persistedStateKey`/`memStateKey`, inclusion by default, explicit `null` to opt out) becomes the canonical list, and both `ComposableObservableStore` configs become views derived from it. Nothing about how or when bytes reach disk changes.

- The messenger-client-init registrations become the canonical registry: `persistedStateKey`, `memStateKey`, and metadata read from the client instances.
- https://github.com/MetaMask/metamask-extension/blob/477be3b0bc9c2a7eb0f74df2a38d60618825c19e/app/scripts/messenger-client-init/utils.ts#L172-L181
- A legacy adapter wraps the controllers still constructed outside messenger-client-init, exposing the same declaration shape. It shrinks as the init migration proceeds and its inventory doubles as the migration checklist.
- Both `ComposableObservableStore` configs are derived from the registry instead of assembled inline from three mechanisms.
- The persistence path is otherwise untouched: `ComposableObservableStore` remains in it, and `PersistenceManager` semantics are unchanged. Persisted-property selection (`deriveStateFromMetadata`, `'persist'`), the debounce, `navigator.locks` serialization, `#pendingPairs` coalescing, the `backedUpStateKeys` re-persist-together rule, the vault backup path, `PatchStore`, and controller code all stay as they are.

**Rollout:** derive-and-compare. Build both configs from the registry alongside the current assembly and assert deep equality in CI. Swap the assembly on sustained parity.

**Follow-up:** this ticket retires one of `ComposableObservableStore`'s three roles, enumeration authority. Its other two roles remain consumed: the merged aggregates (persistence's boot read, the `memStore.subscribe` delivery trigger) and the keyed re-emit (the persistence `stateChange` handler, `PatchStore`). If those consumers migrate to the messenger directly, removing `ComposableObservableStore` becomes a mechanical cleanup: both `ObservableStore`-backed instances go away, along with the duplicate merged copy of controller state each maintains in the background process, leaving persistence and the UI consuming messenger events directly.

---

## Impact

- A silent runtime data-loss class becomes a build failure. A controller with persist-marked metadata and no registration currently ships and loses state on restart with no signal. With the CI guard it cannot merge.
- Landing the guard requires running it against the current controller population, which doubles as an audit: any controller whose persist-marked metadata is registered by no mechanism today is a live silent-loss bug, found and fixed as a side effect.
- Any consumer that derives coverage from the registry (for example a backup writer) gets provable completeness. State cannot be persisted yet missing from that consumer's coverage, or covered yet unpersisted, since both derive from one list.
- No runtime behavior changes. Crashes, corruption rates, write volume, and memory footprint are all untouched, since nothing about how or when bytes reach disk changes. The contribution is prevention, not remediation.

---

## Acceptance Criteria

- [ ] Both `ComposableObservableStore` configs derived from messenger-client-init registrations plus the legacy adapter
- [ ] Parity: derived configs equal the currently assembled configs in CI before cutover
- [ ] CI guard: persist-marked metadata without a registration fails the build
- [ ] Legacy adapter inventory published as the init-migration checklist
- [ ] Documented enumeration interface consumable by consumers beyond the two configs

---

## Labels

`team-extension-platform`

---

## Dependencies

**Depends on:** none
**Dependents:** none

Contributor guide

Open the contributing guide

Research direction

Start by reading app/scripts/metamask-controller.js and app/scripts/messenger-client-init/utils.ts, focusing on the current ComposableObservableStore assembly and registration metadata. Compare the registry-derived configs with the existing configs in CI, then trace the legacy controllers that need the adapter. Done means parity is enforced, uncovered persist-marked metadata fails CI, and the adapter inventory and enumeration interface are documented.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.