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

fix: notification services controller mobile fixes #4441

Merged
merged 8 commits into from
Jun 27, 2024
Merged

fix: notification services controller mobile fixes #4441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ba4f7cf
fix: remove self from web-push
Prithpal-Sooriya Jun 20, 2024
39b2bf0
refactor: add constructor switch to disable push notifications if not…
Prithpal-Sooriya Jun 20, 2024
f230326
refactor: remove const enums
Prithpal-Sooriya Jun 20, 2024
509f795
refactor: fix linting
Prithpal-Sooriya Jun 20, 2024
caf274f
refactor: remove nock fixtures
Prithpal-Sooriya Jun 20, 2024
ab5de02
feat: add notification chain ids
Prithpal-Sooriya Jun 26, 2024
4ca7284
refactor: add defaultState exports for controllers
Prithpal-Sooriya Jun 26, 2024
fe8f87f
Merge branch 'main' of github.com:MetaMask/core into fix/notification…
Prithpal-Sooriya Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions ...-services-controller/src/NotificationServicesController/NotificationServicesController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ export default class NotificationServicesController extends BaseController<
NotificationServicesControllerState,
NotificationServicesControllerMessenger
> {
// Temporary boolean as push notifications are not yet enabled on mobile
#isPushIntegrated = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temp for mobile. We will remove this in a later release

Copy link
Contributor

Choose a reason for hiding this comment

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

Push Notification controllers isn't integrated and won't be on Mobile since MM app relies on Firebase SDK directly. Maybe we should remove the comment and use this flag as a feature that it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK fair, as this is 0.x.x packages, there most likely will be more changes coming down the pipeline. I shall update any documentation and comments in a future version.


#auth = {
getBearerToken: async () => {
return await this.messagingSystem.call(
Expand Down Expand Up @@ -278,24 +281,36 @@ export default class NotificationServicesController extends BaseController<

#pushNotifications = {
enablePushNotifications: async (UUIDs: string[]) => {
return await this.messagingSystem.call(
if (!this.#isPushIntegrated) {
return;
}
await this.messagingSystem.call(
'NotificationServicesPushController:enablePushNotifications',
UUIDs,
);
},
disablePushNotifications: async (UUIDs: string[]) => {
return await this.messagingSystem.call(
if (!this.#isPushIntegrated) {
return;
}
await this.messagingSystem.call(
'NotificationServicesPushController:disablePushNotifications',
UUIDs,
);
},
updatePushNotifications: async (UUIDs: string[]) => {
return await this.messagingSystem.call(
if (!this.#isPushIntegrated) {
return;
}
await this.messagingSystem.call(
'NotificationServicesPushController:updateTriggerPushNotifications',
UUIDs,
);
},
subscribe: () => {
if (!this.#isPushIntegrated) {
return;
}
this.messagingSystem.subscribe(
'NotificationServicesPushController:onNewNotifications',
(notification) => {
Expand All @@ -305,6 +320,9 @@ export default class NotificationServicesController extends BaseController<
);
},
initializePushNotifications: async () => {
if (!this.#isPushIntegrated) {
return;
}
if (!this.state.isNotificationServicesEnabled) {
return;
}
Expand Down Expand Up @@ -411,6 +429,7 @@ export default class NotificationServicesController extends BaseController<
* @param args.state - Initial state to set on this controller.
* @param args.env - environment variables for a given controller.
* @param args.env.featureAnnouncements - env variables for feature announcements.
* @param args.env.isPushIntegrated - toggle push notifications on/off if client has integrated them.
*/
constructor({
messenger,
Expand All @@ -421,6 +440,7 @@ export default class NotificationServicesController extends BaseController<
state?: Partial<NotificationServicesControllerState>;
env: {
featureAnnouncements: FeatureAnnouncementEnv;
isPushIntegrated?: boolean;
};
}) {
super({
Expand All @@ -430,6 +450,8 @@ export default class NotificationServicesController extends BaseController<
state: { ...defaultState, ...state },
});

this.#isPushIntegrated = env.isPushIntegrated ?? true;

this.#featureAnnouncementEnv = env.featureAnnouncements;
this.#registerMessageHandlers();
this.#clearLoadingStates();
Expand Down
1 change: 0 additions & 1 deletion ...notification-services-controller/src/NotificationServicesController/__fixtures__/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@ export * from './mock-notification-trigger';
export * from './mock-notification-user-storage';
export * from './mock-raw-notifications';
export * from './mockResponses';
export * from './mockServices';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mockServices used nock, but was for some reason incompatible with mobile. Removed for now. And we can add back in a later release once we figure out multiple exports and tree shaking.

13 changes: 11 additions & 2 deletions ...n-services-controller/src/NotificationServicesController/constants/notification-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Compute } from '../types/type-utils';

/* eslint-disable @typescript-eslint/naming-convention */
export enum TRIGGER_TYPES {
FEATURES_ANNOUNCEMENT = 'features_announcement',
Expand Down Expand Up @@ -42,15 +44,22 @@ export enum TRIGGER_TYPES_GROUPS {
DEFI = 'defi',
}

export const NOTIFICATION_CHAINS = {
export const NOTIFICATION_CHAINS_ID = {
ETHEREUM: '1',
OPTIMISM: '10',
BSC: '56',
POLYGON: '137',
ARBITRUM: '42161',
AVALANCHE: '43114',
LINEA: '59144',
};
} as const;

type ToPrimitiveKeys<TObj> = Compute<{
[K in keyof TObj]: TObj[K] extends string ? string : TObj[K];
}>;
export const NOTIFICATION_CHAINS: ToPrimitiveKeys<
typeof NOTIFICATION_CHAINS_ID
> = NOTIFICATION_CHAINS_ID;
Comment on lines +47 to +62
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is some fancy types/constants so they can be reused for the /ui folder.

NOTIFICATION_CHAINS_ID is a strict object, whereas NOTIFICATION_CHAINS is the original loose object.


export const CHAIN_SYMBOLS = {
[NOTIFICATION_CHAINS.ETHEREUM]: 'ETH',
Expand Down
1 change: 1 addition & 0 deletions packages/notification-services-controller/src/NotificationServicesController/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export * as Types from './types';
export * as Mocks from './__fixtures__';
export * as Processors from './processors';
export * as Constants from './constants';
export * as UI from './ui';
23 changes: 23 additions & 0 deletions packages/notification-services-controller/src/NotificationServicesController/ui/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { NOTIFICATION_CHAINS_ID } from '../constants/notification-schema';

export const NOTIFICATION_NETWORK_CURRENCY_NAME = {
[NOTIFICATION_CHAINS_ID.ETHEREUM]: 'Ethereum',
[NOTIFICATION_CHAINS_ID.ARBITRUM]: 'Arbitrum',
[NOTIFICATION_CHAINS_ID.AVALANCHE]: 'Avalanche',
[NOTIFICATION_CHAINS_ID.BSC]: 'Binance',
[NOTIFICATION_CHAINS_ID.LINEA]: 'Linea',
[NOTIFICATION_CHAINS_ID.OPTIMISM]: 'Optimism',
[NOTIFICATION_CHAINS_ID.POLYGON]: 'Polygon',
} as const;

export const NOTIFICATION_NETWORK_CURRENCY_SYMBOL = {
[NOTIFICATION_CHAINS_ID.ETHEREUM]: 'ETH',
[NOTIFICATION_CHAINS_ID.ARBITRUM]: 'ETH',
[NOTIFICATION_CHAINS_ID.AVALANCHE]: 'AVAX',
[NOTIFICATION_CHAINS_ID.BSC]: 'BNB',
[NOTIFICATION_CHAINS_ID.LINEA]: 'ETH',
[NOTIFICATION_CHAINS_ID.OPTIMISM]: 'ETH',
[NOTIFICATION_CHAINS_ID.POLYGON]: 'MATIC',
};

export { NOTIFICATION_CHAINS_ID } from '../constants/notification-schema';
Comment on lines +1 to +23
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mobile does not have consistent token name/exports to extension. To keep things in sync, we will use these shared constants in this package.

1 change: 1 addition & 0 deletions packages/notification-services-controller/src/NotificationServicesController/ui/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './constants';
7 changes: 4 additions & 3 deletions ...s-controller/src/NotificationServicesPushController/NotificationServicesPushController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ export type NotificationServicesPushControllerMessenger =
AllowedEvents['type']
>;

export const defaultState: NotificationServicesPushControllerState = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mobile ask - we are now exporting default exports (this ensures that our redux initial state has "something").

fcmToken: '',
};
const metadata = {
fcmToken: {
persist: true,
Expand Down Expand Up @@ -141,9 +144,7 @@ export default class NotificationServicesPushController extends BaseController<
messenger,
metadata,
name: controllerName,
state: {
fcmToken: state?.fcmToken || '',
},
state: { ...defaultState, ...state },
});

this.#env = env;
Expand Down
1 change: 0 additions & 1 deletion ...fication-services-controller/src/NotificationServicesPushController/__fixtures__/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export * from './mockResponse';
export * from './mockServices';
8 changes: 4 additions & 4 deletions ...tion-services-controller/src/NotificationServicesPushController/services/push/push-web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { Types } from '../../../NotificationServicesController';
import { Processors } from '../../../NotificationServicesController';
import type { PushNotificationEnv } from '../../types/firebase';

const sw = self as unknown as ServiceWorkerGlobalScope;
declare const self: ServiceWorkerGlobalScope;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self does not exist on mobile and breaks. So instead removing the sw variable and self is only used within functions instead of a global variable.

In the future (once we support multiple exports and tree shaking) we can tidy this up.


const createFirebaseApp = async (
env: PushNotificationEnv,
Expand Down Expand Up @@ -52,7 +52,7 @@ export async function createRegToken(
try {
const messaging = await getFirebaseMessaging(env);
const token = await getToken(messaging, {
serviceWorkerRegistration: sw.registration,
serviceWorkerRegistration: self.registration,
vapidKey: env.vapidKey,
});
return token;
Expand Down Expand Up @@ -135,8 +135,8 @@ export function listenToPushNotificationsClicked(
handler(event, data);
};

sw.addEventListener('notificationclick', clickHandler);
self.addEventListener('notificationclick', clickHandler);
const unsubscribe = () =>
sw.removeEventListener('notificationclick', clickHandler);
self.removeEventListener('notificationclick', clickHandler);
return unsubscribe;
}
2 changes: 1 addition & 1 deletion packages/notification-services-controller/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"extends": "../../tsconfig.packages.build.json",
"extends": "../../tsconfig.packages.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Misc, noticed we are not using the correct tsconfig for this file compared to other packages. No major issues though.

"compilerOptions": {
"baseUrl": "./"
},
Expand Down
4 changes: 3 additions & 1 deletion packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ export type AuthenticationControllerState = {
isSignedIn: boolean;
sessionData?: SessionData;
};
const defaultState: AuthenticationControllerState = { isSignedIn: false };
export const defaultState: AuthenticationControllerState = {
isSignedIn: false,
};
const metadata: StateMetadata<AuthenticationControllerState> = {
isSignedIn: {
persist: true,
Expand Down
1 change: 0 additions & 1 deletion packages/profile-sync-controller/src/controllers/authentication/__fixtures__/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export * from './mockResponses';
export * from './mockServices';
2 changes: 1 addition & 1 deletion packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export type UserStorageControllerState = {
isProfileSyncingUpdateLoading: boolean;
};

const defaultState: UserStorageControllerState = {
export const defaultState: UserStorageControllerState = {
isProfileSyncingEnabled: true,
isProfileSyncingUpdateLoading: false,
};
Expand Down
1 change: 0 additions & 1 deletion packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from './mockResponses';
export * from './mockServices';
export * from './mockStorage';
2 changes: 1 addition & 1 deletion packages/profile-sync-controller/src/sdk/authentication-jwt-bearer/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Env, Platform } from '../env';

export const enum AuthType {
export enum AuthType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Infura SDK ask. Projects that use isolatedModules are not compatible with const enums.

/* sign in using a private key derived from your secret recovery phrase (SRP).
Uses message signing snap to perform this operation */
SRP = 'SRP',
Expand Down
4 changes: 2 additions & 2 deletions packages/profile-sync-controller/src/sdk/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
export const enum Env {
export enum Env {
DEV = 'dev',
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
UAT = 'uat',
PRD = 'prd',
}

export const enum Platform {
export enum Platform {
MOBILE = 'mobile',
EXTENSION = 'extension',
PORTFOLIO = 'portfolio',
Expand Down
Loading