Update `NotificationServicesController` to accommodate for Snaps Notifications #4809

hmalik88 · 2024-10-17T17:50:04Z

Explanation

What is the current state of things and why does it need to change? We currently have snap types living in the extension, which should exist in this repo instead. Also, the extension is using two controllers to handle notifications, the NotificationController solely exists for the purpose of snap notifications. With the revamping of the notifications system, it is best to house all notifications in one place.
What is the solution your changes offer and how does it work? Add snap logic to the NotificationServicesController
Are there any changes whose purpose might not obvious to those unfamiliar with the domain? The updateMetamaskNotificationsList function will be the gateway for snaps to add notifications to the controller.

Changelog

`@metamask/notification-services-controller`

ADDED: getNotificationsByType to grab a list of notifications by type.
ADDED: deleteNotificationById to delete a notification in the controller's state (to be only used by notifications that live in this controller which currently is just snaps).
CHANGED: fetchAndUpdateMetamaskNotifications to grab snaps from state before repopulating with a new list of other notifications.
CHANGED: markNotificaftionsAsRead to account for snaps notifications.
CHANGED: updateMetamaskNotificationsList to assign a processed notification to state instead of the originally passed in notification as the processSnapNotification function adds on extra properties to the raw notification.

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

…ommodate for snaps and updated NotificationServicesController tests

socket-security · 2024-10-17T19:05:24Z

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

🚮 Removed packages: npm/@metamask/utils@9.2.1

View full report↗︎

hmalik88 · 2024-10-17T19:23:51Z

CODEOWNER reviews are being triggered because I used the latest version of @metamask/utils in the NotificationServicesController and was required to update the rest of the packages as well to be in parity. There are no breaking changes from 9.1.0 -> 9.3.0

EDIT: Split the bumps into a different PR

This reverts commit 7969de5.

This reverts commit 0870e90.

This reverts commit e99b172.

Prithpal-Sooriya

Looks good, some initial comments.

I think we can move forward with this after our sync session.

Prithpal-Sooriya · 2024-10-18T13:10:36Z

...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts

+      const snapNotifications = this.state.metamaskNotificationsList.filter(
+        (notification) => notification.type === TRIGGER_TYPES.SNAP,
+      );


OK, this is so we don't overwrite and lose snap notifications correct?

We take the prev state this.state.metamaskNotificationsList and grab the snap notifications

We reinsert it back into the notification list below.

Prithpal-Sooriya · 2024-10-18T13:13:17Z

...tion-services-controller/src/NotificationServicesController/constants/notification-schema.ts

