diff --git a/packages/notification-services-controller/CHANGELOG.md b/packages/notification-services-controller/CHANGELOG.md index 1ab88f54781..87132a9c7f0 100644 --- a/packages/notification-services-controller/CHANGELOG.md +++ b/packages/notification-services-controller/CHANGELOG.md @@ -9,8 +9,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** `getNotificationsApiConfigCached` now returns `null` when the Trigger API could not be read, and an empty array only when the API reported that no address is subscribed ([#9985](https://github.com/MetaMask/core/pull/9985)) + - Previously both cases returned an empty array, so callers could not tell "the user has nothing enabled" from "we failed to ask", and acting on the guess re-subscribed or unregistered addresses behind the user. - Bump `@metamask/keyring-controller` from `^27.1.0` to `^27.1.1` ([#9791](https://github.com/MetaMask/core/pull/9791)) +### Fixed + +- **BREAKING:** Wallet-activity addresses are once again sourced from the keyring and the Trigger API instead of Authenticated User Storage, which stops users receiving notifications for addresses they do not hold ([#9985](https://github.com/MetaMask/core/pull/9985)) + - Authenticated User Storage is keyed by canonical profile ID, which profile pairing shares across every SRP belonging to the same user, so an address list stored there pooled the addresses of unrelated SRPs and delivered each installation the union of all of them. Addresses are keyring-scoped and cannot live in profile-scoped storage. + - `checkAccountsPresence`, `fetchAndUpdateMetamaskNotifications` and `enablePushNotifications` now take the candidate addresses from the keyring and read the per-address enabled bit from the Trigger API. An installation can therefore only ever ask about addresses it holds. + - `enableAccounts` and `disableAccounts` now write subscriptions to the Trigger API. The endpoint is a per-address upsert, so two installations authenticating as the same profile no longer clobber each other's subscriptions. + - `createOnChainTriggers` no longer writes addresses into `walletActivity.accounts`; it writes an empty list. It subscribes the keyring's accounts only when initializing preferences for the first time and the Trigger API has no subscriptions yet, so the daily re-subscribe can no longer re-enable accounts the user turned off. + - The user-level `walletActivity.inAppNotificationsEnabled` and `walletActivity.pushNotificationsEnabled` toggles are still read from Authenticated User Storage. They contain no addresses, so they remain correct to share across a paired profile. An unreadable preferences blob is treated as both toggles enabled, so a storage outage no longer empties the notification list. +- Unregister the device from push notifications when no account has notifications enabled ([#9985](https://github.com/MetaMask/core/pull/9985)) + - The push API rejects a registration with no addresses, and that request is what performs the delete-and-reinsert of the device's links, so the rejection left the previous links in place and push kept arriving after a user disabled every account. +- `enableAccounts` and `disableAccounts` now reject when the Trigger API rejects the subscription change, instead of reporting success and caching a state the server never applied ([#9985](https://github.com/MetaMask/core/pull/9985)) + - A 4xx/5xx response was treated as success, so the settings UI showed the new toggle position and push links were added or removed for a subscription that did not change. +- A failed Trigger API config read no longer re-enables accounts or unregisters the device ([#9985](https://github.com/MetaMask/core/pull/9985)) + - `createOnChainTriggers` now fails instead of reading a failed config query as "no subscriptions yet", which could subscribe every keyring account for a user who had turned them off, and it writes the preferences blob only after those subscriptions are in place so a failed run can still be retried as a first-time setup. + - `enablePushNotifications` leaves the device's existing push links alone rather than unregistering it, and `checkAccountsPresence` rejects rather than reporting every account as disabled. + ## [26.0.1] ### Changed diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts index 86f39be5c25..30e754cd31f 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts @@ -34,6 +34,7 @@ import { } from './__fixtures__/mockAddresses.js'; import { mockGetOnChainNotificationsConfig, + mockUpdateOnChainNotifications, mockGetAPINotifications, mockFetchFeatureAnnouncementNotifications, mockMarkNotificationsAsRead, @@ -87,16 +88,16 @@ const clearAPICache = (): void => { notificationsConfigCache.clear(); }; -const prefsFromAddresses = ( - accounts: { address: string; enabled: boolean }[], +// An initialized preferences blob. `walletActivity.accounts` is always empty: +// wallet-activity addresses come from the keyring and their enabled state from +// the Trigger API, never from here. +const mockPreferences = ( + overrides?: Partial, ): NotificationPreferences => ({ walletActivity: { inAppNotificationsEnabled: true, pushNotificationsEnabled: true, - accounts: accounts.map((a) => ({ - address: a.address.toLowerCase() as `0x${string}`, - enabled: a.enabled, - })), + accounts: [], }, marketing: { inAppNotificationsEnabled: false, @@ -113,18 +114,18 @@ const prefsFromAddresses = ( }, agenticCli: { ...DEFAULT_AGENTIC_CLI_PREFERENCES }, priceAlerts: { ...DEFAULT_PRICE_ALERT_PREFERENCES }, + ...overrides, }); -const prefsFromAddressesWithMarketingInAppNotifications = ( - accounts: { address: string; enabled: boolean }[], +const mockPreferencesWithMarketingInApp = ( inAppNotificationsEnabled: boolean, -): NotificationPreferences => ({ - ...prefsFromAddresses(accounts), - marketing: { - inAppNotificationsEnabled, - pushNotificationsEnabled: false, - }, -}); +): NotificationPreferences => + mockPreferences({ + marketing: { + inAppNotificationsEnabled, + pushNotificationsEnabled: false, + }, + }); describe('NotificationServicesController', () => { afterEach(() => { @@ -378,6 +379,8 @@ describe('NotificationServicesController', () => { // Arrange const mocks = arrangeMocks(); const mockAPIGetNotificationConfig = mocks.mockGetNotificationPreferences; + // Supplies the enabled wallet-activity addresses that push registers for. + mockGetOnChainNotificationsConfig().persist(); modifications?.(mocks); // Act @@ -508,12 +511,13 @@ describe('NotificationServicesController', () => { describe('checkAccountsPresence', () => { it('returns Record with accounts that have notifications enabled', async () => { const mocks = mockNotificationMessenger(); - mocks.mockGetNotificationPreferences.mockResolvedValueOnce( - prefsFromAddresses([ - { address: ADDRESS_1, enabled: true }, - { address: ADDRESS_2, enabled: false }, - ]), - ); + const mockTriggerQuery = mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: true }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); const controller = new NotificationServicesController({ messenger: mocks.messenger, @@ -524,12 +528,53 @@ describe('NotificationServicesController', () => { ADDRESS_2, ]); - expect(mocks.mockGetNotificationPreferences).toHaveBeenCalled(); + expect(mockTriggerQuery.isDone()).toBe(true); expect(result).toStrictEqual({ [ADDRESS_1]: true, [ADDRESS_2]: false, }); }); + + it('reports accounts the Trigger API does not know about as disabled', async () => { + const mocks = mockNotificationMessenger(); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [{ address: ADDRESS_1.toLowerCase(), enabled: true }], + }); + + const controller = new NotificationServicesController({ + messenger: mocks.messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + const result = await controller.checkAccountsPresence([ + ADDRESS_1, + ADDRESS_3, + ]); + + expect(result).toStrictEqual({ + [ADDRESS_1]: true, + [ADDRESS_3]: false, + }); + }); + + it('throws rather than reporting every account as disabled when the Trigger API is unreadable', async () => { + const mocks = mockNotificationMessenger(); + mockErrorLog(); + mockGetOnChainNotificationsConfig({ + status: 500, + body: { error: 'mock api failure' }, + }); + + const controller = new NotificationServicesController({ + messenger: mocks.messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + await expect( + controller.checkAccountsPresence([ADDRESS_1, ADDRESS_2]), + ).rejects.toThrow('Failed to read wallet-activity subscriptions'); + expect(controller.state.isCheckingAccountsPresence).toBe(false); + }); }); describe('createOnChainTriggers', () => { @@ -552,7 +597,7 @@ describe('NotificationServicesController', () => { }; describe('when AUS preferences are not initialized (preferences are null)', () => { - it('writes a fresh preferences blob using hardcoded defaults, current Trigger API wallet account state, and supplied marketing flags', async () => { + it('writes a fresh preferences blob using hardcoded defaults, no wallet-activity addresses, and supplied marketing flags', async () => { const { messenger, mockEnablePushNotifications, @@ -601,16 +646,7 @@ describe('NotificationServicesController', () => { walletActivity: { inAppNotificationsEnabled: true, pushNotificationsEnabled: true, - accounts: [ - { - address: ADDRESS_1.toLowerCase(), - enabled: true, - }, - { - address: ADDRESS_2.toLowerCase(), - enabled: false, - }, - ], + accounts: [], }, marketing: { inAppNotificationsEnabled: false, @@ -621,6 +657,7 @@ describe('NotificationServicesController', () => { agenticCli: { ...DEFAULT_AGENTIC_CLI_PREFERENCES }, priceAlerts: { ...DEFAULT_PRICE_ALERT_PREFERENCES }, }); + // Only the address the Trigger API reports as enabled. expect(mockEnablePushNotifications).toHaveBeenCalledWith([ ADDRESS_1.toLowerCase(), ]); @@ -665,11 +702,10 @@ describe('NotificationServicesController', () => { expect(mockEnablePushNotifications).not.toHaveBeenCalled(); }); - it('enables all wallet-activity accounts when Trigger API has no enabled accounts for first-time setup', async () => { + it('subscribes all keyring accounts in the Trigger API when none are subscribed yet', async () => { const { messenger, mockEnablePushNotifications, - mockUpdateNotifications, mockKeyringControllerGetState, } = arrangeMocks({ configurePrefs: (mock) => mock.mockResolvedValueOnce(null), @@ -692,6 +728,11 @@ describe('NotificationServicesController', () => { { address: ADDRESS_2.toLowerCase(), enabled: false }, ], }); + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + let subscribeBody: unknown; + mockTriggerUpdate.on('request', (_req, _interceptor, body) => { + subscribeBody = JSON.parse(body as string); + }); const controller = new NotificationServicesController({ messenger, @@ -701,14 +742,99 @@ describe('NotificationServicesController', () => { await controller.createOnChainTriggers(); expect(mockTriggerQuery.isDone()).toBe(true); - const [writtenPrefs] = mockUpdateNotifications.mock.calls[0]; - expect(writtenPrefs.walletActivity.accounts).toStrictEqual([ + expect(mockTriggerUpdate.isDone()).toBe(true); + expect(subscribeBody).toStrictEqual([ { address: ADDRESS_1.toLowerCase(), enabled: true }, { address: ADDRESS_2.toLowerCase(), enabled: true }, ]); + expect(mockEnablePushNotifications).toHaveBeenCalledWith([ + ADDRESS_1, + ADDRESS_2, + ]); + }); + + it('fails without subscribing anything when the Trigger API config cannot be read', async () => { + const { + messenger, + mockEnablePushNotifications, + mockDisablePushNotifications, + mockUpdateNotifications, + mockKeyringControllerGetState, + } = arrangeMocks({ + configurePrefs: (mock) => mock.mockResolvedValueOnce(null), + }); + + mockKeyringControllerGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + accounts: [ADDRESS_1, ADDRESS_2], + type: KeyringTypes.hd, + metadata: { id: 'srp-1', name: 'SRP 1' }, + }, + ], + }); + mockGetOnChainNotificationsConfig({ + status: 500, + body: { error: 'mock api failure' }, + }); + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + await expect(controller.createOnChainTriggers()).rejects.toThrow( + 'Failed to create On Chain triggers', + ); + + expect(mockTriggerUpdate.isDone()).toBe(false); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + expect(mockDisablePushNotifications).not.toHaveBeenCalled(); + // No preferences blob, so a retry is still treated as a first-time + // setup and can seed the subscriptions this run failed to create. + expect(mockUpdateNotifications).not.toHaveBeenCalled(); + }); + + it('leaves existing Trigger API subscriptions alone, so previously disabled accounts stay disabled', async () => { + const { + messenger, + mockEnablePushNotifications, + mockKeyringControllerGetState, + } = arrangeMocks({ + configurePrefs: (mock) => mock.mockResolvedValueOnce(null), + }); + + mockKeyringControllerGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + accounts: [ADDRESS_1, ADDRESS_2], + type: KeyringTypes.hd, + metadata: { id: 'srp-1', name: 'SRP 1' }, + }, + ], + }); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: true }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + await controller.createOnChainTriggers(); + + expect(mockTriggerUpdate.isDone()).toBe(false); expect(mockEnablePushNotifications).toHaveBeenCalledWith([ ADDRESS_1.toLowerCase(), - ADDRESS_2.toLowerCase(), ]); }); @@ -907,11 +1033,10 @@ describe('NotificationServicesController', () => { mockGetConfig, mockUpdateNotifications, } = arrangeMocks({ - configurePrefs: (mock) => - mock.mockResolvedValueOnce( - prefsFromAddresses([{ address: ADDRESS_1, enabled: true }]), - ), + configurePrefs: (mock) => mock.mockResolvedValueOnce(mockPreferences()), }); + mockGetOnChainNotificationsConfig(); + const mockTriggerUpdate = mockUpdateOnChainNotifications(); const controller = new NotificationServicesController({ messenger, @@ -922,21 +1047,97 @@ describe('NotificationServicesController', () => { expect(mockGetConfig).toHaveBeenCalled(); expect(mockUpdateNotifications).not.toHaveBeenCalled(); + expect(mockTriggerUpdate.isDone()).toBe(false); expect(mockEnablePushNotifications).toHaveBeenCalled(); }); - it('preserves user preferences when re-subscribing using enableMetamaskNotifications', async () => { + it('does not re-subscribe accounts on the daily re-subscribe when the user disabled them all', async () => { const { messenger, + mockDisablePushNotifications, + mockEnablePushNotifications, + mockKeyringControllerGetState, + } = arrangeMocks({ + configurePrefs: (mock) => mock.mockResolvedValueOnce(mockPreferences()), + }); + + mockKeyringControllerGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + accounts: [ADDRESS_1, ADDRESS_2], + type: KeyringTypes.hd, + metadata: { id: 'srp-1', name: 'SRP 1' }, + }, + ], + }); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: false }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + await controller.createOnChainTriggers(); + + expect(mockTriggerUpdate.isDone()).toBe(false); + // No addresses to register, so the device is unregistered rather than + // sent an empty list the push API would reject. + await waitFor(() => { + expect(mockDisablePushNotifications).toHaveBeenCalled(); + }); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + }); + + it('does not register push notifications when the wallet-activity push toggle is off', async () => { + const { + messenger, + mockDisablePushNotifications, mockEnablePushNotifications, - mockGetConfig, - mockUpdateNotifications, } = arrangeMocks({ configurePrefs: (mock) => mock.mockResolvedValueOnce( - prefsFromAddresses([{ address: ADDRESS_1, enabled: true }]), + mockPreferences({ + walletActivity: { + inAppNotificationsEnabled: true, + pushNotificationsEnabled: false, + accounts: [], + }, + }), ), }); + mockGetOnChainNotificationsConfig(); + + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + await controller.createOnChainTriggers(); + + await waitFor(() => { + expect(mockDisablePushNotifications).toHaveBeenCalled(); + }); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + }); + + it('preserves user preferences when re-subscribing using enableMetamaskNotifications', async () => { + const { + messenger, + mockEnablePushNotifications, + mockGetConfig, + mockUpdateNotifications, + } = arrangeMocks({ + configurePrefs: (mock) => mock.mockResolvedValueOnce(mockPreferences()), + }); + mockGetOnChainNotificationsConfig(); const controller = new NotificationServicesController({ messenger, @@ -980,19 +1181,29 @@ describe('NotificationServicesController', () => { describe('disableAccounts', () => { const arrangeMocks = (): ReturnType & { - mockUpdateNotifications: jest.Mock; + mockTriggerUpdate: nock.Scope; + getRequestBody: () => unknown; } => { const messengerMocks = mockNotificationMessenger(); - const mockUpdateNotifications = - messengerMocks.mockPutNotificationPreferences; - return { ...messengerMocks, mockUpdateNotifications }; + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + let requestBody: unknown; + mockTriggerUpdate.on('request', (_req, _interceptor, body) => { + requestBody = JSON.parse(body as string); + }); + return { + ...messengerMocks, + mockTriggerUpdate, + getRequestBody: () => requestBody, + }; }; it('disables notifications for given accounts', async () => { const { messenger, - mockUpdateNotifications, + mockTriggerUpdate, + getRequestBody, mockDeletePushNotificationLinks, + mockPutNotificationPreferences, } = arrangeMocks(); const controller = new NotificationServicesController({ messenger, @@ -1001,8 +1212,13 @@ describe('NotificationServicesController', () => { await controller.disableAccounts([ADDRESS_1]); - expect(mockUpdateNotifications).toHaveBeenCalled(); + expect(mockTriggerUpdate.isDone()).toBe(true); + expect(getRequestBody()).toStrictEqual([ + { address: ADDRESS_1.toLowerCase(), enabled: false }, + ]); expect(mockDeletePushNotificationLinks).toHaveBeenCalledWith([ADDRESS_1]); + // Subscriptions live in the Trigger API, so AUS is left untouched. + expect(mockPutNotificationPreferences).not.toHaveBeenCalled(); }); it('throws errors when invalid auth', async () => { @@ -1028,19 +1244,29 @@ describe('NotificationServicesController', () => { describe('enableAccounts', () => { const arrangeMocks = (): ReturnType & { - mockUpdateNotifications: jest.Mock; + mockTriggerUpdate: nock.Scope; + getRequestBody: () => unknown; } => { const messengerMocks = mockNotificationMessenger(); - const mockUpdateNotifications = - messengerMocks.mockPutNotificationPreferences; - return { ...messengerMocks, mockUpdateNotifications }; + const mockTriggerUpdate = mockUpdateOnChainNotifications(); + let requestBody: unknown; + mockTriggerUpdate.on('request', (_req, _interceptor, body) => { + requestBody = JSON.parse(body as string); + }); + return { + ...messengerMocks, + mockTriggerUpdate, + getRequestBody: () => requestBody, + }; }; it('enables notifications for given accounts', async () => { const { messenger, mockAddPushNotificationLinks, - mockUpdateNotifications, + mockTriggerUpdate, + getRequestBody, + mockPutNotificationPreferences, } = arrangeMocks(); const controller = new NotificationServicesController({ messenger, @@ -1049,8 +1275,13 @@ describe('NotificationServicesController', () => { await controller.enableAccounts([ADDRESS_1]); - expect(mockUpdateNotifications).toHaveBeenCalled(); + expect(mockTriggerUpdate.isDone()).toBe(true); + expect(getRequestBody()).toStrictEqual([ + { address: ADDRESS_1.toLowerCase(), enabled: true }, + ]); expect(mockAddPushNotificationLinks).toHaveBeenCalledWith([ADDRESS_1]); + // Subscriptions live in the Trigger API, so AUS is left untouched. + expect(mockPutNotificationPreferences).not.toHaveBeenCalled(); }); it('throws errors when invalid auth', async () => { @@ -1087,11 +1318,10 @@ describe('NotificationServicesController', () => { } => { const messengerMocks = mockNotificationMessenger(); messengerMocks.mockGetNotificationPreferences.mockResolvedValue( - prefsFromAddressesWithMarketingInAppNotifications( - [{ address: '0xTestAddress', enabled: true }], - true, - ), + mockPreferencesWithMarketingInApp(true), ); + // Supplies the enabled wallet-activity addresses. + mockGetOnChainNotificationsConfig(); const mockFeatureAnnouncementAPIResult = createMockFeatureAnnouncementAPIResult(); @@ -1215,10 +1445,7 @@ describe('NotificationServicesController', () => { it('should not fetch feature announcements if AUS marketing in-app notifications are disabled', async () => { const { messenger, ...mocks } = arrangeMocks(); mocks.mockGetNotificationPreferences.mockResolvedValue( - prefsFromAddressesWithMarketingInAppNotifications( - [{ address: '0xTestAddress', enabled: true }], - false, - ), + mockPreferencesWithMarketingInApp(false), ); const controller = arrangeController(messenger); @@ -1236,15 +1463,62 @@ describe('NotificationServicesController', () => { expect(mocks.mockFeatureAnnouncementsAPI.isDone()).toBe(false); }); + it('should not fetch wallet notifications if AUS wallet-activity in-app notifications are disabled', async () => { + const { messenger, ...mocks } = arrangeMocks(); + mocks.mockGetNotificationPreferences.mockResolvedValue( + mockPreferences({ + walletActivity: { + inAppNotificationsEnabled: false, + pushNotificationsEnabled: true, + accounts: [], + }, + marketing: { + inAppNotificationsEnabled: true, + pushNotificationsEnabled: false, + }, + }), + ); + const controller = arrangeController(messenger); + + const result = await controller.fetchAndUpdateMetamaskNotifications(); + + expect( + result.filter( + (notification) => notification.type === TRIGGER_TYPES.ETH_SENT, + ), + ).toHaveLength(0); + expect(mocks.mockOnChainNotificationsAPI.isDone()).toBe(false); + + // The wallet-activity toggle must not affect other notification types. + expect(mocks.mockFeatureAnnouncementsAPI.isDone()).toBe(true); + }); + + it('should still fetch wallet notifications when the preferences blob is unavailable', async () => { + const { messenger, ...mocks } = arrangeMocks(); + mocks.mockGetNotificationPreferences.mockRejectedValue( + new Error('mock storage failure'), + ); + mockErrorLog(); + const controller = arrangeController(messenger); + + // A storage outage must not silently empty the notification list. + const result = await controller.fetchAndUpdateMetamaskNotifications(); + + expect( + result.filter( + (notification) => notification.type === TRIGGER_TYPES.ETH_SENT, + ), + ).toHaveLength(1); + expect(mocks.mockOnChainNotificationsAPI.isDone()).toBe(true); + }); + it('should handle errors gracefully when fetching notifications', async () => { const { messenger, mockGetNotificationPreferences } = mockNotificationMessenger(); mockGetNotificationPreferences.mockResolvedValue( - prefsFromAddressesWithMarketingInAppNotifications( - [{ address: '0xTestAddress', enabled: true }], - true, - ), + mockPreferencesWithMarketingInApp(true), ); + mockGetOnChainNotificationsConfig(); // Mock APIs to fail mockFetchFeatureAnnouncementNotifications({ status: 500 }); @@ -1526,6 +1800,7 @@ describe('NotificationServicesController', () => { it('should sign a user in if not already signed in', async () => { const mocks = arrangeMocks(); + mockGetOnChainNotificationsConfig(); mocks.mockIsSignedIn.mockReturnValue(false); // mock that auth is not enabled const controller = new NotificationServicesController({ messenger: mocks.messenger, @@ -1544,6 +1819,7 @@ describe('NotificationServicesController', () => { // No AUS preferences yet — fresh initialization. configurePrefs: (mock) => mock.mockResolvedValueOnce(null), }); + mockGetOnChainNotificationsConfig(); const controller = new NotificationServicesController({ messenger: mocks.messenger, @@ -1591,11 +1867,9 @@ describe('NotificationServicesController', () => { it('should not create new notification subscriptions when enabling an account that already has notifications', async () => { const mocks = arrangeMocks({ // Mock fully-initialized existing notifications - configurePrefs: (mock) => - mock.mockResolvedValueOnce( - prefsFromAddresses([{ address: ADDRESS_1, enabled: true }]), - ), + configurePrefs: (mock) => mock.mockResolvedValueOnce(mockPreferences()), }); + mockGetOnChainNotificationsConfig(); const controller = new NotificationServicesController({ messenger: mocks.messenger, @@ -1689,18 +1963,30 @@ describe('NotificationServicesController', () => { } => { const messengerMocks = mockNotificationMessenger(); const mockGetConfig = messengerMocks.mockGetNotificationPreferences; - mockGetConfig.mockResolvedValueOnce( - prefsFromAddresses([ - { address: ADDRESS_1, enabled: true }, - { address: ADDRESS_2, enabled: true }, - ]), - ); + mockGetConfig.mockResolvedValueOnce(mockPreferences()); + messengerMocks.mockKeyringControllerGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + accounts: [ADDRESS_1, ADDRESS_2], + type: KeyringTypes.hd, + metadata: { id: 'srp-1', name: 'SRP 1' }, + }, + ], + }); return { ...messengerMocks, mockGetConfig }; }; it('calls push controller and enables notifications for accounts that have subscribed to notifications', async () => { const { messenger, mockGetConfig, mockEnablePushNotifications } = arrangeMocks(); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: true }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); const controller = new NotificationServicesController({ messenger, env: { featureAnnouncements: featureAnnouncementsEnv }, @@ -1712,16 +1998,94 @@ describe('NotificationServicesController', () => { // Assert expect(mockGetConfig).toHaveBeenCalled(); - // Addresses are stored lower-cased in AUS preferences. expect(mockEnablePushNotifications).toHaveBeenCalledWith([ ADDRESS_1.toLowerCase(), - ADDRESS_2.toLowerCase(), ]); }); - it('handles errors gracefully when fetching notification config fails', async () => { - const mocks = mockNotificationMessenger(); - mocks.mockGetNotificationPreferences.mockRejectedValueOnce( + it('unregisters the device when no account has notifications enabled', async () => { + const { + messenger, + mockEnablePushNotifications, + mockDisablePushNotifications, + } = arrangeMocks(); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: false }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + state: { isNotificationServicesEnabled: true }, + }); + + await controller.enablePushNotifications(); + + // The push API rejects a registration with no addresses, which would + // leave the device's existing links in place. + expect(mockDisablePushNotifications).toHaveBeenCalled(); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + }); + + it('leaves existing push links alone when the Trigger API is unreadable', async () => { + const { + messenger, + mockEnablePushNotifications, + mockDisablePushNotifications, + } = arrangeMocks(); + mockGetOnChainNotificationsConfig({ + status: 500, + body: { error: 'mock api failure' }, + }); + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + state: { isNotificationServicesEnabled: true }, + }); + + await controller.enablePushNotifications(); + + // An unreadable list is not an empty one: unregistering here would + // silently stop push for every account over a transient outage. + expect(mockDisablePushNotifications).not.toHaveBeenCalled(); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + }); + + it('unregisters the device when the wallet-activity push toggle is off', async () => { + const { + messenger, + mockGetConfig, + mockEnablePushNotifications, + mockDisablePushNotifications, + } = arrangeMocks(); + mockGetConfig.mockReset(); + mockGetConfig.mockResolvedValue( + mockPreferences({ + walletActivity: { + inAppNotificationsEnabled: true, + pushNotificationsEnabled: false, + accounts: [], + }, + }), + ); + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + state: { isNotificationServicesEnabled: true }, + }); + + await controller.enablePushNotifications(); + + expect(mockDisablePushNotifications).toHaveBeenCalled(); + expect(mockEnablePushNotifications).not.toHaveBeenCalled(); + }); + + it('handles errors gracefully when fetching the bearer token fails', async () => { + const mocks = arrangeMocks(); + mocks.mockGetBearerToken.mockRejectedValue( new Error('mock api failure'), ); mockErrorLog(); @@ -2074,7 +2438,7 @@ function mockNotificationMessenger(): { const mockGetNotificationPreferences = typedMockAction().mockResolvedValue( - prefsFromAddresses([{ address: '0xTestAddress', enabled: true }]), + mockPreferences(), ); const mockPutNotificationPreferences = diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts index 5fac52146a2..e812a1c6b36 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts @@ -4,7 +4,6 @@ import type { NotificationPreferences, PerpsPreference, SocialAIPreference, - WalletActivityAccount, } from '@metamask/authenticated-user-storage'; import { DEFAULT_AGENTIC_CLI_PREFERENCES, @@ -29,7 +28,6 @@ import type { } from '@metamask/keyring-controller'; import type { Messenger } from '@metamask/messenger'; import type { AuthenticationController } from '@metamask/profile-sync-controller'; -import type { Hex } from '@metamask/utils'; import { assert } from '@metamask/utils'; import { debounce } from 'lodash'; import log from 'loglevel'; @@ -51,6 +49,7 @@ import { getAPINotifications, getNotificationsApiConfigCached, markNotificationsAsRead, + updateOnChainNotifications, } from './services/api-notifications.js'; import { getFeatureAnnouncementNotifications } from './services/feature-announcements.js'; import { createPerpOrderNotification } from './services/perp-notifications.js'; @@ -254,79 +253,60 @@ export { } from '@metamask/authenticated-user-storage'; /** - * Builds wallet-activity preferences from the keyring's current accounts. + * Returns the subset of `accounts` that has a wallet-activity subscription in + * the Trigger API, which is the source of truth for the per-address enabled bit. * - * @param accounts - The keyring accounts to build wallet-activity entries for. - * @returns An array of wallet-activity account entries (lower-cased addresses). - */ -const buildWalletActivityAccounts = ( - accounts: { address: string; enabled: boolean }[], -): WalletActivityAccount[] => - accounts.map(({ address, enabled }) => { - const lowercased = address.toLowerCase(); - return { - address: lowercased as Hex, - enabled, - }; - }); - -/** - * Builds wallet-activity initialization from the Trigger API. If Trigger has no - * enabled entries for the current keyring accounts, this is a first-time - * notification setup and all current accounts should start enabled. + * The address universe is always supplied by the caller from the keyring, so the + * result cannot contain an address this installation does not hold. * - * @param bearerToken - JWT used to query Trigger API. - * @param accounts - The keyring accounts to initialize. + * @param bearerToken - JWT used to query the Trigger API. + * @param accounts - The keyring accounts to check. * @param env - The environment to use for the Trigger API call. - * @returns Wallet-activity account initialization entries. + * @returns The enabled addresses, as returned by the Trigger API, or `null` if + * the Trigger API could not be read. */ -const buildWalletActivityAccountsFromTriggerConfig = async ( +const getEnabledAccounts = async ( bearerToken: string, accounts: string[], env: ENV, -): Promise<{ address: string; enabled: boolean }[]> => { +): Promise => { const triggerConfig = await getNotificationsApiConfigCached( bearerToken, accounts, env, ); - const triggerConfigByAddress = new Map( - triggerConfig.map(({ address, enabled }) => [ - address.toLowerCase(), - enabled, - ]), - ); - const hasEnabledTriggerAccount = accounts.some( - (address) => triggerConfigByAddress.get(address.toLowerCase()) === true, - ); - return accounts.map((address) => ({ - address, - enabled: hasEnabledTriggerAccount - ? (triggerConfigByAddress.get(address.toLowerCase()) ?? false) - : true, - })); + if (triggerConfig === null) { + return null; + } + + return triggerConfig + .filter((addressConfig) => Boolean(addressConfig.enabled)) + .map((addressConfig) => addressConfig.address); }; /** * Builds a fresh `NotificationPreferences` blob using hardcoded defaults for - * Perps, Social AI, and Agentic CLI, the supplied wallet-activity accounts and - * the user's marketing/product-announcement flags. + * Perps, Social AI, and Agentic CLI and the user's marketing/product-announcement + * flags. + * + * `walletActivity.accounts` is deliberately left empty. Addresses are scoped to a + * keyring, but this blob is keyed by canonical profile ID, which pairing shares + * across every SRP belonging to the same user — so storing them here pools the + * addresses of unrelated SRPs. Subscriptions live in the Trigger API instead. * - * @param walletActivityAccounts - The wallet-activity account config to initialize. * @param hasMarketingConsent - Whether marketing push notifications should be enabled. * @param productAnnouncementEnabled - Whether marketing in-app notifications should be enabled. * @returns A complete `NotificationPreferences` object. */ const buildFreshPreferences = ( - walletActivityAccounts: { address: string; enabled: boolean }[], hasMarketingConsent: boolean, productAnnouncementEnabled: boolean, ): NotificationPreferences => ({ walletActivity: { inAppNotificationsEnabled: true, pushNotificationsEnabled: true, - accounts: buildWalletActivityAccounts(walletActivityAccounts), + accounts: [], }, marketing: { inAppNotificationsEnabled: productAnnouncementEnabled, @@ -829,69 +809,45 @@ export class NotificationServicesController extends BaseController< } /** - * Updates the `walletActivity.accounts` entries in the user's - * notification-preferences blob in {@link AuthenticatedUserStorageService}. + * Reads the global wallet-activity push toggle from + * {@link AuthenticatedUserStorageService}. * - * `putNotificationPreferences` replaces the entire blob, so we read the - * current preferences, merge the supplied updates into - * `walletActivity.accounts`, and write the result back. This helper is only - * meant to be used for incremental updates (enable/disable individual - * accounts) after the preferences blob has already been initialized via - * {@link createOnChainTriggers}; callers should not rely on it to perform - * first-time initialization. + * Only the channel boolean is read. It is a user-level setting that contains + * no addresses, so sharing it across a paired profile is correct. The address + * list in the same blob is deliberately ignored — see + * {@link buildFreshPreferences}. * - * @param updates - Addresses to register, each with the desired `enabled` flag + * A missing or unreadable blob counts as enabled, matching the API's "every + * toggle is on unless stated otherwise" default, so that a storage outage does + * not silently stop notifications. + * + * @returns Whether wallet-activity push notifications are enabled. */ - async #registerWalletActivityAddresses( - updates: { address: string; enabled: boolean }[], - ): Promise { - if (updates.length === 0) { - return; - } - - const currentPreferences = await this.messenger + async #isWalletActivityPushEnabled(): Promise { + const preferences = await this.messenger .call('AuthenticatedUserStorageService:getNotificationPreferences') - .catch((error) => { - log.error( - 'Failed to get notification preferences. Re-initializing them instead.', - error, - ); - // TODO: return null once validation error captured - // return null; - throw error; - }); + .catch(() => null); - if (!currentPreferences) { - log.warn( - 'Preferences blob not yet initialized; run `createOnChainTriggers` first.', - ); - return; - } + return preferences?.walletActivity.pushNotificationsEnabled ?? true; + } - const accountsByAddress = new Map( - currentPreferences.walletActivity.accounts.map((account) => [ - account.address.toLowerCase(), - { ...account, address: account.address.toLowerCase() as Hex }, - ]), - ); - for (const update of updates) { - const address = update.address.toLowerCase() as Hex; - accountsByAddress.set(address, { address, enabled: update.enabled }); + /** + * Registers this device for push notifications on the given addresses. + * + * An empty list cannot be sent: the push API rejects a registration with no + * addresses, and because that request is what performs the delete-and-reinsert + * of the device's links, a rejection leaves the previous links in place and + * push keeps arriving. So "no addresses" has to mean unregistering the device. + * + * @param addresses - The addresses to receive push notifications for. + */ + async #registerPushNotifications(addresses: string[]): Promise { + if (addresses.length === 0) { + await this.#pushNotifications.disablePushNotifications(); + return; } - const nextPreferences: NotificationPreferences = { - ...currentPreferences, - walletActivity: { - ...currentPreferences.walletActivity, - accounts: [...accountsByAddress.values()], - }, - }; - - await this.messenger.call( - 'AuthenticatedUserStorageService:putNotificationPreferences', - nextPreferences, - this.#featureAnnouncementEnv.platform, - ); + await this.#pushNotifications.enablePushNotifications(addresses); } /** @@ -978,15 +934,27 @@ export class NotificationServicesController extends BaseController< */ public async enablePushNotifications(): Promise { try { - const preferences = await this.messenger.call( - 'AuthenticatedUserStorageService:getNotificationPreferences', + if (!(await this.#isWalletActivityPushEnabled())) { + await this.#pushNotifications.disablePushNotifications(); + return; + } + + const { bearerToken } = await this.#getBearerToken(); + const { accounts } = this.#accounts.listAccounts(); + const enabledAddresses = await getEnabledAccounts( + bearerToken, + accounts, + this.#env, ); - const enabledAddresses = (preferences?.walletActivity.accounts ?? []) - .filter((account) => account.enabled) - .map((account) => account.address); - if (enabledAddresses.length > 0) { - await this.#pushNotifications.enablePushNotifications(enabledAddresses); + + if (enabledAddresses === null) { + // "No addresses" unregisters the device, so an unreadable subscription + // list must not be treated as an empty one: leave the existing links + // alone until we can read the real state. + return; } + + await this.#registerPushNotifications(enabledAddresses); } catch { // Do nothing, failing silently. } @@ -1005,13 +973,23 @@ export class NotificationServicesController extends BaseController< try { this.#setIsCheckingAccountsPresence(true); - const preferences = await this.messenger.call( - 'AuthenticatedUserStorageService:getNotificationPreferences', + const { bearerToken } = await this.#getBearerToken(); + const triggerConfig = await getNotificationsApiConfigCached( + bearerToken, + accounts, + this.#env, ); + + if (triggerConfig === null) { + // Reporting every account as disabled would misrepresent the user's + // settings, so surface the failure instead. + throw new Error('Failed to read wallet-activity subscriptions'); + } + const enabledByAddress = new Map( - (preferences?.walletActivity.accounts ?? []).map((account) => [ - account.address.toLowerCase(), - account.enabled, + triggerConfig.map((addressConfig) => [ + addressConfig.address.toLowerCase(), + Boolean(addressConfig.enabled), ]), ); @@ -1078,7 +1056,8 @@ export class NotificationServicesController extends BaseController< const { accounts } = this.#accounts.listAccounts(); - // 1. Read existing AUS notification preferences and initialize only if absent. + // 1. Read existing AUS notification preferences. Their absence is what + // marks a first-time setup, and they are initialized in step 3. const preferences = await this.messenger.call( 'AuthenticatedUserStorageService:getNotificationPreferences', ); @@ -1087,45 +1066,65 @@ export class NotificationServicesController extends BaseController< const productAnnouncementEnabled = Boolean( opts?.productAnnouncementEnabled, ); - let nextPreferences: NotificationPreferences | undefined; - if (preferences === null) { - const walletActivityAccounts = - await buildWalletActivityAccountsFromTriggerConfig( - bearerToken, - accounts, - this.#env, - ); + const isFirstTimeSetup = preferences === null; + + const isPushEnabled = + preferences?.walletActivity.pushNotificationsEnabled ?? true; + + // 2. Subscribe the keyring's accounts on first-time setup only. + // + // This method also runs on the daily re-subscribe, so the absence of a + // preferences blob — not the absence of subscriptions — is what marks a + // genuine first-time setup. Keying off "no subscriptions" would re-enable + // every account daily for a user who had turned them all off. + // + // Even at first-time setup, existing subscriptions win: a user upgrading + // from a client that never wrote a preferences blob keeps whichever + // accounts they had already disabled. + let accountsWithNotifications = await getEnabledAccounts( + bearerToken, + accounts, + this.#env, + ); + + if (accountsWithNotifications === null) { + // An unreadable subscription list is not an empty one. Subscribing + // every account here would re-enable ones the user had turned off, and + // registering push for that guessed list would send activity for them, + // so fail and let the caller retry. + throw new Error('Failed to read wallet-activity subscriptions'); + } - nextPreferences = buildFreshPreferences( - walletActivityAccounts, - hasMarketingConsent, - productAnnouncementEnabled, + if (isFirstTimeSetup && accountsWithNotifications.length === 0) { + await updateOnChainNotifications( + bearerToken, + accounts.map((address) => ({ address, enabled: true })), + this.#env, ); + accountsWithNotifications = accounts; } - if (nextPreferences) { + // 3. Initialize the preferences blob, only once the subscriptions above + // are in place. Its existence is what makes the next run a re-subscribe + // rather than a first-time setup, so writing it earlier would let a + // failed subscribe leave the user enabled with no accounts subscribed + // and no second chance to seed them. + if (isFirstTimeSetup) { await this.messenger.call( 'AuthenticatedUserStorageService:putNotificationPreferences', - nextPreferences, + buildFreshPreferences(hasMarketingConsent, productAnnouncementEnabled), this.#featureAnnouncementEnv.platform, ); } - const effectivePreferences = nextPreferences ?? preferences; - const accountsWithNotifications = ( - effectivePreferences?.walletActivity.accounts ?? [] - ) - .filter((account) => account.enabled) - .map((account) => account.address); - if (opts.registerPushNotifications ?? true) { // Attempt FCM/device registration only; clients must request OS permission separately. - this.#pushNotifications - .enablePushNotifications(accountsWithNotifications) - .catch(() => { - // Do Nothing - }); + this.#registerPushNotifications( + isPushEnabled ? accountsWithNotifications : [], + ).catch(() => { + // Do Nothing + }); } // Update the state of the controller @@ -1221,11 +1220,12 @@ export class NotificationServicesController extends BaseController< public async disableAccounts(accounts: string[]): Promise { try { this.#updateUpdatingAccountsState(accounts); - // Sign-in gate. - await this.#getBearerToken(); + const { bearerToken } = await this.#getBearerToken(); - await this.#registerWalletActivityAddresses( + await updateOnChainNotifications( + bearerToken, accounts.map((address) => ({ address, enabled: false })), + this.#env, ); await this.#pushNotifications.deletePushNotificationLinks(accounts); @@ -1255,10 +1255,11 @@ export class NotificationServicesController extends BaseController< try { this.#updateUpdatingAccountsState(accounts); - // Sign-in gate. - await this.#getBearerToken(); - await this.#registerWalletActivityAddresses( + const { bearerToken } = await this.#getBearerToken(); + await updateOnChainNotifications( + bearerToken, accounts.map((address) => ({ address, enabled: true })), + this.#env, ); await this.#pushNotifications.addPushNotificationLinks(accounts); @@ -1307,22 +1308,31 @@ export class NotificationServicesController extends BaseController< // Raw On Chain Notifications const rawOnChainNotifications: NormalisedAPINotification[] = []; - if (isGlobalNotifsEnabled) { + const isWalletActivityInAppEnabled = + notificationPreferences?.walletActivity.inAppNotificationsEnabled ?? + true; + if (isGlobalNotifsEnabled && isWalletActivityInAppEnabled) { try { const { bearerToken } = await this.#getBearerToken(); - const addressesWithNotifications = ( - notificationPreferences?.walletActivity.accounts ?? [] - ) - .filter((account) => account.enabled) - .map((account) => account.address); - const notifications = await getAPINotifications( + // Addresses come from the keyring, so this installation can only ever + // ask for activity on accounts it actually holds. The Trigger API + // narrows that to the ones the user has enabled. + const { accounts } = this.#accounts.listAccounts(); + const addressesWithNotifications = await getEnabledAccounts( bearerToken, - addressesWithNotifications, - this.#locale(), - this.#featureAnnouncementEnv.platform, + accounts, this.#env, - ).catch(() => []); - rawOnChainNotifications.push(...notifications); + ); + if (addressesWithNotifications !== null) { + const notifications = await getAPINotifications( + bearerToken, + addressesWithNotifications, + this.#locale(), + this.#featureAnnouncementEnv.platform, + this.#env, + ).catch(() => []); + rawOnChainNotifications.push(...notifications); + } } catch { // Do nothing } diff --git a/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mockServices.ts b/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mockServices.ts index bdab54c3f6d..92441dda335 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mockServices.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/__fixtures__/mockServices.ts @@ -2,6 +2,7 @@ import nock from 'nock'; import { getMockOnChainNotificationsConfig, + getMockUpdateOnChainNotifications, getMockFeatureAnnouncementResponse, getMockListNotificationsResponse, getMockMarkNotificationsAsReadResponse, @@ -39,6 +40,19 @@ export const mockGetOnChainNotificationsConfig = ( return mockEndpoint; }; +export const mockUpdateOnChainNotifications = ( + mockReply?: MockReply, +): nock.Scope => { + const mockResponse = getMockUpdateOnChainNotifications(); + const reply = mockReply ?? { status: 204 }; + + const mockEndpoint = nock(mockResponse.url) + .post('') + .reply(reply.status, reply.body); + + return mockEndpoint; +}; + export const mockGetAPINotifications = (mockReply?: MockReply): nock.Scope => { const mockResponse = getMockListNotificationsResponse(); const reply = mockReply ?? { status: 200, body: mockResponse.response }; diff --git a/packages/notification-services-controller/src/NotificationServicesController/mocks/mockResponses.ts b/packages/notification-services-controller/src/NotificationServicesController/mocks/mockResponses.ts index 279067a5f0b..a30a1b9272d 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/mocks/mockResponses.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/mocks/mockResponses.ts @@ -1,6 +1,7 @@ import { NOTIFICATION_API_LIST_ENDPOINT, NOTIFICATION_API_MARK_ALL_AS_READ_ENDPOINT, + TRIGGER_API_NOTIFICATIONS_ENDPOINT, TRIGGER_API_NOTIFICATIONS_QUERY_ENDPOINT, } from '../services/api-notifications.js'; import { FEATURE_ANNOUNCEMENT_API } from '../services/feature-announcements.js'; @@ -33,6 +34,14 @@ export const getMockOnChainNotificationsConfig = (): MockResponse => { } satisfies MockResponse; }; +export const getMockUpdateOnChainNotifications = (): MockResponse => { + return { + url: TRIGGER_API_NOTIFICATIONS_ENDPOINT(), + requestMethod: 'POST', + response: null, + } satisfies MockResponse; +}; + export const MOCK_RAW_ON_CHAIN_NOTIFICATIONS = createMockRawOnChainNotifications(); diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.test.ts b/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.test.ts index c2253737853..6f991f0d2c7 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.test.ts @@ -1,5 +1,6 @@ import { mockGetOnChainNotificationsConfig, + mockUpdateOnChainNotifications, mockGetAPINotifications, mockMarkNotificationsAsRead, } from '../__fixtures__/mockServices.js'; @@ -8,11 +9,16 @@ import { createMockPlatformNotification, } from '../mocks/index.js'; import * as OnChainNotifications from './api-notifications.js'; +import { notificationsConfigCache } from './notification-config-cache.js'; const MOCK_BEARER_TOKEN = 'MOCK_BEARER_TOKEN'; const MOCK_ADDRESSES = ['0x123', '0x456', '0x789']; describe('On Chain Notifications - getAPINotificationsConfig()', () => { + beforeEach(() => { + notificationsConfigCache.clear(); + }); + it('should return notification config for addresses', async () => { const mockEndpoint = mockGetOnChainNotificationsConfig({ status: 200, @@ -40,7 +46,7 @@ describe('On Chain Notifications - getAPINotificationsConfig()', () => { expect(result).toStrictEqual([]); }); - it('should return [] if endpoint fails', async () => { + it('should return null if endpoint fails, to keep it distinct from "nothing is enabled"', async () => { const mockBadEndpoint = mockGetOnChainNotificationsConfig({ status: 500, body: { error: 'mock api failure' }, @@ -52,10 +58,106 @@ describe('On Chain Notifications - getAPINotificationsConfig()', () => { ); expect(mockBadEndpoint.isDone()).toBe(true); + expect(result).toBeNull(); + }); + + it('should return [] if the endpoint reports no subscriptions', async () => { + const mockEndpoint = mockGetOnChainNotificationsConfig({ + status: 200, + body: [], + }); + + const result = await OnChainNotifications.getNotificationsApiConfigCached( + MOCK_BEARER_TOKEN, + MOCK_ADDRESSES, + ); + + expect(mockEndpoint.isDone()).toBe(true); expect(result).toStrictEqual([]); }); }); +describe('On Chain Notifications - updateOnChainNotifications()', () => { + beforeEach(() => { + notificationsConfigCache.clear(); + }); + + const mockAddressesWithStatus = [ + { address: '0x123', enabled: true }, + { address: '0x456', enabled: false }, + { address: '0x789', enabled: true }, + ]; + + it('should successfully update notification subscriptions', async () => { + const mockEndpoint = mockUpdateOnChainNotifications(); + + await OnChainNotifications.updateOnChainNotifications( + MOCK_BEARER_TOKEN, + mockAddressesWithStatus, + ); + + expect(mockEndpoint.isDone()).toBe(true); + }); + + it('should bail early if given empty list of addresses', async () => { + const mockEndpoint = mockUpdateOnChainNotifications(); + + await OnChainNotifications.updateOnChainNotifications( + MOCK_BEARER_TOKEN, + [], + ); + + expect(mockEndpoint.isDone()).toBe(false); // bailed before API was called + }); + + it('should cache the new subscriptions once the API accepts them', async () => { + mockUpdateOnChainNotifications(); + + await OnChainNotifications.updateOnChainNotifications( + MOCK_BEARER_TOKEN, + mockAddressesWithStatus, + ); + + expect(notificationsConfigCache.get(MOCK_ADDRESSES)).toStrictEqual( + mockAddressesWithStatus, + ); + }); + + it('should reject and leave the cache untouched if the API rejects the update', async () => { + const mockBadEndpoint = mockUpdateOnChainNotifications({ + status: 500, + body: { error: 'mock api failure' }, + }); + + await expect( + OnChainNotifications.updateOnChainNotifications( + MOCK_BEARER_TOKEN, + mockAddressesWithStatus, + ), + ).rejects.toThrow('Failed to update on-chain notifications: 500'); + + expect(mockBadEndpoint.isDone()).toBe(true); + expect(notificationsConfigCache.get(MOCK_ADDRESSES)).toBeNull(); + }); + + it('should lower-case addresses before sending them', async () => { + const mockEndpoint = mockUpdateOnChainNotifications(); + let requestBody: unknown; + mockEndpoint.on('request', (_req, _interceptor, body) => { + requestBody = JSON.parse(body as string); + }); + + await OnChainNotifications.updateOnChainNotifications(MOCK_BEARER_TOKEN, [ + { address: '0xAbCdEf', enabled: true }, + ]); + + expect(mockEndpoint.isDone()).toBe(true); + expect(requestBody).toStrictEqual([ + { address: '0xabcdef', enabled: true }, + ]); + }); +}); + describe('On Chain Notifications - getAPINotifications()', () => { it('should return a list of notifications', async () => { const mockEndpoint = mockGetAPINotifications(); diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.ts b/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.ts index 3fcbc95a0d5..b94dccf37f6 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/api-notifications.ts @@ -34,6 +34,10 @@ export const TRIGGER_API_NOTIFICATIONS_QUERY_ENDPOINT = ( env: ENV = 'prd', ): string => `${TRIGGER_API(env)}/api/v2/notifications/query`; +// Creates/updates account notification subscriptions for each account provided +export const TRIGGER_API_NOTIFICATIONS_ENDPOINT = (env: ENV = 'prd'): string => + `${TRIGGER_API(env)}/api/v2/notifications`; + // Lists notifications for each address provided export const NOTIFICATION_API_LIST_ENDPOINT = (env: ENV = 'prd'): string => `${NOTIFICATION_API(env)}/api/v4/notifications`; @@ -51,13 +55,16 @@ export const NOTIFICATION_API_MARK_ALL_AS_READ_ENDPOINT = ( * @param env - the environment to use for the API call * NOTE the API will return addresses config with false if they have not been created before. * NOTE this is cached for 1s to prevent multiple update calls - * @returns object of notification config, or null if missing + * @returns the config for each requested address, or `null` if the config could + * not be read. An empty array means the API answered and no address is + * subscribed; callers must not read a failure as "nothing is enabled", since + * acting on that would re-subscribe or unregister addresses behind the user. */ export async function getNotificationsApiConfigCached( bearerToken: string, addresses: string[], env: ENV = 'prd', -): Promise<{ address: string; enabled: boolean }[]> { +): Promise<{ address: string; enabled: boolean }[] | null> { if (addresses.length === 0) { return []; } @@ -72,7 +79,7 @@ export async function getNotificationsApiConfigCached( type RequestBody = { address: string }[]; type Response = { address: string; enabled: boolean }[]; const body: RequestBody = normalizedAddresses.map((address) => ({ address })); - const apiResponse = await makeApiCall( + const result = await makeApiCall( bearerToken, TRIGGER_API_NOTIFICATIONS_QUERY_ENDPOINT(env), 'POST', @@ -81,7 +88,9 @@ export async function getNotificationsApiConfigCached( .then((response) => (response.ok ? response.json() : null)) .catch(() => null); - const result = apiResponse ?? []; + if (result === null) { + return null; + } if (result.length > 0) { notificationsConfigCache.set(result); @@ -90,6 +99,53 @@ export async function getNotificationsApiConfigCached( return result; } +/** + * Creates or removes wallet-activity subscriptions for the given addresses. + * + * The endpoint is a per-address upsert-or-delete batch rather than a full + * replace, so two installations authenticating as the same profile do not + * clobber each other's subscriptions. + * + * @param bearerToken - jwt + * @param addresses - addresses to subscribe (`enabled: true`) or unsubscribe (`enabled: false`) + * @param env - the environment to use for the API call + * @throws if the request could not be made or the API rejected it, so callers + * do not report a subscription change the server never applied. + */ +export async function updateOnChainNotifications( + bearerToken: string, + addresses: { address: string; enabled: boolean }[], + env: ENV = 'prd', +): Promise { + if (addresses.length === 0) { + return; + } + + const normalizedAddresses = addresses.map((item) => ({ + ...item, + address: item.address.toLowerCase(), + })); + + type RequestBody = { address: string; enabled: boolean }[]; + const body: RequestBody = normalizedAddresses; + const response = await makeApiCall( + bearerToken, + TRIGGER_API_NOTIFICATIONS_ENDPOINT(env), + 'POST', + body, + ); + + if (!response.ok) { + throw new Error( + `Failed to update on-chain notifications: ${response.status}`, + ); + } + + // Seeded only once the server has accepted the change: the settings UI reads + // this back for the whole TTL, so a rejected write must not look applied. + notificationsConfigCache.set(normalizedAddresses); +} + /** * Fetches on-chain notifications for the given addresses *