Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update NotificationServicesController to accommodate for Snaps Notifications #4809

Merged
merged 33 commits into from
Oct 21, 2024
Merged

Update NotificationServicesController to accommodate for Snaps Notifications #4809

merged 33 commits into from
Oct 21, 2024

Conversation

hmalik88
Copy link
Contributor

@hmalik88 hmalik88 commented Oct 17, 2024 •
edited
Loading

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

@hmalik88 hmalik88 requested a review from a team as a code owner October 17, 2024 17:50
@hmalik88 hmalik88 requested review from a team as code owners October 17, 2024 18:04
@hmalik88 hmalik88 requested review from a team as code owners October 17, 2024 18:23
@hmalik88 hmalik88 changed the title Update NotificationServicesController to accommodate for Snaps Notifications Update NotificationServicesController to accommodate for Snaps Notifications & remove NotificationController Oct 17, 2024
@socket-security Socket Security
Copy link

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

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

🚮 Removed packages: npm/@metamask/[email protected]

View full report↗︎

@hmalik88
Copy link
Contributor Author

hmalik88 commented Oct 17, 2024 •
edited
Loading

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

@hmalik88 hmalik88 mentioned this pull request Oct 17, 2024
Closed
@hmalik88 hmalik88 changed the title Update NotificationServicesController to accommodate for Snaps Notifications & remove NotificationController Update NotificationServicesController to accommodate for Snaps Notifications Oct 17, 2024
@hmalik88 hmalik88 mentioned this pull request Oct 17, 2024
Merged
Prithpal-Sooriya
Prithpal-Sooriya reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some initial comments.

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

...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...tion-services-controller/src/NotificationServicesController/constants/notification-schema.ts Show resolved Hide resolved
...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...ion-services-controller/src/NotificationServicesController/NotificationServicesController.ts Show resolved Hide resolved
...vices-controller/src/NotificationServicesController/processors/process-notifications.test.ts Show resolved Hide resolved
...n-services-controller/src/NotificationServicesController/processors/process-notifications.ts Outdated Show resolved Hide resolved
...on-services-controller/src/NotificationServicesController/types/notification/notification.ts Show resolved Hide resolved
@hmalik88 hmalik88 merged commit d75a971 into main Oct 21, 2024
116 checks passed
@hmalik88 hmalik88 deleted the hm/add-snap-notifications branch October 21, 2024 14:33
@hmalik88 hmalik88 mentioned this pull request Oct 22, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Nov 27, 2024
@hmalik88
feat: Integrate Snap notification services (#27975)
f249b8c 
## **Description**

Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change? Snap notifications were moved into
the `NotificationServicesController` to reduce code duplication.
2. What is the improvement/solution? The `NotificationController` and
associated code was deleted, as well as code that was written to process
snap notifications. Parts of the codebase that expect snap notifications
from the `NotificationController` source were also updated.

See MetaMask/core#4809 for changes made to the
`NotificationServicesController`

## **Screenshots/Recordings**

### **After**




https://github.com/user-attachments/assets/e84f5209-20fa-4b1d-ace7-2eec08bb2329


## **Manual testing steps**
1. Pull down https://github.com/hmalik88/swiss-knife-snap and run `yarn
start` to serve the snap and site.
2. Build this branch using `yarn start:flask`
3. Install the snap from the local host site.
4. Trigger a notification with the "trigger a within limit notification"
button and observe the results.

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
hmalik88 added a commit that referenced this pull request Nov 29, 2024
@hmalik88
Remove NotificationController (#4815)
60e3ce7 
Removes the `NotificationController` in favor of
`NotificationServicesController`, see
#4809
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prithpal-Sooriya Prithpal-Sooriya Prithpal-Sooriya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hmalik88 @Prithpal-Sooriya