@@ -24,6 +24,7 @@ export enum TRIGGER_TYPES {
  ROCKETPOOL_STAKING_REWARDS = 'rocketpool_staking_rewards',
  NOTIONAL_LOAN_EXPIRATION = 'notional_loan_expiration',
  SPARK_FI_HEALTH_FACTOR = 'spark_fi_health_factor',
+  SNAP = 'snap',


I think I agree with this change.

Currently all of these notifications types are notifications coming from our services, we need to see if this doesn't cause issues with our backend calls (create notifications, update notifications, update push links, etc).

I think we are save as the Sets and Data Structures below are responsible for main core logic.

...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts

Prithpal-Sooriya · 2024-10-18T13:57:41Z

...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts

+        snapNotificationIds = snapNotifications.map(
+          (notification) => notification.id,
+        );
+      }
    } catch (err) {
      log.warn('Something failed when marking notifications as read', err);
    }

    // Update the state (state is also used on counter & badge)
    this.update((state) => {
      const currentReadList = state.metamaskNotificationsReadList;


@matteoscurati it's been a while, but what was the purpose of the "read list" when we have the "notification list" below?

I thought the same, good question!

...vices-controller/src/NotificationServicesController/processors/process-notifications.test.ts

...n-services-controller/src/NotificationServicesController/processors/process-notifications.ts

...on-services-controller/src/NotificationServicesController/types/notification/notification.ts

hmalik88 added 7 commits October 17, 2024 11:33

add snap notification types

dc85e1a

add snap processor, mock utility and test

1939195

update notification union to include snap notifications

9961df5

update trigger types

b269078

update process notification function and tests to include snaps

aea9760

fix process notifications execution order

1d3266b

added getNotificationsByType function, updated other functions to acc…

eb8681b

…ommodate for snaps and updated NotificationServicesController tests

hmalik88 requested a review from a team as a code owner October 17, 2024 17:50

hmalik88 added 3 commits October 17, 2024 13:51

Merge branch 'main' into hm/add-snap-notifications

773f426

fix createMockSnapNotification jsdoc

cb8e138

fix lint errors

bcb2263

hmalik88 requested review from a team as code owners October 17, 2024 18:04

hmalik88 added 2 commits October 17, 2024 14:08

fix jsdoc again

9b3fa2a

bump @metamask/utils to 9.3.0

0870e90

hmalik88 requested review from a team as code owners October 17, 2024 18:23

hmalik88 added 2 commits October 17, 2024 14:53

add readDate property to snap notification

08acf8a

remove NotificationController

7969de5

hmalik88 changed the title ~~Update NotificationServicesController to accommodate for Snaps Notifications~~ Update NotificationServicesController to accommodate for Snaps Notifications & remove NotificationController Oct 17, 2024

hmalik88 added 2 commits October 17, 2024 15:00

regenerate lockfile

70f591a

run yarn dedupe

d3ba765

hmalik88 added 2 commits October 17, 2024 15:07

update tsconfig

e99b172

fix test

82b07c2

add deleteNotificationById function

7ab1f78

hmalik88 mentioned this pull request Oct 17, 2024

bump @metamask/utils to 9.3.0 #4814

Open

hmalik88 added 5 commits October 17, 2024 16:47

Revert "remove NotificationController"

7cc4106

This reverts commit 7969de5.

use older version of utils

3fdbeee

regenerate lock file

a15b131

Revert "bump @metamask/utils to 9.3.0"

5ba67cb

This reverts commit 0870e90.

Revert "update tsconfig"

225a7de

This reverts commit e99b172.

hmalik88 changed the title ~~Update NotificationServicesController to accommodate for Snaps Notifications & remove NotificationController~~ Update NotificationServicesController to accommodate for Snaps Notifications Oct 17, 2024

fix lockfile

2e8c150

hmalik88 mentioned this pull request Oct 17, 2024

Remove NotificationController #4815

Open

Prithpal-Sooriya reviewed Oct 18, 2024

View reviewed changes

hmalik88 added 7 commits October 18, 2024 18:30

add assertion to deleteNotificationById and add more tests

3cf79b3

more changes

e59eeb2

lint fix

00922f8

fix type issue

a57d1a7

added deleteNotificationsById for batch deletion

9645a91

Merge branch 'main' into hm/add-snap-notifications

bd312ed

lint fix

0cba55e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `NotificationServicesController` to accommodate for Snaps Notifications #4809

Update `NotificationServicesController` to accommodate for Snaps Notifications #4809

hmalik88 commented Oct 17, 2024 •

edited

Loading

socket-security bot commented Oct 17, 2024 •

edited

Loading

hmalik88 commented Oct 17, 2024 •

edited

Loading

Prithpal-Sooriya left a comment

Prithpal-Sooriya Oct 18, 2024

hmalik88 Oct 18, 2024

Prithpal-Sooriya Oct 18, 2024

Prithpal-Sooriya Oct 18, 2024

hmalik88 Oct 18, 2024

Update NotificationServicesController to accommodate for Snaps Notifications #4809

Are you sure you want to change the base?

Update NotificationServicesController to accommodate for Snaps Notifications #4809

Conversation

hmalik88 commented Oct 17, 2024 • edited Loading

Explanation

Changelog

@metamask/notification-services-controller

Checklist

socket-security bot commented Oct 17, 2024 • edited Loading

hmalik88 commented Oct 17, 2024 • edited Loading

Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

Prithpal-Sooriya Oct 18, 2024

Choose a reason for hiding this comment

hmalik88 Oct 18, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Oct 18, 2024

Choose a reason for hiding this comment

Prithpal-Sooriya Oct 18, 2024

Choose a reason for hiding this comment

hmalik88 Oct 18, 2024

Choose a reason for hiding this comment

Update `NotificationServicesController` to accommodate for Snaps Notifications #4809

Update `NotificationServicesController` to accommodate for Snaps Notifications #4809

hmalik88 commented Oct 17, 2024 •

edited

Loading

`@metamask/notification-services-controller`

socket-security bot commented Oct 17, 2024 •

edited

Loading

hmalik88 commented Oct 17, 2024 •

edited

Loading