feat: push notifications new controller by matteoscurati · Pull Request #23137 · MetaMask/metamask-extension

matteoscurati · 2024-02-22T21:53:58Z

Description

This PR introduces a new SW (Service Worker) and a new controller for managing push notifications.

This integration, due to the differences between MV2 and MV3 regarding the use of background scripts and the SW, is only related to the manifest V2. For the manifest V3, part of this logic will be modified, using the extension's own service worker as a starting point. With manifest V2, on the contrary, it is necessary to initialize a dedicated service worker so that FCM (Firebase Cloud Messaging) can handle messages received in the background.

The final UI will allow the user to enable/disable the receipt of notifications. However, this PR concerns only the

new controller and the build management for the creation of the SW.

The new controller will work in accordance with two other controllers for managing authentication and for managing the triggers necessary to receive notifications.

Related issues

No related issues.

Fixes:

Manual testing steps

At the moment, the necessary environment variables have not been inserted into the build process. To test the branch locally, feel free to ask me for the env to insert into your local .metamaskrc file

Screenshots/Recordings

In this video, I show the interaction between a test server for sending notifications and the extension:

https://www.loom.com/share/702fc6a8aea74510acdb1bfac614fa4a

Before

After

Check the entire flow here https://www.figma.com/file/c7GgNw2kScGrVyRGAPhwEd/Notifications?type=design&node-id=484%3A44713&mode=design&t=QsKrmdqZnx42IrMt-1

Pre-merge author checklist

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-02-22T21:54:10Z

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.

app/scripts/controllers/push-platform-notifications/utils/utils.test.ts

danjm · 2024-03-27T08:36:35Z

app/scripts/controllers/push-platform-notifications/firebase/lib/firebase-app.js

@@ -0,0 +1,2735 @@
+/* eslint-disable */


@matteoscurati could you include a link to where you copied the code from in this code comment?

You're right. I'll include the license and the source of these libraries 👍

Can we not use the npm package for this? https://www.npmjs.com/package/firebase

Great question. Unfortunately, Lavamoat node doesn't support ESM, and Firebase does not expose an entry point in CJS. For this reason, at least for now, we had to import Firebase in the way indicated. If I understand correctly, this problem will be solved with the release of Endomoat

Can we transpile Firebase using Babel instead?

There is an example of how we did this in the past here: https://github.com/MetaMask/metamask-extension/pull/17251/files#diff-a05d1f077953d24c0d8ddecf64163da2653ba3b728e1ba1f18af6d770767b127R831-R841

Ok, thanks to @FrederikBolding 's suggestion, I'm able to import Firebase using npm instead of importing the libraries directly!

danjm · 2024-03-27T08:40:14Z

app/scripts/controllers/push-platform-notifications/firebase/lib/firebase-messaging-sw.js

@@ -0,0 +1,4 @@
+/* eslint-disable */


A few questions/suggestions:

why does this file need to be minified?

is it copied from somewhere? if so where? and then does a license need to be included?

can we include a comment at the top of this file explaining what it is?

Same for the below file

Ok, see the comment above.

The library is minified because it is published that way here https://www.gstatic.com/firebasejs/10.8.0/firebase-messaging-sw.js.

In any case, I have proceeded to expand it.

Sure. See the comment above.

The library is minified because it is published that way here https://www.gstatic.com/firebasejs/10.8.0/firebase-messaging-sw.js.

In any case, I have proceeded to expand it.

danjm · 2024-03-27T08:44:52Z

app/scripts/controllers/push-platform-notifications/firebase/utils/get-notification-data.ts

