refactor: Refactor initialisation of Snaps controllers #30034

Mrtenz · 2025-01-31T15:14:16Z

Description

This refactors the initialisation of Snaps-related controllers to follow the pattern introduced in #28948.

Related issues

Closes #30032.

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

`SnapController`'s name was set to `string` rather than `"SnapController"`, which causes some problems with type inference in MetaMask/metamask-extension#30034.

.yarn/patches/@metamask-snaps-controllers-npm-9.18.0-bdb7548ef5.patch

app/scripts/controller-init/types.ts

metamaskbot · 2025-02-03T11:55:49Z

Builds ready [bcd2967]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1840 ± 92 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	212	2257	1773	399	192
		domContentLoaded	1541	2196	1811	182	87
		load	1551	2261	1840	192	92
		domInteractive	24	100	42	20	9
		backgroundConnect	8	115	32	30	14
		firstReactRender	16	97	43	28	14
		getState	5	88	21	23	11
		initialActions	0	1	0	0	0
		loadScripts	1055	1635	1313	162	78
		setupStore	7	59	13	11	5
		uiStartup	1817	2924	2140	298	143

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 9.29 KiB (0.16%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

app/scripts/controller-init/controller-list.ts

metamaskbot · 2025-02-03T12:45:25Z

Builds ready [3cfc768]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1739 ± 61 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	363	1999	1604	418	201
		domContentLoaded	1538	1957	1713	122	59
		load	1549	2003	1739	126	61
		domInteractive	25	96	42	20	10
		backgroundConnect	8	89	29	24	11
		firstReactRender	15	97	36	26	13
		getState	5	89	22	25	12
		initialActions	0	1	0	0	0
		loadScripts	1066	1506	1244	121	58
		setupStore	8	69	22	22	11
		uiStartup	1769	2605	2011	188	90

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 9.29 KiB (0.16%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json

app/scripts/metamask-controller.js

app/scripts/controller-init/snaps/snaps-registry-init.ts

FrederikBolding · 2025-02-03T14:23:05Z

app/scripts/controller-init/snaps/snap-insights-controller-init.ts

+  SnapInsightsControllerMessenger
+> = ({ controllerMessenger, persistedState }) => {
+  const controller = new SnapInsightsController({
+    // @ts-expect-error: `persistedState.SnapInsightsController` is not


Somewhat worrying that all of these types don't match, any ideas why?

metamaskbot · 2025-02-03T15:31:54Z

Builds ready [b068d1d]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1546 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1369	1713	1546	88	42
		domContentLoaded	1363	1702	1525	82	39
		load	1368	1714	1546	89	43
		domInteractive	22	112	41	24	11
		backgroundConnect	8	73	25	21	10
		firstReactRender	15	101	27	24	12
		getState	4	67	20	20	10
		initialActions	0	1	0	0	0
		loadScripts	958	1236	1105	70	33
		setupStore	6	64	13	14	7
		uiStartup	1565	2125	1768	124	60

github-actions · 2025-02-04T10:55:57Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

socket-security · 2025-02-04T10:57:49Z

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	62.4 kB	metamaskbot

View full report↗︎

socket-security · 2025-02-04T10:57:51Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Mrtenz · 2025-02-04T11:56:39Z

@metamaskbot update-policies

metamaskbot · 2025-02-04T12:09:14Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

Mrtenz · 2025-02-04T12:11:19Z

lavamoat/browserify/main/policy.json

These changes are expected, as bumping @metamask/rate-limit-controller made it possible to dedupe some versions. It does not add any new capabilities to @metamask/rate-limit-controller.

…emonic

Mrtenz added the team-snaps-platform Snaps Platform team label Jan 31, 2025

Mrtenz mentioned this pull request Jan 31, 2025

Set name for SnapController MetaMask/snaps#3052

Merged

Mrtenz added a commit to MetaMask/snaps that referenced this pull request Jan 31, 2025

Set name for SnapController (#3052)

dfcdee2

`SnapController`'s name was set to `string` rather than `"SnapController"`, which causes some problems with type inference in MetaMask/metamask-extension#30034.

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 56cf332 to 1dcd71a Compare February 3, 2025 10:56

Mrtenz commented Feb 3, 2025

View reviewed changes

.yarn/patches/@metamask-snaps-controllers-npm-9.18.0-bdb7548ef5.patch Outdated Show resolved Hide resolved

Mrtenz commented Feb 3, 2025

View reviewed changes

app/scripts/controller-init/types.ts Outdated Show resolved Hide resolved

Mrtenz marked this pull request as ready for review February 3, 2025 11:21

Mrtenz requested a review from a team as a code owner February 3, 2025 11:21

GuillaumeRx reviewed Feb 3, 2025

View reviewed changes

app/scripts/controller-init/controller-list.ts Show resolved Hide resolved

Mrtenz changed the base branch from main to fb/snaps-bump-v87 February 3, 2025 14:08

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 3cfc768 to 430f2e8 Compare February 3, 2025 14:09

FrederikBolding reviewed Feb 3, 2025

View reviewed changes

test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json Outdated Show resolved Hide resolved

FrederikBolding reviewed Feb 3, 2025

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

FrederikBolding reviewed Feb 3, 2025

View reviewed changes

app/scripts/controller-init/snaps/snaps-registry-init.ts Outdated Show resolved Hide resolved

FrederikBolding reviewed Feb 3, 2025

View reviewed changes

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 6699f5f to 35fe54f Compare February 4, 2025 10:55

Mrtenz requested review from a team as code owners February 4, 2025 10:55

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 35fe54f to 5ae3d12 Compare February 4, 2025 10:56

Mrtenz marked this pull request as draft February 4, 2025 10:57

Mrtenz removed the request for review from a team February 4, 2025 10:57

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch 2 times, most recently from 55bbf3d to 11d0475 Compare February 4, 2025 11:01

Mrtenz commented Feb 4, 2025

View reviewed changes

Base automatically changed from fb/snaps-bump-v87 to main February 4, 2025 12:42

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 8c3471b to 24deb57 Compare February 4, 2025 13:30

Mrtenz and others added 17 commits February 4, 2025 17:17

Refactor initialisation of Snaps controllers

c4e8e3f

Add JSDoc to all functions

78cd881

Add basic tests

4d7628c

Remove unused import

575ed7d

Update state snapshot

8150706

Fix lint errors

7fcb454

Add missing controllers to flat state type

fb8a939

Rename JsonSnapsRegistry to SnapsRegistry

e568f48

Only pass removeAllConnections functions to init, reimplement getMn…

752fbe7

…emonic

Remove default parameters

78e4e07

Rename messenger

c8cecd9

Move rate limit controller to init function

1ee6227

Update ControllerMessenger to Messenger

cd7b788

Move execution service to separate file

ec15cf9

Fix lint errors

7f2f8e9

Update LavaMoat policies

c01f197

Fix MetaMaskController test

5a9ea5a

Mrtenz force-pushed the mrtenz/refactor-snaps-controllers-init branch from 24deb57 to 5a9ea5a Compare February 4, 2025 16:17

Mrtenz added 2 commits February 4, 2025 17:31

Remove execution service from ControllerFlatState

615be5c

Don't persist execution service state

3d253a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Refactor initialisation of Snaps controllers #30034

refactor: Refactor initialisation of Snaps controllers #30034

Mrtenz commented Jan 31, 2025 •

edited

Loading

metamaskbot commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

FrederikBolding Feb 3, 2025

metamaskbot commented Feb 3, 2025

github-actions bot commented Feb 4, 2025

socket-security bot commented Feb 4, 2025 •

edited

Loading

socket-security bot commented Feb 4, 2025 •

edited

Loading

Mrtenz commented Feb 4, 2025

metamaskbot commented Feb 4, 2025

Mrtenz Feb 4, 2025

refactor: Refactor initialisation of Snaps controllers #30034

Are you sure you want to change the base?

refactor: Refactor initialisation of Snaps controllers #30034

Conversation

Mrtenz commented Jan 31, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

metamaskbot commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

FrederikBolding Feb 3, 2025

Choose a reason for hiding this comment

metamaskbot commented Feb 3, 2025

github-actions bot commented Feb 4, 2025

socket-security bot commented Feb 4, 2025 • edited Loading

socket-security bot commented Feb 4, 2025 • edited Loading

Mrtenz commented Feb 4, 2025

metamaskbot commented Feb 4, 2025

Mrtenz Feb 4, 2025

Choose a reason for hiding this comment

Mrtenz commented Jan 31, 2025 •

edited

Loading

socket-security bot commented Feb 4, 2025 •

edited

Loading

socket-security bot commented Feb 4, 2025 •

edited

Loading