MetaMask / MetaMask/metamask-extension
[Epic] `MetamaskController` decomposition
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
## Motivation
### Cross-client defects eliminated by construction
[`MetamaskController`](https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/metamask-controller.js) (9,260 lines, 164 methods) and mobile's [`Engine.ts`](https://github.com/MetaMask/metamask-mobile/blob/main/app/core/Engine/Engine.ts) reimplement the same wallet framework orchestration with independent drift. Both clients duplicate six domain clusters (vault, accounts, transactions, permissions, snaps, tokens), so every bug fix lands twice. Mobile's aggregation root ships with four documented defect classes: a listener leak from duplicate controller registration, per-controller Redux dispatch with zero batching, a suppressed TypeScript error on `ComposableController`, and a hand-written persistence blacklist that bypasses [`deriveStateFromMetadata`](https://github.com/MetaMask/core/tree/main/packages/base-controller). A typed cross-client registry prevents all four structurally. They cannot reappear in the shared code path because the shape that produces them no longer exists there.
### TypeScript migration unblocked
[`metamask-extension#23020`](https://github.com/MetaMask/metamask-extension/issues/23020) has been open since February 2024 as "the final converting task after all other dependency level files are finished." It has not progressed because `MetamaskController` is too large to convert in one pass. This epic shrinks `MetamaskController` from 9,260 to ~3,500 lines by moving bodies into modules that are already TypeScript. Every body port is a JS→TS migration. After this epic and [`decisions#117`](https://github.com/MetaMask/decisions/pull/117), the residual `.js` surface fits a single PR cycle rather than a multi-quarter project.
### Review bandwidth recovered
[`metamask-extension#29600`](https://github.com/MetaMask/metamask-extension/issues/29600) Phase 2 stalled on reviewer load, not technical difficulty. Reviewer context window is the binding constraint in this codebase. This decomposition ships as focused S/M/L tickets, each targeting one module and one subject area.
### The cost is lower than it looks
Per-ticket effort splits into audit (what each `messenger.call(...)` site really does) and execution (move body, rewire references, add tests). Execution is mechanical and mostly scriptable:
1. Port the method body from `metamask-controller.js` to the target TypeScript module.
2. Register the module's public methods as actions on a per-module delegating messenger that delegates to the root messenger (per the `WalletService`-messenger pattern from [`decisions#117`](https://github.com/MetaMask/decisions/pull/117)). These are the actions the subsequent `getApi()` rewrites target.
3. Rewrite `this.fooController.bar(...)` as `messenger.call('FooController:bar', ...)` or typed direct injection.
4. Delete the `MetamaskController` original; rewrite the corresponding `getApi()` entry to call `messenger.call('Module:action', ...)` directly. No forwarding stub — a method whose entire body is a single `this.controllerMessenger.call(...)` delegation — may remain after the PR lands.
5. Add a structural unit test against a stub messenger.
Steps 1, 2, 4, and 5 are codemod territory ([`jscodeshift`](https://github.com/facebook/jscodeshift) against `MetamaskController`, [`ts-morph`](https://github.com/dsherret/ts-morph) on the module side; step 2 is one registration entry per public method). Step 3 is the only phase that needs human judgment. Every ticket carries all five steps — no separate "final deletion" or "integration verification" ticket exists.
### What deferring costs
Status quo is not stationary. Every new domain method written on one client without the other adds a drift point. Mobile's four defect classes stay as latent incidents. Most importantly, [`#23020`](https://github.com/MetaMask/metamask-extension/issues/23020) stays open indefinitely because the surface keeps growing.
## Context
`MetamaskController` on `main` is 9,260 lines. None of the six `wallet-services/` modules or the five extension-specific modules exist on `main` today. Decomposition is independent of the parallel `State Sync Redesign` work and has no hard dependency on it — every ticket merges to `main` directly.
**Line-count target (two horizons):**
- **Post-epic (this plan alone): 9,260 → ~3,500 lines.**
- **After [`decisions#117`](https://github.com/MetaMask/decisions/pull/117) also lands: ~3,500 → ~1,300 lines.** `decisions#117` deletes the ~1,500–2,000-line `getApi()` pass-through block; that removal is out of scope here but compounds with this epic to reach the thin composition-root shape.
Neither target is achievable from extraction alone without #117, and the success bar below reflects the post-epic horizon unless otherwise marked.
## Goal
Drive `MetamaskController` from 9,260 → ~3,500 lines (post-epic) / ~1,300 lines (after [`decisions#117`](https://github.com/MetaMask/decisions/pull/117) deletes `getApi()`) by extracting the six `wallet-services/` modules and the five extension-specific modules from scratch on `main`, enforcing the `wallet-services/` boundary via ESLint.
## Success bar
- `MetamaskController` under ~3,500 lines post-epic; under ~1,500 lines after [`decisions#117`](https://github.com/MetaMask/decisions/pull/117) lands
- All six `wallet-services/` modules exist on `main` with full bodies + structural unit tests + per-module actions constants
- `register.ts` has no `as never` cast
- `connection-manager`, `controller-subscriptions`, `session-manager`, `hardware-wallet`, `provider-engine` exist on `main` with full bodies
- ESLint `import/no-restricted-paths` enforcing `wallet-services/` boundaries in CI
- Module unit tests pass against mock messenger
- Manual smoke: extension dev build loads, onboarding completes, send tx succeeds, no new Sentry error signatures on the decomposition build
## Sub-epic structure
Two subject-based sub-epics. Each extraction ticket is self-contained: it scaffolds the module, ports the body, rewires references, deletes the `MetamaskController` original, rewires `getApi()` entries to call `messenger.call(...)` directly (no forwarding stubs), and adds tests — no separate final-deletion or integration-verification ticket.
| Sub-epic | Subject | Tickets | Sizes |
| -------- | -------------------------- | ------- | ------------------- |
| **A** | wallet-services completion | A.1–A.9 | S·S·M·L·L·XL·M·S·S |
| **B** | extension-specific modules | B.1–B.5 | M·L·M·M·L |
## Roadmap
```mermaid
flowchart LR
classDef tier1 fill:#d4edda,stroke:#28a745,color:#000
classDef tier2 fill:#fff3cd,stroke:#ffc107,color:#000
classDef tier3 fill:#f8d7da,stroke:#dc3545,color:#000
subgraph SA["Sub-epic A · wallet-services completion"]
direction TB
A1["A.1 token-resolution
S"]
A2["A.2 snap-management
core · #42522"]
A3["A.3 permission-management
M"]
A4["A.4 account-management
L"]
A5["A.5 transaction-lifecycle
L"]
A6["A.6 vault-management
XL · last"]
A7["A.7 AllowedActions union
M"]
A8["A.8 remove as never
S"]
A9["A.9 ESLint boundary rule
S"]
end
subgraph SB["Sub-epic B · extension-specific modules"]
direction TB
B1["B.1 session-manager
M"]
B2["B.2 connection-manager
L"]
B3["B.3 hardware-wallet
M"]
B4["B.4 controller-subscriptions
M"]
B5["B.5 provider-engine
L"]
end
A1 --> A7
A2 --> A7
A3 --> A7
A4 --> A7
A5 --> A7
A6 --> A7
A7 --> A8
B1 --> B4
B2 --> B5
class A1,A2,A3,A4,A5,A6,A9,B1,B2,B3 tier1
class B4,B5 tier2
class A7,A8 tier3
```
**Legend:** green = tier 1 (no blockers, start any time), yellow = tier 2 (one dependency), red = tier 3 (multi-dependency, late in the critical path).
**Critical path:** A.6 (vault, XL) → A.7 → A.8. A.6 is intentionally sequenced last among Sub-epic A extractions to absorb lessons from A.3–A.5.
**A.2 downgraded (snap-management).** The snap-keyring slice is delivered by the accounts team via the published core package [`@metamask/snap-account-service`](https://github.com/MetaMask/core/tree/main/packages/snap-account-service), integrated by [#42522](https://github.com/MetaMask/metamask-extension/pull/42522) (consume-core path: publish → wire via modular init → reroute → delete, no extension-local body-port). Residual snap passthroughs fold into the [`decisions#117`](https://github.com/MetaMask/decisions/pull/117) `getApi()` rewrite; `handleWatchAssetRequest` moves to A.1. See [#41740](https://github.com/MetaMask/metamask-extension/issues/41740). This is the first landed instance of the core-as-destination outcome described in [`decisions#158` (ADR 0021)](https://github.com/MetaMask/decisions/pull/158).
## Spike PR validation
[PR #41758](https://github.com/MetaMask/metamask-extension/pull/41758) (draft, DO-NOT-MERGE) validates the following beyond extraction mechanics:
- **Partial `EventEmitter` migration.** Controller `stateChange` routing moves off `MetamaskController`'s `EventEmitter` base onto `Messenger` / `ControllerRegistry` subscriptions (`update` emit sites). `lock` and `unlock` emits delete as dead code (zero listeners). `startUISync` and `controllerConnectionChanged` stay on `EventEmitter`: these fire during early app load before controllers and their messengers are guaranteed to be initialized, so the `Messenger` substrate isn't available yet to take their place. `MetamaskController extends EventEmitter` is retained.
- **`background.ts` migration.** `background.js` (2,450 lines) renamed to `.ts` with type annotations on all function signatures and module-scope variables. Build entry point updated. Validates that the service worker entry point converts to TypeScript after registry wiring removes untyped `@metamask/obs-store` APIs.
- **Combined CI signal.** Decomposition + state-broadcast layer removal + TS migration in one build. Confirms the three workstreams don't conflict.
- **Legacy layer deletion.** `ComposableObservableStore`, `PatchStore`, and `publicConfigStore` are absent from the spike branch — all controller `stateChange` flow runs through `ControllerRegistry` + `PatchBuffer`.
## References
- [`decisions#117` — UI Messenger ADR](https://github.com/MetaMask/decisions/pull/117) — gates the post-epic → post-#117 horizon (deletes `getApi()`)
- [`decisions/core/0005-modular-controller-initialisation`](https://github.com/MetaMask/decisions/blob/main/decisions/core/0005-modular-controller-initialisation.md) — prerequisite ADR for composition-root reshaping
- [`metamask-extension#23020`](https://github.com/MetaMask/metamask-extension/issues/23020) — "Convert `metamask-controller.js` to typescript" (this epic feeds it)
- [`metamask-extension#29600`](https://github.com/MetaMask/metamask-extension/issues/29600) — prior decomposition attempt that stalled on review bandwidth
- [`app/scripts/controller-init/messengers/`](https://github.com/MetaMask/metamask-extension/tree/main/app/scripts/controller-init/messengers) — canonical verification source for allowed-action unions per controller
## Sub-epics
- [#41736 — Sub-epic A: `wallet-services` completion](https://github.com/MetaMask/metamask-extension/issues/41736)
- [#41737 — Sub-epic B: extension-specific modules](https://github.com/MetaMask/metamask-extension/issues/41737)
## Validation
- `wc -l app/scripts/metamask-controller.js` under ~3,500 post-epic (under ~1,500 after [`decisions#117`](https://github.com/MetaMask/decisions/pull/117))
- `yarn lint` green with A.9 boundary rule active; deliberate violation caught
- `grep -n "as never" app/scripts/lib/wallet-services/register.ts` returns empty
- `yarn test app/scripts/lib/wallet-services` passes with structural mocks only
- Extension dev build loads, onboarding completes, send tx succeeds, no new Sentry signatures
Contributor guide
Research direction
This is an epic covering app/scripts/metamask-controller.js, six wallet-services modules, and five extension-specific modules; start with sub-epics #41736 and #41737 and the spike PR #41758. Review app/scripts/controller-init/messengers/ and app/scripts/lib/wallet-services/register.ts before choosing a scoped ticket. Done means the selected extraction has its body, messenger actions, structural tests, and no forwarding stub, while the listed lint and test checks pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- eslint, javascript, typescript
- Domain
- backend, build-system, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100