+  decimalPlaces: 4,
+};
+
+export function calcTokenAmount(value: string, decimals: string) {


I see you added a code comment for the same function in shared/lib/transactions-controller-utils.js. Did you intend to import that function here?

Yes, I'm duplicating this function to keep the SW as light as possible. This way, I don't have to import the entire util file. I know duplication isn't a desirable pattern, but our SW is already quite heavy, and I would like to avoid unnecessary imports.

I added a comment directly in the file!

shared/constants/platform-notifications.ts

danjm · 2024-03-27T08:54:22Z

shared/constants/platform-notifications.ts

+  ERC1155_RECEIVED = 'erc1155_received',
+}
+
+const chains = {


Nit: because we refer to ETHEREUM as MAINNET throughout the shared/constants/network file, and that is used throughout the codebase, it might be best to name this chains.ETHEREUM property as chains.MAINNET

app/scripts/controllers/push-platform-notifications/firebase/utils/get-notification-message.ts

danjm · 2024-03-27T09:10:22Z

app/scripts/controllers/push-platform-notifications/push-platform-notifications.ts

+  },
+};
+
+export class PushPlatformNotificationsController extends BaseController<


Could you add a brief code comment that describes the purpose of the controller?

danjm · 2024-03-27T09:26:30Z

app/scripts/controllers/push-platform-notifications/types/on-chain-notification/schema.d.ts

