From f0530bb3e619c119a3d2aca3e1d6a672f86183b3 Mon Sep 17 00:00:00 2001 From: Dovydas Stankevicius Date: Thu, 18 Jul 2024 14:26:57 +0100 Subject: [PATCH 1/7] feat: removed node dependancy from profile-sync-sdk (#4539) ## Explanation Removed NodeJs dependency that was breaking the SDK usage on the client --- packages/profile-sync-controller/package.json | 1 - .../src/controllers/authentication/auth-snap-requests.ts | 6 +++--- yarn.lock | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index 65887acd4de..658ac124793 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -44,7 +44,6 @@ "@metamask/base-controller": "^6.0.1", "@metamask/snaps-controllers": "^8.1.1", "@metamask/snaps-sdk": "^4.2.0", - "@metamask/snaps-utils": "^7.4.0", "@noble/ciphers": "^0.5.2", "@noble/hashes": "^1.4.0", "immer": "^9.0.6", diff --git a/packages/profile-sync-controller/src/controllers/authentication/auth-snap-requests.ts b/packages/profile-sync-controller/src/controllers/authentication/auth-snap-requests.ts index 649c51a5580..347e79800aa 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/auth-snap-requests.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/auth-snap-requests.ts @@ -1,6 +1,6 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ import type { HandleSnapRequest } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; -import { HandlerType } from '@metamask/snaps-utils'; type SnapRPCRequest = Parameters[0]; @@ -15,7 +15,7 @@ export function createSnapPublicKeyRequest(): SnapRPCRequest { return { snapId, origin: '', - handler: HandlerType.OnRpcRequest, + handler: 'onRpcRequest' as any, request: { method: 'getPublicKey', }, @@ -34,7 +34,7 @@ export function createSnapSignMessageRequest( return { snapId, origin: '', - handler: HandlerType.OnRpcRequest, + handler: 'onRpcRequest' as any, request: { method: 'signMessage', params: { message }, diff --git a/yarn.lock b/yarn.lock index e28f4614f4d..1eef5f68e21 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3527,7 +3527,6 @@ __metadata: "@metamask/base-controller": "npm:^6.0.1" "@metamask/snaps-controllers": "npm:^8.1.1" "@metamask/snaps-sdk": "npm:^4.2.0" - "@metamask/snaps-utils": "npm:^7.4.0" "@noble/ciphers": "npm:^0.5.2" "@noble/hashes": "npm:^1.4.0" "@types/jest": "npm:^27.4.1" From 7969752025ef418a5ad72f250421e54bc81c1002 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 15:41:23 +0100 Subject: [PATCH 2/7] fix: notification services controller missing env vars (#4530) ## Explanation If platforms try to use this controller but pass invalid environment variables, we will make unnecessary calls to fetch feature announcements that will fail. (This mostly happens in Extension, where some controllers are in JS and do not provide type errors). NOTE - that a new service is being developed to deliver feature announcements, which might allow us to not require platforms to pass in an environment. ## References [NOTIFY-848](https://consensyssoftware.atlassian.net/browse/NOTIFY-848) ## Changelog ### `@metamask/notification-services-controller` - **CHANGED**: Updated notification mocks to use realistic tx hashes - **FIXED**: Logic to check feature announcement environment before fetching ## 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 --- .../__fixtures__/mock-raw-notifications.ts | 12 ++++++--- .../services/feature-announcements.test.ts | 25 +++++++++++++++++++ .../services/feature-announcements.ts | 14 +++++++---- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mock-raw-notifications.ts b/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mock-raw-notifications.ts index 69b1de3c61e..9d0163b5b53 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mock-raw-notifications.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mock-raw-notifications.ts @@ -14,7 +14,8 @@ export function createMockNotificationEthSent(): OnChainRawNotification { chain_id: 1, block_number: 17485840, block_timestamp: '2022-03-01T00:00:00Z', - tx_hash: '0x881D40237659C251811CEC9c364ef91dC08D300C', + tx_hash: + '0xb2256b183f2fb3872f99294ab55fb03e6a479b0d4aca556a3b27568b712505a6', unread: true, created_at: '2022-03-01T00:00:00Z', address: '0x881D40237659C251811CEC9c364ef91dC08D300C', @@ -48,7 +49,8 @@ export function createMockNotificationEthReceived(): OnChainRawNotification { chain_id: 1, block_number: 17485840, block_timestamp: '2022-03-01T00:00:00Z', - tx_hash: '0x881D40237659C251811CEC9c364ef91dC08D300C', + tx_hash: + '0xb2256b183f2fb3872f99294ab55fb03e6a479b0d4aca556a3b27568b712505a6', unread: true, created_at: '2022-03-01T00:00:00Z', address: '0x881D40237659C251811CEC9c364ef91dC08D300C', @@ -82,7 +84,8 @@ export function createMockNotificationERC20Sent(): OnChainRawNotification { chain_id: 1, block_number: 17485840, block_timestamp: '2022-03-01T00:00:00Z', - tx_hash: '0x881D40237659C251811CEC9c364ef91dC08D300C', + tx_hash: + '0xb2256b183f2fb3872f99294ab55fb03e6a479b0d4aca556a3b27568b712505a6', unread: true, created_at: '2022-03-01T00:00:00Z', address: '0x881D40237659C251811CEC9c364ef91dC08D300C', @@ -122,7 +125,8 @@ export function createMockNotificationERC20Received(): OnChainRawNotification { chain_id: 1, block_number: 17485840, block_timestamp: '2022-03-01T00:00:00Z', - tx_hash: '0x881D40237659C251811CEC9c364ef91dC08D300C', + tx_hash: + '0xb2256b183f2fb3872f99294ab55fb03e6a479b0d4aca556a3b27568b712505a6', unread: true, created_at: '2022-03-01T00:00:00Z', address: '0x881D40237659C251811CEC9c364ef91dC08D300C', diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.test.ts b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.test.ts index 506f8020226..55b1b85183b 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.test.ts @@ -3,6 +3,10 @@ import { mockFetchFeatureAnnouncementNotifications } from '../__fixtures__/mockS import { TRIGGER_TYPES } from '../constants/notification-schema'; import { getFeatureAnnouncementNotifications } from './feature-announcements'; +// Mocked type for testing, allows overwriting TS to test erroneous values +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type MockedType = any; + jest.mock('@contentful/rich-text-html-renderer', () => ({ documentToHtmlString: jest .fn() @@ -20,6 +24,27 @@ describe('Feature Announcement Notifications', () => { jest.clearAllMocks(); }); + it('should return an empty array if invalid environment provided', async () => { + mockFetchFeatureAnnouncementNotifications(); + + const assertEnvEmpty = async ( + override: Partial, + ) => { + const result = await getFeatureAnnouncementNotifications({ + ...featureAnnouncementsEnv, + ...override, + }); + expect(result).toHaveLength(0); + }; + + await assertEnvEmpty({ accessToken: null as MockedType }); + await assertEnvEmpty({ platform: null as MockedType }); + await assertEnvEmpty({ spaceId: null as MockedType }); + await assertEnvEmpty({ accessToken: '' }); + await assertEnvEmpty({ platform: '' }); + await assertEnvEmpty({ spaceId: '' }); + }); + it('should return an empty array if fetch fails', async () => { const mockEndpoint = mockFetchFeatureAnnouncementNotifications({ status: 500, diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts index 02cd54753bd..42d982e8663 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts @@ -135,10 +135,14 @@ const fetchFeatureAnnouncementNotifications = async ( export async function getFeatureAnnouncementNotifications( env: Env, ): Promise { - const rawNotifications = await fetchFeatureAnnouncementNotifications(env); - const notifications = rawNotifications.map((notification) => - processFeatureAnnouncement(notification), - ); + if (env?.accessToken && env?.spaceId && env?.platform) { + const rawNotifications = await fetchFeatureAnnouncementNotifications(env); + const notifications = rawNotifications.map((notification) => + processFeatureAnnouncement(notification), + ); + + return notifications; + } - return notifications; + return []; } From 4bc736ac66625b751514491b04aa905f108fcfc3 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 15:46:37 +0100 Subject: [PATCH 3/7] fix: notification services controller - remove retries (#4531) ## Explanation These retries are not necessary, and also are performed on the UI. Also these retries at a controller level are introducing bottlenecks, as retrying can extend async calls making UI loading much longer. ## References [NOTIFY-860](https://consensyssoftware.atlassian.net/browse/NOTIFY-860) ## Changelog ### `@metamask/notification-services-controller` - **CHANGED**: removed internal retry logic as this is not necessary and also expensive (extends promises) - ****: Your change here ## 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 --- .../services/feature-announcements.ts | 53 ++++++------------- .../utils/utils.ts | 48 +---------------- 2 files changed, 16 insertions(+), 85 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts index 42d982e8663..957ea79848c 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts @@ -1,6 +1,5 @@ import { documentToHtmlString } from '@contentful/rich-text-html-renderer'; -import type { Entry, Asset } from 'contentful'; -import log from 'loglevel'; +import type { Entry, Asset, EntryCollection } from 'contentful'; import { TRIGGER_TYPES } from '../constants/notification-schema'; import { processFeatureAnnouncement } from '../processors/process-feature-announcement'; @@ -37,53 +36,30 @@ export type ContentfulResult = { items?: TypeFeatureAnnouncement[]; }; -const fetchFromContentful = async ( - url: string, - retries = 3, - retryDelay = 1000, -): Promise => { - let lastError: Error | null = null; - - for (let i = 0; i < retries; i++) { - try { - const response = await fetch(url); - if (!response.ok) { - throw new Error(`Fetch failed with status: ${response.status}`); - } - return await response.json(); - } catch (error) { - if (error instanceof Error) { - lastError = error; - } - if (i < retries - 1) { - await new Promise((resolve) => setTimeout(resolve, retryDelay)); - } - } - } - - log.error( - `Error fetching from Contentful after ${retries} retries:`, - lastError, - ); - return null; -}; +const getFeatureAnnouncementUrl = (env: Env) => + FEATURE_ANNOUNCEMENT_URL.replace(DEFAULT_SPACE_ID, env.spaceId) + .replace(DEFAULT_ACCESS_TOKEN, env.accessToken) + .replace(DEFAULT_CLIENT_ID, env.platform); const fetchFeatureAnnouncementNotifications = async ( env: Env, ): Promise => { - const url = FEATURE_ANNOUNCEMENT_URL.replace(DEFAULT_SPACE_ID, env.spaceId) - .replace(DEFAULT_ACCESS_TOKEN, env.accessToken) - .replace(DEFAULT_CLIENT_ID, env.platform); - const data = await fetchFromContentful(url); + const url = getFeatureAnnouncementUrl(env); + + const data = await fetch(url) + .then((r) => r.json()) + .catch(() => null); if (!data) { return []; } const findIncludedItem = (sysId: string) => { + const typedData: EntryCollection = + data; const item = - data?.includes?.Entry?.find((i: Entry) => i?.sys?.id === sysId) || - data?.includes?.Asset?.find((i: Asset) => i?.sys?.id === sysId); + typedData?.includes?.Entry?.find((i: Entry) => i?.sys?.id === sysId) || + typedData?.includes?.Asset?.find((i: Asset) => i?.sys?.id === sysId); return item ? item?.fields : null; }; @@ -94,6 +70,7 @@ const fetchFeatureAnnouncementNotifications = async ( const imageFields = fields.image ? (findIncludedItem(fields.image.sys.id) as ImageFields['fields']) : undefined; + const extensionLinkFields = fields.extensionLink ? (findIncludedItem( fields.extensionLink.sys.id, diff --git a/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts b/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts index de03d12e122..6e77d17a6e1 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts @@ -1,4 +1,3 @@ -import log from 'loglevel'; import { v4 as uuidv4 } from 'uuid'; import { @@ -425,47 +424,6 @@ export function toggleUserStorageTriggerStatus( return userStorage; } -/** - * Attempts to fetch a resource from the network, retrying the request up to a specified number of times - * in case of failure, with a delay between attempts. - * - * @param url - The resource URL. - * @param options - The options for the fetch request. - * @param retries - Maximum number of retry attempts. Defaults to 3. - * @param retryDelay - Delay between retry attempts in milliseconds. Defaults to 1000. - * @returns A Promise resolving to the Response object. - * @throws Will throw an error if the request fails after the specified number of retries. - */ -async function fetchWithRetry( - url: string, - options: RequestInit, - retries = 3, - retryDelay = 1000, -): Promise { - for (let attempt = 1; attempt <= retries; attempt++) { - try { - const response = await fetch(url, options); - if (!response.ok) { - throw new Error(`Fetch failed with status: ${response.status}`); - } - return response; - } catch (error) { - log.error(`Attempt ${attempt} failed for fetch:`, error); - if (attempt < retries) { - await new Promise((resolve) => setTimeout(resolve, retryDelay)); - } else { - throw new Error( - `Fetching failed after ${retries} retries. Last error: ${ - error instanceof Error ? error.message : 'Unknown error' - }`, - ); - } - } - } - - throw new Error('Unexpected error in fetchWithRetry'); -} - /** * Performs an API call with automatic retries on failure. * @@ -473,8 +431,6 @@ async function fetchWithRetry( * @param endpoint - The URL of the API endpoint to call. * @param method - The HTTP method ('POST' or 'DELETE'). * @param body - The body of the request. It should be an object that can be serialized to JSON. - * @param retries - The number of retry attempts in case of failure (default is 3). - * @param retryDelay - The delay between retries in milliseconds (default is 1000). * @returns A Promise that resolves to the response of the fetch request. */ export async function makeApiCall( @@ -482,8 +438,6 @@ export async function makeApiCall( endpoint: string, method: 'POST' | 'DELETE', body: Body, - retries = 3, - retryDelay = 1000, ): Promise { const options: RequestInit = { method, @@ -494,5 +448,5 @@ export async function makeApiCall( body: JSON.stringify(body), }; - return fetchWithRetry(endpoint, options, retries, retryDelay); + return await fetch(endpoint, options); } From aa1912c2e9fe65dfc943618f5a802dbdf9083044 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 15:51:37 +0100 Subject: [PATCH 4/7] chore: profile sync controller - add snap caching (#4532) ## Explanation making multiple calls to snaps can cause an internal 429 too many requests as snap requests are queued. This adds simple in-mem caching to prevent calling the snap with the same values. ## References [NOTIFY-847](https://consensyssoftware.atlassian.net/browse/NOTIFY-847) ## Changelog ### `@metamask/profile-sync-controller` - **ADDED**: Internal in-memory caching to pre-installed snap calls - ****: Your change here ## 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 --- .../AuthenticationController.ts | 32 +++++++++++++++---- .../user-storage/UserStorageController.ts | 16 ++++++++-- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index e2285c667f1..5eed4c09b68 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -311,28 +311,48 @@ export default class AuthenticationController extends BaseController< return THIRTY_MIN_MS > diffMs; } + #_snapPublicKeyCache: string | undefined; + /** * Returns the auth snap public key. * * @returns The snap public key. */ - #snapGetPublicKey(): Promise { - return this.messagingSystem.call( + async #snapGetPublicKey(): Promise { + if (this.#_snapPublicKeyCache) { + return this.#_snapPublicKeyCache; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapPublicKeyRequest(), - ) as Promise; + )) as string; + + this.#_snapPublicKeyCache = result; + + return result; } + #_snapSignMessageCache: Record<`metamask:${string}`, string> = {}; + /** * Signs a specific message using an underlying auth snap. * * @param message - A specific tagged message to sign. * @returns A Signature created by the snap. */ - #snapSignMessage(message: `metamask:${string}`): Promise { - return this.messagingSystem.call( + async #snapSignMessage(message: `metamask:${string}`): Promise { + if (this.#_snapSignMessageCache[message]) { + return this.#_snapSignMessageCache[message]; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), - ) as Promise; + )) as string; + + this.#_snapSignMessageCache[message] = result; + + return result; } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 3aa9e3ce772..c4829d89d2b 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -373,17 +373,27 @@ export default class UserStorageController extends BaseController< return storageKey; } + #_snapSignMessageCache: Record<`metamask:${string}`, string> = {}; + /** * Signs a specific message using an underlying auth snap. * * @param message - A specific tagged message to sign. * @returns A Signature created by the snap. */ - #snapSignMessage(message: `metamask:${string}`): Promise { - return this.messagingSystem.call( + async #snapSignMessage(message: `metamask:${string}`): Promise { + if (this.#_snapSignMessageCache[message]) { + return this.#_snapSignMessageCache[message]; + } + + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), - ) as Promise; + )) as string; + + this.#_snapSignMessageCache[message] = result; + + return result; } #setIsProfileSyncingUpdateLoading( From db8774cb7651536870d2071ffcfbdbb7aeb61582 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 15:56:22 +0100 Subject: [PATCH 5/7] fix: profile sync controller - prevent infinite crashes. (#4533) ## Explanation This is an interested bug that happens through communications with our controllers. 1. If authentication fails, we call "disable profile syncing" 2. This "disable profile syncing" will make calls to our "notification services controller" 3. The notification services controller relies on auth, so if auth continues to fail then we have a loop. We are now removing the call to "disable profile syncing", and instead this will be used in the UI or called in a different area. CC @matteoscurati ## References [NOTIFY-861](https://consensyssoftware.atlassian.net/browse/NOTIFY-861) ## Changelog ### `@metamask/profile-sync-controller` - **REMOVED**: Removed internal calls to `UserStorageController:disableProfileSyncing`. ## 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 --- .../authentication/AuthenticationController.test.ts | 13 +++++-------- .../authentication/AuthenticationController.ts | 9 +-------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts index c9553930e69..66f650dd44e 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -10,6 +10,7 @@ import { mockEndpointLogin, } from './__fixtures__/mockServices'; import type { + Actions, AllowedActions, AuthenticationControllerState, } from './AuthenticationController'; @@ -271,7 +272,7 @@ describe('authentication/authentication-controller - getSessionProfile() tests', * @returns Auth Messenger */ function createAuthenticationMessenger() { - const messenger = new ControllerMessenger(); + const messenger = new ControllerMessenger(); return messenger.getRestricted({ name: 'AuthenticationController', allowedActions: [`SnapController:handleRequest`], @@ -310,13 +311,9 @@ function createMockAuthenticationMessenger() { ); } - const exhaustedMessengerMocks = (action: never) => { - throw new Error( - `MOCK_FAIL - unsupported messenger call: ${action as string}`, - ); - }; - - return exhaustedMessengerMocks(actionType); + throw new Error( + `MOCK_FAIL - unsupported messenger call: ${actionType as string}`, + ); }); return { messenger, mockSnapGetPublicKey, mockSnapSignMessage }; diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index 5eed4c09b68..fa9730e0e4a 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -5,7 +5,6 @@ import type { import { BaseController } from '@metamask/base-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; -import type { UserStorageControllerDisableProfileSyncing } from '../user-storage/UserStorageController'; import { createSnapPublicKeyRequest, createSnapSignMessageRequest, @@ -88,9 +87,7 @@ export type AuthenticationControllerGetSessionProfile = export type AuthenticationControllerIsSignedIn = ActionsObj['isSignedIn']; // Allowed Actions -export type AllowedActions = - | HandleSnapRequest - | UserStorageControllerDisableProfileSyncing; +export type AllowedActions = HandleSnapRequest; // Messenger export type AuthenticationControllerMessenger = RestrictedControllerMessenger< @@ -281,10 +278,6 @@ export default class AuthenticationController extends BaseController< }; } catch (e) { console.error('Failed to authenticate', e); - // Disable Profile Syncing - await this.messagingSystem.call( - 'UserStorageController:disableProfileSyncing', - ); const errorMessage = e instanceof Error ? e.message : JSON.stringify(e ?? ''); throw new Error( From b7fa70e191625b7de4755bd1e44f3890adf62a62 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 16:35:07 +0100 Subject: [PATCH 6/7] refactor: notification services controller - silently handle push notifications (#4536) ## Explanation Even if extensions permit push notifications, browsers can still block push notifications. This change allows push notifications to silently fail, and allow notifications to still be created. ## References ## Changelog ### `@metamask/notification-services-controller` - **ADDED**: catch statements to log and silently fail push notification actions. ## 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 --- .../NotificationServicesController.ts | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts index 87573d62a98..f7d6747180a 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts @@ -284,28 +284,40 @@ export default class NotificationServicesController extends BaseController< if (!this.#isPushIntegrated) { return; } - await this.messagingSystem.call( - 'NotificationServicesPushController:enablePushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'NotificationServicesPushController:enablePushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to enable push notifications', e); + } }, disablePushNotifications: async (UUIDs: string[]) => { if (!this.#isPushIntegrated) { return; } - await this.messagingSystem.call( - 'NotificationServicesPushController:disablePushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'NotificationServicesPushController:disablePushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to disable push notifications', e); + } }, updatePushNotifications: async (UUIDs: string[]) => { if (!this.#isPushIntegrated) { return; } - await this.messagingSystem.call( - 'NotificationServicesPushController:updateTriggerPushNotifications', - UUIDs, - ); + try { + await this.messagingSystem.call( + 'NotificationServicesPushController:updateTriggerPushNotifications', + UUIDs, + ); + } catch (e) { + log.error('Silently failed to update push notifications', e); + } }, subscribe: () => { if (!this.#isPushIntegrated) { From ed7a9dbc23f9ba52545ca35815090b8016f4eaaf Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Thu, 18 Jul 2024 16:18:23 -0600 Subject: [PATCH 7/7] Fix permission-controller lint violations (#4537) There are some `permission-controller` tests which attempt to push an async middleware function onto an engine. Although this works in practice, `@typescript-eslint` balks at this, because the type for `JsonRpcEngine.push` says it takes a `JsonRpcMiddleware`, and a `JsonRpcMiddleware` has a return type of `void`, not `Promise`. We could change that return type to `void | Promise` but that would cause other changes we may not want to make. So this commit merely ignores the lint violations. This should be safe because `JsonRpcEngine` doesn't await middleware anyway. Interestingly, the lint violations caused by this discrepancy only appear locally and not on CI. I am not sure why this is. --- .../src/rpc-methods/getPermissions.test.ts | 14 +++++--- .../rpc-methods/requestPermissions.test.ts | 14 +++++--- .../src/rpc-methods/revokePermissions.test.ts | 35 +++++++++++++------ 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/packages/permission-controller/src/rpc-methods/getPermissions.test.ts b/packages/permission-controller/src/rpc-methods/getPermissions.test.ts index 99e0d02c485..f7724f4617b 100644 --- a/packages/permission-controller/src/rpc-methods/getPermissions.test.ts +++ b/packages/permission-controller/src/rpc-methods/getPermissions.test.ts @@ -14,13 +14,16 @@ describe('getPermissions RPC method', () => { const engine = new JsonRpcEngine(); engine.push( - async ( + ( req: JsonRpcRequest<[]>, res: PendingJsonRpcResponse, next, end, ) => { - await implementation(req, res, next, end, { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { getPermissionsForOrigin: mockGetPermissionsForOrigin, }); }, @@ -44,13 +47,16 @@ describe('getPermissions RPC method', () => { const engine = new JsonRpcEngine(); engine.push( - async ( + ( req: JsonRpcRequest<[]>, res: PendingJsonRpcResponse, next, end, ) => { - await implementation(req, res, next, end, { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { getPermissionsForOrigin: mockGetPermissionsForOrigin, }); }, diff --git a/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts b/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts index 6d467910d1c..77dfc93ae57 100644 --- a/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts +++ b/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts @@ -32,8 +32,11 @@ describe('requestPermissions RPC method', () => { const engine = new JsonRpcEngine(); engine.push<[RequestedPermissions], PermissionConstraint[]>( - async (req, res, next, end) => { - await implementation(req, res, next, end, { + (req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { requestPermissionsForOrigin: mockRequestPermissionsForOrigin, }); }, @@ -102,8 +105,11 @@ describe('requestPermissions RPC method', () => { const engine = new JsonRpcEngine(); engine.push<[RequestedPermissions], PermissionConstraint[]>( - async (req, res, next, end) => { - await implementation(req, res, next, end, { + (req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { requestPermissionsForOrigin: mockRequestPermissionsForOrigin, }); }, diff --git a/packages/permission-controller/src/rpc-methods/revokePermissions.test.ts b/packages/permission-controller/src/rpc-methods/revokePermissions.test.ts index 8a77226df52..5d9a9fdc216 100644 --- a/packages/permission-controller/src/rpc-methods/revokePermissions.test.ts +++ b/packages/permission-controller/src/rpc-methods/revokePermissions.test.ts @@ -27,8 +27,11 @@ describe('revokePermissions RPC method', () => { const mockRevokePermissionsForOrigin = jest.fn(); const engine = new JsonRpcEngine(); - engine.push(async (req, res, next, end) => { - await implementation(req, res, next, end, { + engine.push((req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { revokePermissionsForOrigin: mockRevokePermissionsForOrigin, }); }); @@ -57,8 +60,11 @@ describe('revokePermissions RPC method', () => { const mockRevokePermissionsForOrigin = jest.fn(); const engine = new JsonRpcEngine(); - engine.push(async (req, res, next, end) => { - await implementation(req, res, next, end, { + engine.push((req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { revokePermissionsForOrigin: mockRevokePermissionsForOrigin, }); }); @@ -91,8 +97,11 @@ describe('revokePermissions RPC method', () => { const mockRevokePermissionsForOrigin = jest.fn(); const engine = new JsonRpcEngine(); - engine.push(async (req, res, next, end) => { - await implementation(req, res, next, end, { + engine.push((req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { revokePermissionsForOrigin: mockRevokePermissionsForOrigin, }); }); @@ -125,8 +134,11 @@ describe('revokePermissions RPC method', () => { const mockRevokePermissionsForOrigin = jest.fn(); const engine = new JsonRpcEngine(); - engine.push(async (req, res, next, end) => { - await implementation(req, res, next, end, { + engine.push((req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { revokePermissionsForOrigin: mockRevokePermissionsForOrigin, }); }); @@ -158,8 +170,11 @@ describe('revokePermissions RPC method', () => { const mockRevokePermissionsForOrigin = jest.fn(); const engine = new JsonRpcEngine(); - engine.push(async (req, res, next, end) => { - await implementation(req, res, next, end, { + engine.push((req, res, next, end) => { + // We intentionally do not await this promise; JsonRpcEngine won't await + // middleware anyway. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + implementation(req, res, next, end, { revokePermissionsForOrigin: mockRevokePermissionsForOrigin, }); });