MetaMask / MetaMask/metamask-extension
[P2] Consolidate controller state registration into a single declaration source
- 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
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