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.

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

feat: add unlock checks for notification and profile sync controllers #4569

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Jul 29, 2024

Explanation

This adds wallet unlock checks to ensure we only can call these controllers when the wallet is unlocked. This fixes potential issues in extension where we call these controllers when the wallet is locked (e.g. on browser startup)

References

Ticket

Changelog

@metamask/profile-sync-controller

  • ADDED: Actions and Events listening to the Keyring Controller unlock requests
  • ADDED: Unlock checks when the preinstalled snap is called.

@metamask/notification-services-controller

  • ADDED: Actions and Events listening to the Keyring Controller unlock requests
  • ADDED: Unlock checks when trying to setup push notifications

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

@Prithpal-Sooriya Prithpal-Sooriya force-pushed the NOTIFY-865-add-unlock-checks-for-notification-controllers branch from f6a62e9 to 76832a8 Compare July 29, 2024 13:17
@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review July 29, 2024 14:14
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team as a code owner July 29, 2024 14:14
@Prithpal-Sooriya Prithpal-Sooriya added the team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications label Jul 29, 2024
mathieuartu
mathieuartu previously approved these changes Jul 29, 2024
Copy link
Contributor

@mathieuartu mathieuartu left a comment

Choose a reason for hiding this comment

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

All good, just one nit on naming to help understand more quickly when skimming!

also changed requested name from review
…rs' of github.com:MetaMask/core into NOTIFY-865-add-unlock-checks-for-notification-controllers
@Prithpal-Sooriya Prithpal-Sooriya merged commit fc660e9 into main Jul 29, 2024
116 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the NOTIFY-865-add-unlock-checks-for-notification-controllers branch July 29, 2024 15:31
AugmentedMode pushed a commit that referenced this pull request Jul 30, 2024
…#4569)

## Explanation

This adds wallet unlock checks to ensure we only can call these
controllers when the wallet is unlocked. This fixes potential issues in
extension where we call these controllers when the wallet is locked
(e.g. on browser startup)

## References

[Ticket](https://consensyssoftware.atlassian.net/browse/NOTIFY-865)

## 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**: Actions and Events listening to the Keyring Controller
unlock requests
- **ADDED**: Unlock checks when the preinstalled snap is called.

### `@metamask/notification-services-controller`

- **ADDED**: Actions and Events listening to the Keyring Controller
unlock requests
- **ADDED**: Unlock checks when trying to setup push notifications

## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-notifications Notification Team changes. https://github.com/orgs/MetaMask/teams/notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants