feat: Convert mmi controller to a non-controller #27983

kanthesha · 2024-10-21T10:48:13Z

Description

We want to bring MMIController up to date with our latest controller patterns.

After review, it turns out that MMIController does not have any state and therefore should not inherit from BaseController (or anything, for that matter).

Related issues

Fixes: #25926

Manual testing steps

N/A

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.

github-actions · 2024-10-21T10:48:25Z

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.

metamaskbot · 2024-10-21T11:58:08Z

Builds ready [ce40ec1]

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

Page Load Metrics (2172 ± 161 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	373	3018	2075	515	247
		domContentLoaded	1721	2949	2127	313	150
		load	1732	3010	2172	335	161
		domInteractive	26	100	47	17	8
		backgroundConnect	9	118	35	33	16
		firstReactRender	51	161	112	24	12
		getState	6	107	26	26	13
		initialActions	0	1	0	0	0
		loadScripts	1266	2397	1596	282	135
		setupStore	13	102	30	24	12
		uiStartup	1938	3253	2442	409	196

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

kanthesha · 2024-10-21T12:03:36Z

The below part of the ticket is not covered at the moment because MetaMetricsController:getState doesn't exist yet!

MMIController no longer takes metaMetricsController as a constructor option.
It no longer reads the state directly from this controller, but uses the MetaMetricsController:getState action through the messenger to do so instead.

app/scripts/metamask-controller.js

metamaskbot · 2024-10-21T13:18:24Z

Builds ready [78451b7]

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

Page Load Metrics (1861 ± 97 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	340	2323	1612	550	264
		domContentLoaded	1600	2432	1835	205	99
		load	1610	2443	1861	202	97
		domInteractive	23	195	55	37	18
		backgroundConnect	7	67	31	19	9
		firstReactRender	46	292	109	66	32
		getState	4	49	13	13	6
		initialActions	0	1	0	0	0
		loadScripts	1161	1877	1362	180	87
		setupStore	11	67	34	24	11
		uiStartup	1796	2706	2103	227	109

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

cryptodev-2s · 2024-10-21T14:56:10Z

Can we add the missing types, even if they would have a value of never, to accurately reflect the original issue requirements? Alternatively, should we update the requirements to exclude those types?

The MMIControllerActions and MMIControllerEvents types exist.
The AllowedActions and AllowedEvents types exist.

kanthesha · 2024-10-21T16:01:24Z

Can we add the missing types, even if they would have a value of never, to accurately reflect the original issue requirements? Alternatively, should we update the requirements to exclude those types?
The MMIControllerActions and MMIControllerEvents types exist.
The AllowedActions and AllowedEvents types exist.

Yeah, I think it'll make sense to update the requirements to exclude these types which are irrelevant here. I'll update it now.

mcmire

Nice PR. Just had some comments on improved ways we could accomplish certain things.

shared/constants/mmi-controller.ts

app/scripts/controllers/mmi-controller.ts

app/scripts/metamask-controller.js

shared/constants/mmi-controller.ts

app/scripts/controllers/mmi-controller.test.ts

…r instance instead of prototype in tests

metamaskbot · 2024-10-30T11:12:08Z

Builds ready [57c8a45]

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

Page Load Metrics (1670 ± 98 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1495	2429	1673	213	102
		domContentLoaded	1485	2354	1643	205	98
		load	1497	2386	1670	205	98
		domInteractive	22	167	52	33	16
		backgroundConnect	9	60	30	19	9
		firstReactRender	46	283	97	51	25
		getState	4	71	14	19	9
		initialActions	0	1	0	0	0
		loadScripts	1065	1973	1230	211	101
		setupStore	10	73	18	16	8
		uiStartup	1647	2558	1865	245	118

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

background: 68 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

…ller

…b.com:MetaMask/metamask-extension into feat/convert-mmi-controller-to-non-controller

metamaskbot · 2024-10-30T12:54:59Z

Builds ready [843328c]

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

Page Load Metrics (1977 ± 97 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	367	2617	1892	405	194
		domContentLoaded	1739	2537	1946	196	94
		load	1789	2621	1977	203	97
		domInteractive	26	148	52	27	13
		backgroundConnect	8	83	34	24	11
		firstReactRender	51	212	95	34	16
		getState	6	66	21	21	10
		initialActions	0	1	0	0	0
		loadScripts	1247	1934	1436	187	90
		setupStore	13	81	39	26	12
		uiStartup	1954	2880	2200	223	107

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

mcmire

Noticed one more thing but after that this should be good!

app/scripts/controllers/mmi-controller.test.ts

app/scripts/controllers/mmi-controller.ts

…he tests

…ller

metamaskbot · 2024-11-01T18:10:11Z

Builds ready [7f0ff01]

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

Page Load Metrics (1952 ± 109 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1586	2349	1954	232	111
		domContentLoaded	1577	2300	1923	213	103
		load	1587	2351	1952	228	109
		domInteractive	28	79	51	17	8
		backgroundConnect	10	120	24	26	12
		firstReactRender	46	160	91	30	15
		getState	5	117	34	32	15
		initialActions	0	1	0	0	0
		loadScripts	1166	1737	1422	166	80
		setupStore	11	82	35	25	12
		uiStartup	1778	2807	2191	279	134

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

mcmire

Looks good!

mikesposito

LGTM!

DDDDDanica · 2024-11-06T10:37:41Z

LGTM, Thanks @kanthesha !

convert mmi controller to a non-controller

c93a059

kanthesha self-assigned this Oct 21, 2024

kanthesha added the team-wallet-framework label Oct 21, 2024

kanthesha requested a review from a team October 21, 2024 10:48

resolve merge conflict

ce40ec1

kanthesha changed the title ~~convert mmi controller to a non-controller~~ feat: Convert mmi controller to a non-controller Oct 21, 2024

kanthesha marked this pull request as ready for review October 21, 2024 11:59

kanthesha requested a review from a team as a code owner October 21, 2024 11:59

cryptodev-2s reviewed Oct 21, 2024

View reviewed changes

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

remove preferences controller from mmi controller

78451b7

mcmire reviewed Oct 25, 2024

View reviewed changes

kanthesha added 2 commits October 29, 2024 15:28

changes based on the review.

f2dc20d

rename messagingSystem in constructor to messenger and spyon messenge…

57c8a45

…r instance instead of prototype in tests

kanthesha added 3 commits October 30, 2024 11:57

Merge branch 'develop' into feat/convert-mmi-controller-to-non-contro…

f29c8a2

…ller

remove unwanted TODO

1c405ff

Merge branch 'feat/convert-mmi-controller-to-non-controller' of githu…

843328c

…b.com:MetaMask/metamask-extension into feat/convert-mmi-controller-to-non-controller

mcmire reviewed Nov 1, 2024

View reviewed changes

app/scripts/controllers/mmi-controller.test.ts Outdated Show resolved Hide resolved

app/scripts/controllers/mmi-controller.ts Outdated Show resolved Hide resolved

kanthesha added 2 commits November 1, 2024 16:59

replace .networkConfigurationsByChainId with messenger call and fix t…

dd56869

…he tests

Merge branch 'develop' into feat/convert-mmi-controller-to-non-contro…

7f0ff01

…ller

mcmire approved these changes Nov 1, 2024

View reviewed changes

kanthesha mentioned this pull request Nov 5, 2024

fix: remove preferences controller from mmi controller constructor #27898

Closed

7 tasks

mikesposito approved these changes Nov 5, 2024

View reviewed changes

kanthesha added this pull request to the merge queue Nov 5, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 5, 2024

DDDDDanica approved these changes Nov 6, 2024

View reviewed changes

kanthesha added this pull request to the merge queue Nov 6, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2024

kanthesha added this pull request to the merge queue Nov 7, 2024

Merged via the queue into develop with commit 765c62e Nov 7, 2024
76 checks passed

kanthesha deleted the feat/convert-mmi-controller-to-non-controller branch November 7, 2024 15:04

github-actions bot locked and limited conversation to collaborators Nov 7, 2024

metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Convert mmi controller to a non-controller #27983

feat: Convert mmi controller to a non-controller #27983

kanthesha commented Oct 21, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 21, 2024

metamaskbot commented Oct 21, 2024

kanthesha commented Oct 21, 2024

metamaskbot commented Oct 21, 2024

cryptodev-2s commented Oct 21, 2024

kanthesha commented Oct 21, 2024

mcmire left a comment

metamaskbot commented Oct 30, 2024

metamaskbot commented Oct 30, 2024

mcmire left a comment

metamaskbot commented Nov 1, 2024

mcmire left a comment

mikesposito left a comment

DDDDDanica commented Nov 6, 2024

feat: Convert mmi controller to a non-controller #27983

feat: Convert mmi controller to a non-controller #27983

Conversation

kanthesha commented Oct 21, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 21, 2024

metamaskbot commented Oct 21, 2024

kanthesha commented Oct 21, 2024

metamaskbot commented Oct 21, 2024

cryptodev-2s commented Oct 21, 2024

kanthesha commented Oct 21, 2024

mcmire left a comment

Choose a reason for hiding this comment

metamaskbot commented Oct 30, 2024

metamaskbot commented Oct 30, 2024

mcmire left a comment

Choose a reason for hiding this comment

metamaskbot commented Nov 1, 2024

mcmire left a comment

Choose a reason for hiding this comment

mikesposito left a comment

Choose a reason for hiding this comment

DDDDDanica commented Nov 6, 2024

kanthesha commented Oct 21, 2024 •

edited by github-actions bot

Loading