feat: Upgrade alert controller to base controller v2 #28054

kanthesha · 2024-10-23T22:13:18Z

Description

Following the Wallet Framework team's OKRs for Q3 2024, we want to bring AlertController up to date with our latest controller patterns.

Related issues

Fixes: #25915

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.

github-actions · 2024-10-23T22:13:33Z

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.

…troller-v2

metamaskbot · 2024-10-23T22:56:22Z

Builds ready [3c04312]

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 (2279 ± 109 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	284	2620	1862	786	377
		domContentLoaded	1780	2547	2241	235	113
		load	1831	2594	2279	228	109
		domInteractive	31	104	59	21	10
		backgroundConnect	8	104	36	31	15
		firstReactRender	62	303	139	59	28
		getState	7	66	35	20	10
		initialActions	0	0	0	0	0
		loadScripts	1298	1974	1698	206	99
		setupStore	16	90	39	22	11
		uiStartup	2040	3337	2613	316	152

Bundle size diffs [🚀 Bundle size reduced!]

background: -587 Bytes (-0.01%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

mikesposito

Looks good! only minor comments.

mikesposito · 2024-10-24T09:03:14Z

app/scripts/controllers/alert-controller.ts

 };

-const defaultState: AlertControllerState = {
+export const getDefaultAlertControllerState = (): AlertControllerState => ({


nit: can we add some jsdoc for this exported var?

mikesposito · 2024-10-24T09:06:14Z

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


-    const alertMessenger = controllerMessenger.getRestricted({
+const setupController = ({


nit: we could use the withController pattern we have been using for other controllers

Sure, it make sense to have a common coding patterns.

metamaskbot · 2024-10-25T13:17:31Z

Builds ready [7c3949e]

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 (1957 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1772	2460	1951	157	75
		domContentLoaded	1726	2389	1901	148	71
		load	1773	2464	1957	155	74
		domInteractive	17	97	55	21	10
		backgroundConnect	12	95	51	28	13
		firstReactRender	54	192	104	27	13
		getState	6	65	21	19	9
		initialActions	0	1	0	0	0
		loadScripts	1278	1788	1425	118	56
		setupStore	12	80	27	21	10
		uiStartup	1939	2687	2181	176	84

Bundle size diffs [🚀 Bundle size reduced!]

background: -587 Bytes (-0.01%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

mcmire

Overall looks pretty good, just had some suggestions on simplifying certain sections.

mcmire · 2024-10-25T17:52:10Z

app/scripts/controllers/alert-controller.ts

-      web3ShimUsageOrigins[origin] = value;
-      this.store.updateState({ web3ShimUsageOrigins });
+      this.update((state) => {
+        state.web3ShimUsageOrigins = state.web3ShimUsageOrigins || {};


Do we need this line? It wasn't there in the original code. We also already check for the presence of this property on line 220.

Suggested change

state.web3ShimUsageOrigins = state.web3ShimUsageOrigins || {};

mcmire · 2024-10-25T18:03:22Z

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

+      await withController({ state: initialState }, ({ controller }) => {
+        expect(controller.state).toStrictEqual({
+          alertEnabledness: {
+            unconnectedAccount: true,
+            web3ShimUsage: true,
+          },
+          unconnectedAccountAlertShownOrigins: {
+            testUnconnectedOrigin: false,
+          },
+          web3ShimUsageOrigins: {
+            testWeb3ShimUsageOrigin: 0,
+          },
+        });


Hmm, this test is funny. "Default state" implies that no initial state is passed and yet this controller was instantiated with initial state in the test setup. And now we are explicitly passing state. So I feel like one or both of these things isn't needed.

Perhaps we can simplify it to the following?

Suggested change

await withController({ state: initialState }, ({ controller }) => {

expect(controller.state).toStrictEqual({

alertEnabledness: {

unconnectedAccount: true,

web3ShimUsage: true,

},

unconnectedAccountAlertShownOrigins: {

testUnconnectedOrigin: false,

},

web3ShimUsageOrigins: {

testWeb3ShimUsageOrigin: 0,

},

});

const defaultState = getDefaultAlertControllerState();

await withController({ state: defaultState }, ({ controller }) => {

expect(controller.state).toStrictEqual(defaultState);

mcmire · 2024-10-25T18:05:05Z

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

-        alertController.store.getState().alertEnabledness.unconnectedAccount,
-      ).toStrictEqual(true);
+    it('should default unconnectedAccount of alertEnabledness to true', async () => {
+      await withController({ state: initialState }, ({ controller }) => {


Do we need to pass initial state? The controller is defined such the state option is optional, in which case it just uses the default state. So it seems like we could say:

Suggested change

await withController({ state: initialState }, ({ controller }) => {

await withController(({ controller }) => {

mcmire · 2024-10-25T18:05:47Z

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

+      await withController(
+        { state: initialState },
+        ({ controller, messenger }) => {


Similar point as above: can we simplify this?

Suggested change

await withController(

{ state: initialState },

({ controller, messenger }) => {

await withController(({ controller, messenger }) => {

I won't make suggestions on the remaining tests, but the same applies.

mcmire · 2024-10-25T18:08:03Z

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

+            messenger.call('AlertController:getState').web3ShimUsageOrigins,
+          ).toStrictEqual(defaultWeb3ShimUsageOrigins);
+        },
+      );
    });
  });

  describe('AlertController:stateChange', () => {


Hmm, is this test needed at all? We don't often test that a controller's stateChange event gets published when its state changes, because that functionality is provided by BaseController, and we already test it there. Perhaps we can remove it entirely?

upgrade alert controller to base controller v2

9a7393b

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

kanthesha requested a review from a team October 23, 2024 22:13

kanthesha self-assigned this Oct 23, 2024

Merge branch 'develop' into feat/upgrade-alert-controller-to-base-con…

3c04312

…troller-v2

kanthesha marked this pull request as ready for review October 24, 2024 08:33

kanthesha requested a review from a team as a code owner October 24, 2024 08:33

mikesposito previously approved these changes Oct 24, 2024

View reviewed changes

cryptodev-2s previously approved these changes Oct 24, 2024

View reviewed changes

add jsdoc to getDefaultState and add withController in tests

7c3949e

kanthesha dismissed stale reviews from cryptodev-2s and mikesposito via 7c3949e October 25, 2024 12:44

mcmire reviewed Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Upgrade alert controller to base controller v2 #28054

feat: Upgrade alert controller to base controller v2 #28054

kanthesha commented Oct 23, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 23, 2024

metamaskbot commented Oct 23, 2024

mikesposito left a comment

mikesposito Oct 24, 2024

mikesposito Oct 24, 2024

kanthesha Oct 25, 2024

metamaskbot commented Oct 25, 2024

mcmire left a comment

mcmire Oct 25, 2024

mcmire Oct 25, 2024

mcmire Oct 25, 2024

mcmire Oct 25, 2024

mcmire Oct 25, 2024


		const alertMessenger = controllerMessenger.getRestricted({
		const setupController = ({

	await withController({ state: initialState }, ({ controller }) => {
	await withController(({ controller }) => {

feat: Upgrade alert controller to base controller v2 #28054

Are you sure you want to change the base?

feat: Upgrade alert controller to base controller v2 #28054

Conversation

kanthesha commented Oct 23, 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 23, 2024

metamaskbot commented Oct 23, 2024

mikesposito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Oct 25, 2024

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanthesha commented Oct 23, 2024 •

edited by github-actions bot

Loading