From 8febbf23c39d77909b9950b29faaa5e2fe5e2a6d Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Wed, 16 Sep 2026 19:33:43 +0200 Subject: [PATCH] fix(notification-services-controller): skip no-op state writes listAccounts, fetchAndUpdateMetamaskNotifications, and markMetamaskNotificationsAsRead all rebuild an array (spread/map) and reassign it unconditionally, even when the result is identical to what's already in state. Since state.update() uses Immer, assigning a new-but-equal reference still emits a patch and publishes stateChange, which triggers a full state persist. listAccounts in particular gets called by several unrelated methods just to read the current accounts array, so on a wallet with many accounts this fired a lot more often than the account set itself actually changed. Add a cheap check before each of the three writes so they're skipped when nothing changed, and add tests asserting no stateChange is published in that case (and that it still is when something genuinely changes). --- .../CHANGELOG.md | 4 + .../NotificationServicesController.test.ts | 249 ++++++++++++++++++ .../NotificationServicesController.ts | 36 ++- 3 files changed, 277 insertions(+), 12 deletions(-) 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(