feat: Integrate notifications related controller into core packages #4320
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
Prithpal-Sooriya
left a comment
Prithpal-Sooriya
left a comment
There was a problem hiding this comment.
Structure looks good! This is mostly a copy of extension
Jonathansoufer
added
enhancement
Jonathansoufer
changed the title
note - we need to ensure types and linting are passing
There was a problem hiding this comment.
Hello! I see that there are new controllers being added in this PR. I left some suggestions on aligning to our current standards for controllers. I realize these aren't documented anywhere — we're working on that — but in the meantime I've left some suggestions. I also realize that this code may have been lifted directly from the extension. So if this PR is primarily designed to match the extension, and then work is planned to align it with this repo in another PR, I am fine with that.
That said, we should definitely ensure that versions of new packages starts at 0.0.0 and not 1.0.0 so that they don't get prematurely published accidentally.
packages/notifications-controller/src/MetamaskNotificationsController.ts
Outdated
Show resolved
Hide resolved
packages/notifications-controller/src/MetamaskNotificationsController.ts
Outdated
Show resolved
Hide resolved
packages/notifications-controller/src/MetamaskNotificationsController.ts
Outdated
Show resolved
Hide resolved
packages/notifications-controller/src/MetamaskNotificationsController.ts
Outdated
Show resolved
Hide resolved
packages/notifications-controller/src/MetamaskNotificationsController.ts
Outdated
Show resolved
Hide resolved
packages/push-platform-notifications-controller/tsconfig.build.json
Outdated
Show resolved
Hide resolved
packages/notifications-controller/src/MetamaskNotificationsController.ts
Show resolved
Hide resolved
|
Hey @mcmire, TY for the comments and reviews! This PR is mostly to keep our Extension and Mobile teams unblocked for delivering the notifications feature and ensuring the Mobile and Extension logic doesn't diverge too much. If possible, we would like to release this as a V0 until the main development phase is done and the controllers start being more concrete. |
|
Ah, I think this just needs a |
…otifications-controllers
reorganise logic and controllers into separate folders. Will figure out module exports in a little bit
fix circular deps, tests, types, linting
we pass feature announcement env through the controller config now
fix types, imports, tests, builds, circular deps, and decouple as much as I can from extension.
some stuff was using the old controller name. There are some places that use the older name, but might be a hassle to update
these 2 controllers are heavily tied together and should belong under the same package
…otifications-controllers
for some reason the yarn.lock file got mangled. This mostly reverts it & ensures CI passes
|
Hey all, I'm closing this PR since it's become a little too cumbersome to work in. CI is all passing now, so will open a fresh PR and get the notifications team for a thorough first. |
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> This adds some core controllers for the notification services that are consumed in both mobile and extension. (Maintained by the notifications team). Controllers are: - `ProfileSyncController` - which contains 2 controllers (`AuthenticationController` and `UserStorageController`) - These are used for adding or consuming authenticated endpoints and also make use of the profile syncing feature across platforms and devices. - `NotificationServicesController` - which also contains 2 controllers (`NotificationServicesController` and `NotificationsServicesPushController`) - Both of these controllers are tightly coupled. The first manages pull based notifications and creation of resource; and the second is responsible for push notifications. NOTE - these controllers should be treated as V0.x.x, as they are under development on extension and mobile (iron out any issues). Previous PR: #4320 (closed this as it was getting to unruly to work in.) ## References Here is a loom recording for the new controllers. https://www.loom.com/share/4e95a8fcc2ae4d81b737265fc75571c5?sid=8247a832-3f2e-4837-ab74-30c45524ccfe Future Improvements (as separate PRs) 1. Lets tidy up and improve Push Notifications for Mobile (after dog-fooding) - the implementation may differ on mobile. 2. I also want to improve imports so we can do paths (e.g. `@metamask/profile-sync-controller/sdk`) instead of the global named exports have. 3. Decoupling this from extension has shown some annoying messaging system actions back and forth (leading to some circular dependencies due to importing types). This has been resolved for now, but I want our team to rethink how we are using the messaging system to be more streamlined. - I don't want UserStorage controller calling actions to Notifications (we should not do this) - I don't really like how tied the notifications and push notifications do communication back and forth. I would rather we orchestrate communication in 1 controller (so communication is 1 way). <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/profile-sync-controller` - **ADDED**: new `AuthenticationController` - **ADDED**: new `UserStorageController` ### `@metamask/notfication-services-controller` - **ADDED**: new `NotificationServicesController` - **ADDED**: new `NotificationServicesPushController` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: JSoufer <jonathan.ferreira@consensys.net> Co-authored-by: Jonathan Ferreira <44679989+Jonathansoufer@users.noreply.github.com> Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Explanation
This PR adds/integrates all major controllers necessary for the new Notifications feature. These controllers were first created integrated into MM Extension, but need to be shared among clients [extension, mobile, portfolio].
References
Changelog
@metamask/notifications-controller@metamask/profile-sync-controller@metamask/push-platform-notifications-controllerChecklist