diff --git a/packages/notification-services-controller/CHANGELOG.md b/packages/notification-services-controller/CHANGELOG.md index 56b9735388b..22eea8e6628 100644 --- a/packages/notification-services-controller/CHANGELOG.md +++ b/packages/notification-services-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Skip `subscriptionAccountsSeen`, `metamaskNotificationsList`, and `metamaskNotificationsReadList` writes when the computed value is unchanged, so `stateChange` and a full state persist no longer fire on every `listAccounts`/notification fetch/mark-as-read call that doesn't actually change anything ([#10275](https://github.com/MetaMask/core/pull/10275)) + ## [29.0.0] ### Changed diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts index 79939136cd7..38afcb31aed 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts @@ -1683,6 +1683,99 @@ describe('NotificationServicesController', () => { // Should still return empty array and not throw expect(Array.isArray(result)).toBe(true); }); + + /** + * `fetchAndUpdateMetamaskNotifications` always flips + * `isFetchingMetamaskNotifications` true then false, which is a real, + * expected `stateChange` on every call. To isolate whether + * `metamaskNotificationsList` itself was touched, inspect the Immer + * patches the event carries rather than whether it fired at all. + * + * @param messenger - the controller's messenger. + * @returns a function returning whether any observed stateChange patch + * touched `metamaskNotificationsList`. + */ + const arrangeNotificationsListPatchSpy = ( + messenger: NotificationServicesControllerMessenger, + ): (() => boolean) => { + let sawNotificationsListPatch = false; + messenger.subscribe( + 'NotificationServicesController:stateChange', + (_state, patches) => { + if ( + patches.some( + (patch) => patch.path[0] === 'metamaskNotificationsList', + ) + ) { + sawNotificationsListPatch = true; + } + }, + ); + return () => sawNotificationsListPatch; + }; + + it('does not touch metamaskNotificationsList when a repeat fetch returns the same notifications', async () => { + const { + messenger, + mockFeatureAnnouncementAPIResult, + mockOnChainNotificationsAPIResult, + } = arrangeMocks(); + const controller = arrangeController(messenger); + + await controller.fetchAndUpdateMetamaskNotifications(); + + const sawNotificationsListPatch = + arrangeNotificationsListPatchSpy(messenger); + + // Every mock in `arrangeMocks()` is a one-shot nock interceptor; + // re-register each one with the *exact same* response body objects + // `arrangeMocks()` already used for the first fetch (rather than + // calling `createMockFeatureAnnouncementAPIResult()` again, which + // embeds a fresh `Date.now()`-based timestamp each time and would + // make this a non-repeat, non-deterministic fetch instead). + mockGetOnChainNotificationsConfig(); + mockFetchFeatureAnnouncementNotifications({ + status: 200, + body: mockFeatureAnnouncementAPIResult, + }); + mockGetAPINotifications({ + status: 200, + body: mockOnChainNotificationsAPIResult, + }); + + await controller.fetchAndUpdateMetamaskNotifications(); + + expect(sawNotificationsListPatch()).toBe(false); + }); + + it('touches metamaskNotificationsList when a repeat fetch returns a genuinely new notification', async () => { + const { messenger } = arrangeMocks(); + const controller = arrangeController(messenger); + + await controller.fetchAndUpdateMetamaskNotifications(); + + const sawNotificationsListPatch = + arrangeNotificationsListPatchSpy(messenger); + + // A distinct on-chain notification (different id) arrives on the next + // fetch, so the combined list can't be identical to what's in state. + // The feature-announcement mock is intentionally re-derived here + // (fresh timestamp and all) since this test only needs *a* difference + // to exist, not a controlled absence of one. + mockGetOnChainNotificationsConfig(); + mockFetchFeatureAnnouncementNotifications({ + status: 200, + body: createMockFeatureAnnouncementAPIResult(), + }); + mockGetAPINotifications({ + status: 200, + body: [{ ...createMockNotificationEthSent(), id: 'a-different-id' }], + }); + + await controller.fetchAndUpdateMetamaskNotifications(); + + expect(sawNotificationsListPatch()).toBe(true); + }); }); describe('getNotificationsByType', () => { @@ -1914,6 +2007,68 @@ describe('NotificationServicesController', () => { controller.state.metamaskNotificationsList[0].readDate, ).not.toBeNull(); }); + + it('does not publish stateChange when marking an already-read notification as read again', async () => { + const { messenger } = arrangeMocks(); + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + const notification = processNotification( + createMockFeatureAnnouncementRaw(), + ); + await controller.markMetamaskNotificationsAsRead([notification]); + expect(controller.state.metamaskNotificationsReadList).toHaveLength(1); + + const stateChangeListener = jest.fn(); + messenger.subscribe( + 'NotificationServicesController:stateChange', + stateChangeListener, + ); + + // Re-processing the same, already-read notification (e.g. the caller + // re-marks a list that includes items already marked as read). + await controller.markMetamaskNotificationsAsRead([ + { ...notification, isRead: true }, + ]); + + expect(stateChangeListener).not.toHaveBeenCalled(); + }); + + it('publishes stateChange when a genuinely new notification is marked as read', async () => { + const { messenger } = arrangeMocks(); + const controller = new NotificationServicesController({ + messenger, + env: { featureAnnouncements: featureAnnouncementsEnv }, + }); + + // Feature announcements are marked as read locally and never call the + // on-chain mark-as-read API, so a fresh one-shot mock per call is not + // needed for this test (unlike an ETH_SENT/on-chain notification, + // whose second call would otherwise hit the already-consumed mock). + const firstNotification = processNotification( + createMockFeatureAnnouncementRaw(), + ); + await controller.markMetamaskNotificationsAsRead([firstNotification]); + expect(controller.state.metamaskNotificationsReadList).toHaveLength(1); + + const stateChangeListener = jest.fn(); + messenger.subscribe( + 'NotificationServicesController:stateChange', + stateChangeListener, + ); + + const secondRawAnnouncement = createMockFeatureAnnouncementRaw(); + const secondNotification = processNotification({ + ...secondRawAnnouncement, + data: { ...secondRawAnnouncement.data, id: 'a-different-id' }, + }); + await controller.markMetamaskNotificationsAsRead([secondNotification]); + + expect(stateChangeListener).toHaveBeenCalled(); + expect(controller.state.metamaskNotificationsReadList).toHaveLength(2); + }); }); describe('enableMetamaskNotifications', () => { @@ -2213,6 +2368,100 @@ describe('NotificationServicesController', () => { expect(mockEnablePushNotifications).not.toHaveBeenCalled(); }); + it('does not publish stateChange when the account set is unchanged on a repeat call', async () => { + const { messenger, mockGetConfig } = 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 }, + }); + controller.init(); + + await controller.enablePushNotifications(); + expect(controller.state.subscriptionAccountsSeen).toStrictEqual([ + ADDRESS_1, + ADDRESS_2, + ]); + + const stateChangeListener = jest.fn(); + messenger.subscribe( + 'NotificationServicesController:stateChange', + stateChangeListener, + ); + mockGetConfig.mockResolvedValueOnce(mockPreferences()); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [ + { address: ADDRESS_1.toLowerCase(), enabled: true }, + { address: ADDRESS_2.toLowerCase(), enabled: false }, + ], + }); + + // Same two keyring accounts as before, in the same order. + await controller.enablePushNotifications(); + + expect(stateChangeListener).not.toHaveBeenCalled(); + }); + + it('publishes stateChange when the account set genuinely changes on a repeat call', async () => { + const { messenger, mockGetConfig, mockKeyringControllerGetState } = + 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 }, + }); + controller.init(); + + await controller.enablePushNotifications(); + + const stateChangeListener = jest.fn(); + messenger.subscribe( + 'NotificationServicesController:stateChange', + stateChangeListener, + ); + mockGetConfig.mockResolvedValueOnce(mockPreferences()); + mockGetOnChainNotificationsConfig({ + status: 200, + body: [{ address: ADDRESS_1.toLowerCase(), enabled: true }], + }); + // A new account has been added to the keyring since the first call. + mockKeyringControllerGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + accounts: [ADDRESS_1, ADDRESS_2, ADDRESS_3], + type: KeyringTypes.hd, + metadata: { id: 'srp-1', name: 'SRP 1' }, + }, + ], + }); + + await controller.enablePushNotifications(); + + expect(stateChangeListener).toHaveBeenCalled(); + // `ADDRESS_3`'s fixture string is not itself in EIP-55 checksum case + // (unlike `ADDRESS_1`/`ADDRESS_2`), so compare against its checksummed + // form rather than the raw fixture constant. + expect(controller.state.subscriptionAccountsSeen).toStrictEqual([ + ADDRESS_1, + ADDRESS_2, + ControllerUtils.toChecksumHexAddress(ADDRESS_3), + ]); + }); + it('unregisters the device when no account has notifications enabled', async () => { const { messenger, diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts index b3aa98d4883..27e929d8eb5 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts @@ -29,7 +29,7 @@ import type { import type { Messenger } from '@metamask/messenger'; import type { AuthenticationController } from '@metamask/profile-sync-controller'; import { assert } from '@metamask/utils'; -import { debounce } from 'lodash-es'; +import { debounce, isEqual } from 'lodash-es'; import log from 'loglevel'; import type { @@ -634,10 +634,12 @@ export class NotificationServicesController extends BaseController< (account) => !currentAccountsSet.has(account), ); - // Update accounts seen - this.update((state) => { - state.subscriptionAccountsSeen = [...currentAccountsSet]; - }); + // Only persist if the account set actually changed. + if (accountsAdded.length > 0 || accountsRemoved.length > 0) { + this.update((state) => { + state.subscriptionAccountsSeen = [...currentAccountsSet]; + }); + } return { accountsAdded, @@ -1344,9 +1346,13 @@ export class NotificationServicesController extends BaseController< ); // Update State - this.update((state) => { - state.metamaskNotificationsList = metamaskNotifications; - }); + if ( + !isEqual(this.state.metamaskNotificationsList, metamaskNotifications) + ) { + this.update((state) => { + state.metamaskNotificationsList = metamaskNotifications; + }); + } this.messenger.publish( `${controllerName}:notificationsListUpdated`, @@ -1521,11 +1527,14 @@ export class NotificationServicesController extends BaseController< ...featureAnnouncementNotificationIds, ...snapNotificationIds, ]; - state.metamaskNotificationsReadList = [ - ...new Set([...currentReadList, ...newReadIds]), - ]; + const currentReadSet = new Set(currentReadList); + if (newReadIds.some((id) => !currentReadSet.has(id))) { + state.metamaskNotificationsReadList = [ + ...new Set([...currentReadList, ...newReadIds]), + ]; + } - state.metamaskNotificationsList = state.metamaskNotificationsList.map( + const nextNotificationsList = state.metamaskNotificationsList.map( (notification: INotification) => { if ( newReadIds.includes(notification.id) || @@ -1543,6 +1552,9 @@ export class NotificationServicesController extends BaseController< return notification; }, ); + if (!isEqual(state.metamaskNotificationsList, nextNotificationsList)) { + state.metamaskNotificationsList = nextNotificationsList; + } }); this.messenger.publish(