@@ -0,0 +1,300 @@
+/**
+ * This file was auto-generated by openapi-typescript.


Perhaps include one more line here on how to generate the file, maybe even just the specific command that was used?

Yep, in fact this was in a previous PR so will remove this and make sure the generated file is documented

danjm · 2024-03-27T09:29:52Z

app/scripts/controllers/push-platform-notifications/push-platform-notifications.ts

+  public async enablePushNotifications(UUIDs: string[]) {
+    const bearerToken = await this.getBearerToken();
+    if (!bearerToken) {
+      log.error('Failed to enable push notifications: BearerToken token is missing.');


Any reason not to include the log.error message in the new Error thrown below? I think that with the current implementation, Sentry won't capture the specific error message.

No, you're right. I've corrected it using this pattern:

const errorMessage = `Failed to update triggers for push notifications: ${error}`; log.error(errorMessage); throw new Error(errorMessage);

Can it work?

danjm

Okay, I have completed a first review of this PR. Looks really good overall. My comments are relatively minor

matteoscurati · 2024-03-29T15:57:38Z

@danjm thank you very much!

I've reviewed your comments and modified the code as requested. I hope everything is ok.

If so, I'll wait to follow the order for merging the PRs we discussed to:

change the status of the PR from draft to ready to be merged
fix the build tests

app/scripts/controllers/push-platform-notifications/push-platform-notifications.ts

app/scripts/controllers/push-platform-notifications/utils/utils.test.ts

metamaskbot · 2024-04-23T14:09:02Z

Builds ready [8e304f6]

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, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1138 ± 635 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	206	106	31	15
		domContentLoaded	9	29	17	5	3
		load	55	3207	1138	1323	635
		domInteractive	9	29	17	5	3

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

background: 159.96 KiB (4.42%)
ui: 378 Bytes (0.01%)
common: 2.62 KiB (0.04%)

metamaskbot · 2024-04-24T08:16:59Z

Builds ready [51a7968]

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, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1597 ± 725 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	70	221	133	45	21
		domContentLoaded	15	101	34	22	11
		load	58	3769	1597	1510	725
		domInteractive	15	101	34	22	11

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

background: 159.96 KiB (4.42%)
ui: 378 Bytes (0.01%)
common: 2.62 KiB (0.04%)

danjm · 2024-04-24T09:24:30Z

app/scripts/metamask-controller.js

    delete flatState.vault;

+    // The FCM should not be exposed to the UI
+    delete flatState.fcmToken;


I think we can avoid having to do this. If the PushPlatformNotificationsController is added to this.store in the metamask-controller but not this.memStore, then it's state won't be exposed to the UI, but it will still be persisted.

Ok, thanks for the comment, now everything is much clearer. Removed. Thx!

metamaskbot · 2024-04-24T11:51:38Z

Builds ready [f8a4b25]

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, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1613 ± 675 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	247	128	50	24
		domContentLoaded	10	71	29	17	8
		load	59	3225	1613	1405	675
		domInteractive	10	71	29	17	8

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

background: 159.94 KiB (4.42%)
ui: 378 Bytes (0.01%)
common: 2.62 KiB (0.04%)

Prithpal-Sooriya

Some comments - will resolve them

app/scripts/controllers/push-platform-notifications/push-platform-notifications.test.ts

app/scripts/controllers/push-platform-notifications/push-platform-notifications.ts

app/scripts/controllers/push-platform-notifications/types/__mocks__/mockedHalNotifications.ts

...controllers/push-platform-notifications/types/on-chain-notification/on-chain-notification.ts

ui/store/actions.ts

app/scripts/controllers/push-platform-notifications/types/type-utils.ts

development/build/constants.js

development/verify-locale-strings.js

shared/lib/transactions-controller-utils.js

remove duplicate files and files that should not impact this build

Prithpal-Sooriya · 2024-04-24T14:22:01Z

app/scripts/controllers/metamask-notifications/constants/notification-schema.ts

+export const CHAIN_SYMBOLS = {
+  [NOTIFICATION_CHAINS.ETHEREUM]: 'ETH',
+  [NOTIFICATION_CHAINS.OPTIMISM]: 'ETH',
+  [NOTIFICATION_CHAINS.BSC]: 'BNB',
+  [NOTIFICATION_CHAINS.POLYGON]: 'MATIC',
+  [NOTIFICATION_CHAINS.ARBITRUM]: 'ETH',
+  [NOTIFICATION_CHAINS.AVALANCHE]: 'AVAX',
+  [NOTIFICATION_CHAINS.LINEA]: 'ETH',
+};


Hardcoded Chain Symbols since we need Platform agnostic values (not tied to extension).

If there are shared values then sweet, otherwise we'll keep as is for now.

development/build/scripts.js

this isn't really needed - we can add a feature comment once everything is merged

lavamoat/browserify/beta/policy.json

Prithpal-Sooriya

LGTM! We will wait for an extension review (and also lets merge this after next RC to avoid additional MV3 debugging)

danjm

LGTM

metamaskbot · 2024-04-25T16:48:29Z

Builds ready [b0e6312]

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, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1958 ± 806 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	72	303	170	57	27
		domContentLoaded	10	138	48	37	18
		load	57	4030	1958	1678	806
		domInteractive	10	138	48	37	18

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

background: 159.24 KiB (4.43%)
ui: 0 Bytes (0.00%)
common: 2.17 KiB (0.04%)

metamaskbot · 2024-04-26T10:44:32Z

Builds ready [a0dc43f]

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, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (190 ± 269 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	57	137	77	20	10
		domContentLoaded	9	68	15	14	7
		load	46	2635	190	561	269
		domInteractive	9	67	15	14	7

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

background: 159.24 KiB (4.42%)
ui: 0 Bytes (0.00%)
common: 2.17 KiB (0.04%)

matteoscurati added the team-notifications-deprecated DEPRECATED: please use "team-assets" instead label Feb 22, 2024

github-advanced-security bot found potential problems Mar 4, 2024

View reviewed changes

app/scripts/controllers/push-platform-notifications/utils/utils.test.ts Fixed Show fixed Hide fixed

matteoscurati force-pushed the feature/install-and-use-fcm-sw branch from ea7d9a4 to ddb0703 Compare March 4, 2024 11:21

matteoscurati marked this pull request as ready for review March 5, 2024 13:42

matteoscurati requested review from a team and kumavis as code owners March 5, 2024 13:42

matteoscurati marked this pull request as draft March 5, 2024 13:42

matteoscurati changed the title ~~Feature/install and use fcm sw~~ Feature - Push Notifications New Controller Mar 6, 2024

matteoscurati changed the title ~~Feature - Push Notifications New Controller~~ feat: push notifications new controller Mar 6, 2024

matteoscurati requested a review from Prithpal-Sooriya March 6, 2024 12:48

gauthierpetetin added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 6, 2024

danjm reviewed Mar 27, 2024

View reviewed changes

shared/constants/platform-notifications.ts Outdated Show resolved Hide resolved

danjm reviewed Mar 27, 2024

View reviewed changes

app/scripts/controllers/push-platform-notifications/firebase/utils/get-notification-message.ts Outdated Show resolved Hide resolved

danjm reviewed Mar 27, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

danjm suggested changes Mar 27, 2024

View reviewed changes

matteoscurati force-pushed the feature/install-and-use-fcm-sw branch from 4744f9f to b660747 Compare March 28, 2024 15:51

rekmarks reviewed Apr 1, 2024

View reviewed changes

app/scripts/controllers/push-platform-notifications/push-platform-notifications.ts Show resolved Hide resolved

matteoscurati force-pushed the feature/install-and-use-fcm-sw branch from f52d1fd to efbb9b3 Compare April 9, 2024 08:59

github-advanced-security bot found potential problems Apr 11, 2024

View reviewed changes

app/scripts/controllers/push-platform-notifications/utils/utils.test.ts Fixed Show fixed Hide fixed

matteoscurati force-pushed the feature/install-and-use-fcm-sw branch 2 times, most recently from 46b861d to 162355b Compare April 11, 2024 20:55

Merge branch 'develop' into feature/install-and-use-fcm-sw

8e304f6

Merge branch 'develop' into feature/install-and-use-fcm-sw

58255f8

matteoscurati requested a review from danjm April 24, 2024 07:41

Merge branch 'develop' into feature/install-and-use-fcm-sw

51a7968

danjm reviewed Apr 24, 2024

View reviewed changes

matteoscurati added 2 commits April 24, 2024 13:09

Merge branch 'develop' into feature/install-and-use-fcm-sw

5ce2538

refactor: ♻️ keep the fcmtoken in the state

f8a4b25

Prithpal-Sooriya reviewed Apr 24, 2024

View reviewed changes

refactor: suggested changes

e9675b8

remove duplicate files and files that should not impact this build

Prithpal-Sooriya reviewed Apr 24, 2024

View reviewed changes

refactor: fix linting

66ccaca

Prithpal-Sooriya reviewed Apr 24, 2024

View reviewed changes

development/build/scripts.js Show resolved Hide resolved

Prithpal-Sooriya added 2 commits April 24, 2024 15:32

refactor: revert file changes that are not needed

a740610

refactor: remove controller comment

e30a211

this isn't really needed - we can add a feature comment once everything is merged

Prithpal-Sooriya reviewed Apr 24, 2024

View reviewed changes

lavamoat/browserify/beta/policy.json Show resolved Hide resolved

Prithpal-Sooriya approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'develop' into feature/install-and-use-fcm-sw

b0e6312

danjm approved these changes Apr 25, 2024

View reviewed changes

Merge branch 'develop' into feature/install-and-use-fcm-sw

a0dc43f

Prithpal-Sooriya merged commit f6728c8 into develop Apr 26, 2024

Prithpal-Sooriya deleted the feature/install-and-use-fcm-sw branch April 26, 2024 10:35

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 26, 2024

github-actions bot locked and limited conversation to collaborators Apr 26, 2024

metamaskbot added the release-11.17.0 label Apr 26, 2024

gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024

		@@ -0,0 +1,300 @@
		/**
		* This file was auto-generated by openapi-typescript.

Conversation

matteoscurati commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekmarks Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danjm Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

matteoscurati commented Mar 29, 2024

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Apr 23, 2024

Uh oh!

metamaskbot commented Apr 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Apr 24, 2024

Uh oh!

Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matteoscurati commented Feb 22, 2024 •

edited

Loading

rekmarks Apr 1, 2024 •

edited

Loading

danjm Mar 27, 2024 •

edited

Loading