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: new metamask notifications #24482

Merged
merged 125 commits into from May 23, 2024
Merged

Conversation

matteoscurati
Copy link
Contributor

@matteoscurati matteoscurati commented May 10, 2024

Description

This PR introduces new features within the MetaMask Extension:

Authentication -> Users can choose to authenticate using the new Profile Syncing feature. This functionality allows users to anonymously share data across different clients authenticated with the same account. MetaMask cannot decrypt this information.
Notifications -> Users can decide whether to enable notifications to track their on-chain activities. Notifications can be viewed on a dedicated page or received via browser notifications.
Users can choose to activate or deactivate these services at any time.

Four new controllers are managing the feature, already merged in previous PRs:

  1. Authentication Controller
  2. User Storage Controller
  3. Push Notification Controller
  4. MetaMask Notification Controller
    Open in GitHub Codespaces

Related issues

Manual testing steps

To test the function, it is necessary:

  1. Follow the onboarding procedure.
  2. In the menu, use the notifications option to enable notifications.

At this point, the user will have enabled notifications.

Screenshots/Recordings

Before

After

To see the entire flow, there is this Figma where the various steps used are described:

https://www.figma.com/design/c7GgNw2kScGrVyRGAPhwEd/Notifications

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@metamaskbot
Copy link
Collaborator

Builds ready [341d8b7]
Page Load Metrics (303 ± 338 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6212486178
domContentLoaded9351363
load512427303703338
domInteractive9351263
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 43.43 KiB (1.24%)
  • ui: 159.42 KiB (2.40%)
  • common: 11.22 KiB (0.18%)

we can sync with snaps team to make sure that the new UI is aligned.
@@ -104,11 +104,11 @@ describe('Test Snap Notification', function () {

// look for the correct text in notifications (via xpath)
await driver.waitForSelector({
css: '.notifications__item__details__message',
css: '.snap-notifications__item__details__message',
Copy link
Contributor

Choose a reason for hiding this comment

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

classname change - the corresponding scss classname has also been updated. We are retaining the original styles.

FUTURE - a separate PR will be made to see if we can get snap notifications to be in a "similar-ish" design to the other notifications. See Figma

snapNotification: SnapNotification;
};

function NotificationItem({ snapNotification }: SnapComponentProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the original snap notification item. Mostly ported over from JS to TS (which found some bugs) - we kept as is (using @ts-expect-error). These fixes, alongside a minor redesign that both teams are satisfied with, will come in a separate PR.

Comment on lines +11 to +17
export type SnapNotification = {
id: string;
createdAt: string;
isRead: boolean;
type: typeof SNAP;
data: RawSnapNotification;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the new snap notification shape.

The data field retains the original snap shape.

The reason for these additional fields are for Notification interface consistency & so we can figure out which notification is which (via the type field acting as a discriminated union.)

import type { RawSnapNotification, SnapNotification } from '../types/types';
import { SNAP } from '../types/types';

export const processSnapNotifications = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an internal process to convert snap notifications into the new shape (which vibes with the rest of out notifications).

@metamaskbot
Copy link
Collaborator

Builds ready [636d009]
Page Load Metrics (619 ± 537 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint69163902613
domContentLoaded8221342
load5631296191119537
domInteractive8221342
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 43.43 KiB (1.24%)
  • ui: 159.8 KiB (2.41%)
  • common: 11.4 KiB (0.18%)

matteoscurati and others added 3 commits May 23, 2024 12:37
this includes removing the toggle and removing from our controller. If we need fine grain control, this will happen in a future PR
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Re-approving the privacy snapshot changes (I also have reviewed most of the code in the app/ directory)

@metamaskbot
Copy link
Collaborator

Builds ready [3e1e984]
Page Load Metrics (284 ± 320 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint5512278157
domContentLoaded8431384
load442500284666320
domInteractive8431384
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 43.03 KiB (1.23%)
  • ui: 158.83 KiB (2.40%)
  • common: 11.19 KiB (0.18%)

@matteoscurati matteoscurati merged commit cde9ebd into develop May 23, 2024
71 of 72 checks passed
@matteoscurati matteoscurati deleted the feat/new-metamask-notifications branch May 23, 2024 12:11
@github-actions github-actions bot locked and limited conversation to collaborators May 23, 2024
@metamaskbot metamaskbot added the release-11.18.0 Issue or pull request that will be included in release 11.18.0 label May 23, 2024
@matteoscurati matteoscurati restored the feat/new-metamask-notifications branch May 23, 2024 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.18.0 Issue or pull request that will be included in release 11.18.0 team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants