From d2c82677171ff27aad5cfc14455fda8aa7386807 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Fri, 13 Sep 2024 16:02:15 +0100 Subject: [PATCH 1/5] feat: add main network sync controller integration I need to add some integration tests to be a bit more confident in outcome --- .../UserStorageController.test.ts | 14 +- .../user-storage/UserStorageController.ts | 110 ++++++++++------ .../controller-integration.test.ts | 16 +-- .../network-syncing/controller-integration.ts | 121 ++++++++++++----- .../network-syncing/sync-all.test.ts | 123 ++++++------------ .../user-storage/network-syncing/sync-all.ts | 39 +----- 6 files changed, 211 insertions(+), 212 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 164ab168b4..135c3555f5 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -1417,17 +1417,9 @@ function mockUserStorageMessenger(options?: { return mockAccountsUpdateAccountMetadata(args.slice(1)); } - if (actionType === 'AccountsController:getAccountByAddress') { - return mockAccountsGetAccountByAddress(); - } - - 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 { 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 5af28ab924..0039566652 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -18,7 +18,11 @@ import type { KeyringControllerUnlockEvent, KeyringControllerAddNewAccountAction, } from '@metamask/keyring-controller'; -import type { NetworkConfiguration } from '@metamask/network-controller'; +import type { + NetworkConfiguration, + NetworkController, + NetworkControllerGetStateAction, +} from '@metamask/network-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; import { createSnapSignMessageRequest } from '../authentication/auth-snap-requests'; @@ -35,7 +39,10 @@ import { mapInternalAccountToUserStorageAccount, } from './accounts/user-storage'; import { createSHA256Hash } from './encryption'; -import { startNetworkSyncing } from './network-syncing/controller-integration'; +import { + performMainNetworkSync, + startNetworkSyncing, +} from './network-syncing/controller-integration'; import type { UserStoragePathWithFeatureAndKey, UserStoragePathWithFeatureOnly, @@ -46,26 +53,32 @@ import { upsertUserStorage, } from './services'; -// TODO: add external NetworkController event -// Need to listen for when a network gets added +// TODO - replace shimmed interface with actual interfaces once merged +// Waiting on #4698 type NetworkControllerNetworkAddedEvent = { type: 'NetworkController:networkAdded'; payload: [networkConfiguration: NetworkConfiguration]; }; - -// TODO: add external NetworkController event -// Need to listen for when a network is updated, or the default rpc/block explorer changes -type NetworkControllerNetworkChangedEvent = { - type: 'NetworkController:networkChanged'; +type NetworkControllerNetworkUpdatedEvent = { + type: 'NetworkController:networkUpdated'; payload: [networkConfiguration: NetworkConfiguration]; }; - -// TODO: add external NetworkController event -// Need to listen for when a network gets deleted -type NetworkControllerNetworkDeletedEvent = { - type: 'NetworkController:networkDeleted'; +type NetworkControllerNetworkRemovedEvent = { + type: 'NetworkController:networkRemoved'; payload: [networkConfiguration: NetworkConfiguration]; }; +type NetworkControllerAddNetworkAction = { + type: 'NetworkController:addNetwork'; + handler: NetworkController['addNetwork']; +}; +type NetworkControllerUpdateNetworkAction = { + type: 'NetworkController:updateNetwork'; + handler: NetworkController['updateNetwork']; +}; +type NetworkControllerRemoveNetworkAction = { + type: 'NetworkController:removeNetwork'; + handler: NetworkController['removeNetwork']; +}; // TODO: fix external dependencies export declare type NotificationServicesControllerDisableNotificationServices = @@ -173,10 +186,15 @@ export type AllowedActions = // Metamask Notifications | NotificationServicesControllerDisableNotificationServices | NotificationServicesControllerSelectIsNotificationServicesEnabled - // Account syncing + // Account Syncing | AccountsControllerListAccountsAction | AccountsControllerUpdateAccountMetadataAction - | KeyringControllerAddNewAccountAction; + | KeyringControllerAddNewAccountAction + // Network Syncing + | NetworkControllerGetStateAction + | NetworkControllerAddNetworkAction + | NetworkControllerUpdateNetworkAction + | NetworkControllerRemoveNetworkAction; // Messenger events export type UserStorageControllerStateChangeEvent = ControllerStateChangeEvent< @@ -207,8 +225,8 @@ export type AllowedEvents = | AccountsControllerAccountRenamedEvent // Network Syncing Events | NetworkControllerNetworkAddedEvent - | NetworkControllerNetworkChangedEvent - | NetworkControllerNetworkDeletedEvent; + | NetworkControllerNetworkUpdatedEvent + | NetworkControllerNetworkRemovedEvent; // Messenger export type UserStorageControllerMessenger = RestrictedControllerMessenger< @@ -232,6 +250,13 @@ export default class UserStorageController extends BaseController< UserStorageControllerState, UserStorageControllerMessenger > { + // This is replaced with the actual value in the constructor + // We will remove this once the feature will be released + #env = { + isAccountSyncingEnabled: false, + isNetworkSyncingEnabled: false, + }; + #auth = { getBearerToken: async () => { return await this.messagingSystem.call( @@ -260,17 +285,12 @@ export default class UserStorageController extends BaseController< }; #accounts = { - // This is replaced with the actual value in the constructor - // We will remove this once the feature will be released - isAccountSyncingEnabled: false, isAccountSyncingInProgress: false, canSync: () => { try { this.#assertProfileSyncingEnabled(); - return ( - this.#accounts.isAccountSyncingEnabled && this.#auth.isAuthEnabled() - ); + return this.#env.isAccountSyncingEnabled && this.#auth.isAuthEnabled(); } catch { return false; } @@ -406,9 +426,8 @@ export default class UserStorageController extends BaseController< state: { ...defaultState, ...state }, }); - this.#accounts.isAccountSyncingEnabled = Boolean( - env?.isAccountSyncingEnabled, - ); + this.#env.isAccountSyncingEnabled = Boolean(env?.isAccountSyncingEnabled); + this.#env.isNetworkSyncingEnabled = Boolean(env?.isNetworkSyncingEnabled); this.getMetaMetricsState = getMetaMetricsState; this.#keyringController.setupLockedStateSubscriptions(); @@ -417,18 +436,10 @@ export default class UserStorageController extends BaseController< this.#accounts.setupAccountSyncingSubscriptions(); // Network Syncing - if (env?.isNetworkSyncingEnabled) { + if (this.#env.isNetworkSyncingEnabled) { startNetworkSyncing({ messenger, - getStorageConfig: async () => { - const { storageKey, bearerToken } = - await this.#getStorageKeyAndBearerToken(); - return { - storageKey, - bearerToken, - nativeScryptCrypto: this.#nativeScryptCrypto, - }; - }, + getStorageConfig: this.#getStorageOptions, }); } } @@ -479,6 +490,20 @@ export default class UserStorageController extends BaseController< ); } + async #getStorageOptions() { + if (!this.state.isProfileSyncingEnabled) { + return null; + } + + const { storageKey, bearerToken } = + await this.#getStorageKeyAndBearerToken(); + return { + storageKey, + bearerToken, + nativeScryptCrypto: this.#nativeScryptCrypto, + }; + } + public async enableProfileSyncing(): Promise { try { this.#setIsProfileSyncingUpdateLoading(true); @@ -868,4 +893,15 @@ export default class UserStorageController extends BaseController< ); } } + + async syncNetworks() { + if (!this.#env.isNetworkSyncingEnabled) { + return; + } + + await performMainNetworkSync({ + messenger: this.messagingSystem, + getStorageConfig: this.#getStorageOptions, + }); + } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts index 162d865dca..4990d9734a 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts @@ -36,23 +36,13 @@ type ExternalEvents = NotNamespacedBy< >; const getEvents = (): ExternalEvents[] => [ 'NetworkController:networkAdded', - 'NetworkController:networkChanged', - 'NetworkController:networkDeleted', + 'NetworkController:networkUpdated', + 'NetworkController:networkRemoved', ]; const testMatrix = [ { - event: 'NetworkController:networkAdded' as const, - arrangeSyncFnMock: () => - jest.spyOn(SyncModule, 'addNetwork').mockResolvedValue(), - }, - { - event: 'NetworkController:networkChanged' as const, - arrangeSyncFnMock: () => - jest.spyOn(SyncModule, 'updateNetwork').mockResolvedValue(), - }, - { - event: 'NetworkController:networkDeleted' as const, + event: 'NetworkController:networkRemoved' as const, arrangeSyncFnMock: () => jest.spyOn(SyncModule, 'deleteNetwork').mockResolvedValue(), }, diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 901046a295..1e8fde240d 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -2,28 +2,50 @@ import log from 'loglevel'; import type { UserStorageBaseOptions } from '../services'; import type { UserStorageControllerMessenger } from '../UserStorageController'; -import { addNetwork, deleteNetwork, updateNetwork } from './sync-mutations'; +import { getAllRemoteNetworks } from './services'; +import { findNetworksToUpdate } from './sync-all'; +import { batchUpdateNetworks, deleteNetwork } from './sync-mutations'; -type SetupNetworkSyncingProps = { +type NetworkSyncingProps = { messenger: UserStorageControllerMessenger; - getStorageConfig: () => Promise; + getStorageConfig: () => Promise; }; +/** + * Global in-mem cache to signify that the network syncing is in progress + * Ensures that listeners do not fire during main sync (prevent double requests) + */ +let isMainNetworkSyncInProgress = false; + /** * Initialize and setup events to listen to for network syncing + * We will be listening to: + * - Remove Event, to indicate that we need to remote network from remote + * + * We will not be listening to: + * - Add/Update events are not required, as we can sync these during the main sync + * * @param props - parameters used for initializing and enabling network syncing */ -export function startNetworkSyncing(props: SetupNetworkSyncingProps) { +export function startNetworkSyncing(props: NetworkSyncingProps) { const { messenger, getStorageConfig } = props; - try { messenger.subscribe( - 'NetworkController:networkAdded', + 'NetworkController:networkRemoved', // eslint-disable-next-line @typescript-eslint/no-misused-promises async (networkConfiguration) => { try { + // As main sync is in progress, it will already local and remote networks + // So no need to re-process again. + if (isMainNetworkSyncInProgress) { + return; + } + const opts = await getStorageConfig(); - await addNetwork(networkConfiguration, opts); + if (!opts) { + return; + } + await deleteNetwork(networkConfiguration, opts); } catch { // Silently fail sync } @@ -32,38 +54,75 @@ export function startNetworkSyncing(props: SetupNetworkSyncingProps) { } catch (e) { log.warn('NetworkSyncing, event subscription failed', e); } +} +/** + * Action to perform the main network sync. + * It will fetch local networks and remote networks, then determines which networks (local and remote) to add/update + * @param props - parameters used for this main sync + */ +export async function performMainNetworkSync(props: NetworkSyncingProps) { + const { messenger, getStorageConfig } = props; + isMainNetworkSyncInProgress = true; try { - messenger.subscribe( - 'NetworkController:networkDeleted', - // eslint-disable-next-line @typescript-eslint/no-misused-promises - async (networkConfiguration) => { + const opts = await getStorageConfig(); + if (!opts) { + return; + } + + const localNetworks = Object.values( + messenger.call('NetworkController:getState') + .networkConfigurationsByChainId ?? {}, + ); + + const remoteNetworks = await getAllRemoteNetworks(opts); + + const networksToUpdate = findNetworksToUpdate({ + localNetworks, + remoteNetworks, + }); + + // Update Remote + if (networksToUpdate?.remoteNetworksToUpdate) { + await batchUpdateNetworks(networksToUpdate?.remoteNetworksToUpdate, opts); + } + + // Add missing local networks + if (networksToUpdate?.missingLocalNetworks) { + networksToUpdate.missingLocalNetworks.forEach((n) => { try { - const opts = await getStorageConfig(); - await deleteNetwork(networkConfiguration, opts); + messenger.call('NetworkController:addNetwork', n); } catch { - // Silently fail sync + // Silently fail, we can try this again on next main sync } - }, - ); - } catch (e) { - log.warn('NetworkSyncing, event subscription failed', e); - } + }); + } - try { - messenger.subscribe( - 'NetworkController:networkChanged', - // eslint-disable-next-line @typescript-eslint/no-misused-promises - async (networkConfiguration) => { + // Update local networks + if (networksToUpdate?.localNetworksToUpdate) { + const promises = networksToUpdate.localNetworksToUpdate.map(async (n) => { try { - const opts = await getStorageConfig(); - await updateNetwork(networkConfiguration, opts); + await messenger.call('NetworkController:updateNetwork', n.chainId, n); } catch { - // Silently fail sync + // Silently fail, we can try this again on next main sync } - }, - ); - } catch (e) { - log.warn('NetworkSyncing, event subscription failed', e); + }); + await Promise.all(promises); + } + + // Remove local networks + if (networksToUpdate?.localNetworksToRemove) { + networksToUpdate.localNetworksToRemove.forEach((n) => { + try { + messenger.call('NetworkController:removeNetwork', n.chainId); + } catch { + // Silently fail, we can try this again on next main sync + } + }); + } + } catch { + // Silently fail sync + } finally { + isMainNetworkSyncInProgress = false; } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts index 9702f641ad..efd314fee7 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts @@ -4,11 +4,10 @@ import { } from './__fixtures__/mockNetwork'; import { checkWhichNetworkIsLatest, - findNetworksToUpdate, getDataStructures, getMissingNetworkLists, - getNewLocalNetworks, getUpdatedNetworkLists, + findNetworksToUpdate, } from './sync-all'; import type { NetworkConfiguration, RemoteNetworkConfiguration } from './types'; @@ -205,75 +204,22 @@ describe('getUpdatedNetworkLists()', () => { }); }); -/** - * This is not used externally, but meant to check logic is consistent - */ -describe('getNewLocalNetworks()', () => { - it('should append original list with missing networks', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const missingNetworks = arrangeLocalNetworks(['4']); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: missingNetworks, - localNetworksToRemove: [], - localNetworksToUpdate: [], - }); - - expect(result).toHaveLength(4); - expect(result.map((n) => n.chainId)).toStrictEqual([ - '0x1', - '0x2', - '0x3', - '0x4', - ]); - }); - - it('should update original list if there are networks that need updating', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const updatedNetwork = createMockNetworkConfiguration({ - chainId: '0x1', - name: 'Updated Name', - }); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: [], - localNetworksToRemove: [], - localNetworksToUpdate: [updatedNetwork], - }); - - expect(result).toHaveLength(3); - expect(result.find((n) => n.chainId === '0x1')?.name).toBe('Updated Name'); - }); - - it('should remote a network from the original list if there are networks that need to be removed', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const deletedNetwork = createMockNetworkConfiguration({ chainId: '0x1' }); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: [], - localNetworksToRemove: [deletedNetwork], - localNetworksToUpdate: [], - }); - - expect(result).toHaveLength(2); - expect(result.find((n) => n.chainId === '0x1')).toBeUndefined(); - }); -}); - describe('findNetworksToUpdate()', () => { it('should add missing networks to remote and local', () => { const localNetworks = arrangeLocalNetworks(['1']); const remoteNetworks = arrangeRemoteNetworks(['2']); const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - expect(result?.newLocalNetworks).toHaveLength(2); - expect(result?.newLocalNetworks.map((n) => n.chainId)).toStrictEqual([ - '0x1', - '0x2', - ]); + + // Only 1 network needs to be added to local + expect(result?.missingLocalNetworks).toHaveLength(1); + expect(result?.missingLocalNetworks?.[0]?.chainId).toBe('0x2'); + + // No networks are to be removed locally + expect(result?.localNetworksToRemove).toStrictEqual([]); + + // No networks are to be updated locally + expect(result?.localNetworksToUpdate).toStrictEqual([]); // Only 1 network needs to be updated expect(result?.remoteNetworksToUpdate).toHaveLength(1); @@ -302,38 +248,43 @@ describe('findNetworksToUpdate()', () => { }); const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - const newLocalIds = result?.newLocalNetworks?.map((n) => n.chainId) ?? []; + + // Assert - No local networks to add or remove + expect(result?.missingLocalNetworks).toStrictEqual([]); + expect(result?.localNetworksToRemove).toStrictEqual([]); + + // Assert - Local and Remote networks to update + const updateLocalIds = + result?.localNetworksToUpdate?.map((n) => n.chainId) ?? []; const updateRemoteIds = result?.remoteNetworksToUpdate?.map((n) => n.chainId) ?? []; - // Assert - Test Matrix combinations were all tested + + // Check Test Matrix combinations were all tested let testCount = 0; testMatrix.forEach(({ actual }, idx) => { const chainId = `0x${idx}` as const; if (actual === 'Do Nothing') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will not include this, as it is not a network that needs updating on remote + // No lists are updated if nothing changes // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), - ]).toStrictEqual([true, false]); + ]).toStrictEqual([false, false]); } else if (actual === 'Local Wins') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will include this, as we need to update remote + // Only remote is updated if local wins // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), - ]).toStrictEqual([true, true]); + ]).toStrictEqual([false, true]); } else if (actual === 'Remote Wins') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will not include this, as it is not a network that needs updating on remote + // Only local is updated if remote wins // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), ]).toStrictEqual([true, false]); } @@ -349,11 +300,17 @@ describe('findNetworksToUpdate()', () => { remoteNetworks[1].d = true; const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - // Combined Local List is updated - expect(result?.newLocalNetworks).toHaveLength(1); - expect( - result?.newLocalNetworks.find((n) => n.chainId === '0x2'), - ).toBeUndefined(); + + // Assert no remote networks need updating + expect(result?.remoteNetworksToUpdate).toStrictEqual([]); + + // Assert no local networks need to be updated or added + expect(result?.localNetworksToUpdate).toStrictEqual([]); + expect(result?.missingLocalNetworks).toStrictEqual([]); + + // Assert that a network needs to be removed locally (network 0x2) + expect(result?.localNetworksToRemove).toHaveLength(1); + expect(result?.localNetworksToRemove?.[0]?.chainId).toBe('0x2'); // Remote List does not have any networks that need updating expect(result?.remoteNetworksToUpdate).toHaveLength(0); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts index f3cd7da156..4f07912d03 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts @@ -173,35 +173,6 @@ export const getUpdatedNetworkLists = ( }; }; -export const getNewLocalNetworks = (props: { - originalList: NetworkConfiguration[]; - missingLocalNetworks: NetworkConfiguration[]; - localNetworksToUpdate: NetworkConfiguration[]; - localNetworksToRemove: NetworkConfiguration[]; -}) => { - let newList = [...props.originalList]; - newList.push(...props.missingLocalNetworks); - const updateMap = createMap(props.localNetworksToUpdate); - const remoteMap = createMap(props.localNetworksToRemove); - - newList = newList - .map((n) => { - if (remoteMap.has(n.chainId)) { - return undefined; - } - - const updatedNetwork = updateMap.get(n.chainId); - if (updatedNetwork) { - return updatedNetwork; - } - - return n; - }) - .filter((n): n is NetworkConfiguration => n !== undefined); - - return newList; -}; - export const findNetworksToUpdate = (props: FindNetworksToUpdateProps) => { try { const { localNetworks, remoteNetworks } = props; @@ -221,17 +192,11 @@ export const findNetworksToUpdate = (props: FindNetworksToUpdateProps) => { ...updatedNetworks.remoteNetworksToUpdate, ]; - // List of new local networks - const newLocalNetworks = getNewLocalNetworks({ - originalList: localNetworks, + return { + remoteNetworksToUpdate, missingLocalNetworks: missingNetworks.missingLocalNetworks, localNetworksToRemove: updatedNetworks.localNetworksToRemove, localNetworksToUpdate: updatedNetworks.localNetworksToUpdate, - }); - - return { - remoteNetworksToUpdate, - newLocalNetworks, }; } catch { // Unable to perform sync, silently fail From c7e9673a8a8c7431b58aec32f347d6261edc1fa7 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Fri, 13 Sep 2024 16:02:15 +0100 Subject: [PATCH 2/5] feat: add main network sync controller integration I need to add some integration tests to be a bit more confident in outcome --- .../UserStorageController.test.ts | 14 +- .../user-storage/UserStorageController.ts | 110 ++++++++++------ .../controller-integration.test.ts | 16 +-- .../network-syncing/controller-integration.ts | 121 ++++++++++++----- .../network-syncing/sync-all.test.ts | 123 ++++++------------ .../user-storage/network-syncing/sync-all.ts | 39 +----- 6 files changed, 211 insertions(+), 212 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 164ab168b4..135c3555f5 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -1417,17 +1417,9 @@ function mockUserStorageMessenger(options?: { return mockAccountsUpdateAccountMetadata(args.slice(1)); } - if (actionType === 'AccountsController:getAccountByAddress') { - return mockAccountsGetAccountByAddress(); - } - - 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 { 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 5af28ab924..0039566652 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -18,7 +18,11 @@ import type { KeyringControllerUnlockEvent, KeyringControllerAddNewAccountAction, } from '@metamask/keyring-controller'; -import type { NetworkConfiguration } from '@metamask/network-controller'; +import type { + NetworkConfiguration, + NetworkController, + NetworkControllerGetStateAction, +} from '@metamask/network-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; import { createSnapSignMessageRequest } from '../authentication/auth-snap-requests'; @@ -35,7 +39,10 @@ import { mapInternalAccountToUserStorageAccount, } from './accounts/user-storage'; import { createSHA256Hash } from './encryption'; -import { startNetworkSyncing } from './network-syncing/controller-integration'; +import { + performMainNetworkSync, + startNetworkSyncing, +} from './network-syncing/controller-integration'; import type { UserStoragePathWithFeatureAndKey, UserStoragePathWithFeatureOnly, @@ -46,26 +53,32 @@ import { upsertUserStorage, } from './services'; -// TODO: add external NetworkController event -// Need to listen for when a network gets added +// TODO - replace shimmed interface with actual interfaces once merged +// Waiting on #4698 type NetworkControllerNetworkAddedEvent = { type: 'NetworkController:networkAdded'; payload: [networkConfiguration: NetworkConfiguration]; }; - -// TODO: add external NetworkController event -// Need to listen for when a network is updated, or the default rpc/block explorer changes -type NetworkControllerNetworkChangedEvent = { - type: 'NetworkController:networkChanged'; +type NetworkControllerNetworkUpdatedEvent = { + type: 'NetworkController:networkUpdated'; payload: [networkConfiguration: NetworkConfiguration]; }; - -// TODO: add external NetworkController event -// Need to listen for when a network gets deleted -type NetworkControllerNetworkDeletedEvent = { - type: 'NetworkController:networkDeleted'; +type NetworkControllerNetworkRemovedEvent = { + type: 'NetworkController:networkRemoved'; payload: [networkConfiguration: NetworkConfiguration]; }; +type NetworkControllerAddNetworkAction = { + type: 'NetworkController:addNetwork'; + handler: NetworkController['addNetwork']; +}; +type NetworkControllerUpdateNetworkAction = { + type: 'NetworkController:updateNetwork'; + handler: NetworkController['updateNetwork']; +}; +type NetworkControllerRemoveNetworkAction = { + type: 'NetworkController:removeNetwork'; + handler: NetworkController['removeNetwork']; +}; // TODO: fix external dependencies export declare type NotificationServicesControllerDisableNotificationServices = @@ -173,10 +186,15 @@ export type AllowedActions = // Metamask Notifications | NotificationServicesControllerDisableNotificationServices | NotificationServicesControllerSelectIsNotificationServicesEnabled - // Account syncing + // Account Syncing | AccountsControllerListAccountsAction | AccountsControllerUpdateAccountMetadataAction - | KeyringControllerAddNewAccountAction; + | KeyringControllerAddNewAccountAction + // Network Syncing + | NetworkControllerGetStateAction + | NetworkControllerAddNetworkAction + | NetworkControllerUpdateNetworkAction + | NetworkControllerRemoveNetworkAction; // Messenger events export type UserStorageControllerStateChangeEvent = ControllerStateChangeEvent< @@ -207,8 +225,8 @@ export type AllowedEvents = | AccountsControllerAccountRenamedEvent // Network Syncing Events | NetworkControllerNetworkAddedEvent - | NetworkControllerNetworkChangedEvent - | NetworkControllerNetworkDeletedEvent; + | NetworkControllerNetworkUpdatedEvent + | NetworkControllerNetworkRemovedEvent; // Messenger export type UserStorageControllerMessenger = RestrictedControllerMessenger< @@ -232,6 +250,13 @@ export default class UserStorageController extends BaseController< UserStorageControllerState, UserStorageControllerMessenger > { + // This is replaced with the actual value in the constructor + // We will remove this once the feature will be released + #env = { + isAccountSyncingEnabled: false, + isNetworkSyncingEnabled: false, + }; + #auth = { getBearerToken: async () => { return await this.messagingSystem.call( @@ -260,17 +285,12 @@ export default class UserStorageController extends BaseController< }; #accounts = { - // This is replaced with the actual value in the constructor - // We will remove this once the feature will be released - isAccountSyncingEnabled: false, isAccountSyncingInProgress: false, canSync: () => { try { this.#assertProfileSyncingEnabled(); - return ( - this.#accounts.isAccountSyncingEnabled && this.#auth.isAuthEnabled() - ); + return this.#env.isAccountSyncingEnabled && this.#auth.isAuthEnabled(); } catch { return false; } @@ -406,9 +426,8 @@ export default class UserStorageController extends BaseController< state: { ...defaultState, ...state }, }); - this.#accounts.isAccountSyncingEnabled = Boolean( - env?.isAccountSyncingEnabled, - ); + this.#env.isAccountSyncingEnabled = Boolean(env?.isAccountSyncingEnabled); + this.#env.isNetworkSyncingEnabled = Boolean(env?.isNetworkSyncingEnabled); this.getMetaMetricsState = getMetaMetricsState; this.#keyringController.setupLockedStateSubscriptions(); @@ -417,18 +436,10 @@ export default class UserStorageController extends BaseController< this.#accounts.setupAccountSyncingSubscriptions(); // Network Syncing - if (env?.isNetworkSyncingEnabled) { + if (this.#env.isNetworkSyncingEnabled) { startNetworkSyncing({ messenger, - getStorageConfig: async () => { - const { storageKey, bearerToken } = - await this.#getStorageKeyAndBearerToken(); - return { - storageKey, - bearerToken, - nativeScryptCrypto: this.#nativeScryptCrypto, - }; - }, + getStorageConfig: this.#getStorageOptions, }); } } @@ -479,6 +490,20 @@ export default class UserStorageController extends BaseController< ); } + async #getStorageOptions() { + if (!this.state.isProfileSyncingEnabled) { + return null; + } + + const { storageKey, bearerToken } = + await this.#getStorageKeyAndBearerToken(); + return { + storageKey, + bearerToken, + nativeScryptCrypto: this.#nativeScryptCrypto, + }; + } + public async enableProfileSyncing(): Promise { try { this.#setIsProfileSyncingUpdateLoading(true); @@ -868,4 +893,15 @@ export default class UserStorageController extends BaseController< ); } } + + async syncNetworks() { + if (!this.#env.isNetworkSyncingEnabled) { + return; + } + + await performMainNetworkSync({ + messenger: this.messagingSystem, + getStorageConfig: this.#getStorageOptions, + }); + } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts index 162d865dca..4990d9734a 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts @@ -36,23 +36,13 @@ type ExternalEvents = NotNamespacedBy< >; const getEvents = (): ExternalEvents[] => [ 'NetworkController:networkAdded', - 'NetworkController:networkChanged', - 'NetworkController:networkDeleted', + 'NetworkController:networkUpdated', + 'NetworkController:networkRemoved', ]; const testMatrix = [ { - event: 'NetworkController:networkAdded' as const, - arrangeSyncFnMock: () => - jest.spyOn(SyncModule, 'addNetwork').mockResolvedValue(), - }, - { - event: 'NetworkController:networkChanged' as const, - arrangeSyncFnMock: () => - jest.spyOn(SyncModule, 'updateNetwork').mockResolvedValue(), - }, - { - event: 'NetworkController:networkDeleted' as const, + event: 'NetworkController:networkRemoved' as const, arrangeSyncFnMock: () => jest.spyOn(SyncModule, 'deleteNetwork').mockResolvedValue(), }, diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 901046a295..1e8fde240d 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -2,28 +2,50 @@ import log from 'loglevel'; import type { UserStorageBaseOptions } from '../services'; import type { UserStorageControllerMessenger } from '../UserStorageController'; -import { addNetwork, deleteNetwork, updateNetwork } from './sync-mutations'; +import { getAllRemoteNetworks } from './services'; +import { findNetworksToUpdate } from './sync-all'; +import { batchUpdateNetworks, deleteNetwork } from './sync-mutations'; -type SetupNetworkSyncingProps = { +type NetworkSyncingProps = { messenger: UserStorageControllerMessenger; - getStorageConfig: () => Promise; + getStorageConfig: () => Promise; }; +/** + * Global in-mem cache to signify that the network syncing is in progress + * Ensures that listeners do not fire during main sync (prevent double requests) + */ +let isMainNetworkSyncInProgress = false; + /** * Initialize and setup events to listen to for network syncing + * We will be listening to: + * - Remove Event, to indicate that we need to remote network from remote + * + * We will not be listening to: + * - Add/Update events are not required, as we can sync these during the main sync + * * @param props - parameters used for initializing and enabling network syncing */ -export function startNetworkSyncing(props: SetupNetworkSyncingProps) { +export function startNetworkSyncing(props: NetworkSyncingProps) { const { messenger, getStorageConfig } = props; - try { messenger.subscribe( - 'NetworkController:networkAdded', + 'NetworkController:networkRemoved', // eslint-disable-next-line @typescript-eslint/no-misused-promises async (networkConfiguration) => { try { + // As main sync is in progress, it will already local and remote networks + // So no need to re-process again. + if (isMainNetworkSyncInProgress) { + return; + } + const opts = await getStorageConfig(); - await addNetwork(networkConfiguration, opts); + if (!opts) { + return; + } + await deleteNetwork(networkConfiguration, opts); } catch { // Silently fail sync } @@ -32,38 +54,75 @@ export function startNetworkSyncing(props: SetupNetworkSyncingProps) { } catch (e) { log.warn('NetworkSyncing, event subscription failed', e); } +} +/** + * Action to perform the main network sync. + * It will fetch local networks and remote networks, then determines which networks (local and remote) to add/update + * @param props - parameters used for this main sync + */ +export async function performMainNetworkSync(props: NetworkSyncingProps) { + const { messenger, getStorageConfig } = props; + isMainNetworkSyncInProgress = true; try { - messenger.subscribe( - 'NetworkController:networkDeleted', - // eslint-disable-next-line @typescript-eslint/no-misused-promises - async (networkConfiguration) => { + const opts = await getStorageConfig(); + if (!opts) { + return; + } + + const localNetworks = Object.values( + messenger.call('NetworkController:getState') + .networkConfigurationsByChainId ?? {}, + ); + + const remoteNetworks = await getAllRemoteNetworks(opts); + + const networksToUpdate = findNetworksToUpdate({ + localNetworks, + remoteNetworks, + }); + + // Update Remote + if (networksToUpdate?.remoteNetworksToUpdate) { + await batchUpdateNetworks(networksToUpdate?.remoteNetworksToUpdate, opts); + } + + // Add missing local networks + if (networksToUpdate?.missingLocalNetworks) { + networksToUpdate.missingLocalNetworks.forEach((n) => { try { - const opts = await getStorageConfig(); - await deleteNetwork(networkConfiguration, opts); + messenger.call('NetworkController:addNetwork', n); } catch { - // Silently fail sync + // Silently fail, we can try this again on next main sync } - }, - ); - } catch (e) { - log.warn('NetworkSyncing, event subscription failed', e); - } + }); + } - try { - messenger.subscribe( - 'NetworkController:networkChanged', - // eslint-disable-next-line @typescript-eslint/no-misused-promises - async (networkConfiguration) => { + // Update local networks + if (networksToUpdate?.localNetworksToUpdate) { + const promises = networksToUpdate.localNetworksToUpdate.map(async (n) => { try { - const opts = await getStorageConfig(); - await updateNetwork(networkConfiguration, opts); + await messenger.call('NetworkController:updateNetwork', n.chainId, n); } catch { - // Silently fail sync + // Silently fail, we can try this again on next main sync } - }, - ); - } catch (e) { - log.warn('NetworkSyncing, event subscription failed', e); + }); + await Promise.all(promises); + } + + // Remove local networks + if (networksToUpdate?.localNetworksToRemove) { + networksToUpdate.localNetworksToRemove.forEach((n) => { + try { + messenger.call('NetworkController:removeNetwork', n.chainId); + } catch { + // Silently fail, we can try this again on next main sync + } + }); + } + } catch { + // Silently fail sync + } finally { + isMainNetworkSyncInProgress = false; } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts index 9702f641ad..efd314fee7 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.test.ts @@ -4,11 +4,10 @@ import { } from './__fixtures__/mockNetwork'; import { checkWhichNetworkIsLatest, - findNetworksToUpdate, getDataStructures, getMissingNetworkLists, - getNewLocalNetworks, getUpdatedNetworkLists, + findNetworksToUpdate, } from './sync-all'; import type { NetworkConfiguration, RemoteNetworkConfiguration } from './types'; @@ -205,75 +204,22 @@ describe('getUpdatedNetworkLists()', () => { }); }); -/** - * This is not used externally, but meant to check logic is consistent - */ -describe('getNewLocalNetworks()', () => { - it('should append original list with missing networks', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const missingNetworks = arrangeLocalNetworks(['4']); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: missingNetworks, - localNetworksToRemove: [], - localNetworksToUpdate: [], - }); - - expect(result).toHaveLength(4); - expect(result.map((n) => n.chainId)).toStrictEqual([ - '0x1', - '0x2', - '0x3', - '0x4', - ]); - }); - - it('should update original list if there are networks that need updating', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const updatedNetwork = createMockNetworkConfiguration({ - chainId: '0x1', - name: 'Updated Name', - }); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: [], - localNetworksToRemove: [], - localNetworksToUpdate: [updatedNetwork], - }); - - expect(result).toHaveLength(3); - expect(result.find((n) => n.chainId === '0x1')?.name).toBe('Updated Name'); - }); - - it('should remote a network from the original list if there are networks that need to be removed', () => { - const originalList = arrangeLocalNetworks(['1', '2', '3']); - const deletedNetwork = createMockNetworkConfiguration({ chainId: '0x1' }); - - const result = getNewLocalNetworks({ - originalList, - missingLocalNetworks: [], - localNetworksToRemove: [deletedNetwork], - localNetworksToUpdate: [], - }); - - expect(result).toHaveLength(2); - expect(result.find((n) => n.chainId === '0x1')).toBeUndefined(); - }); -}); - describe('findNetworksToUpdate()', () => { it('should add missing networks to remote and local', () => { const localNetworks = arrangeLocalNetworks(['1']); const remoteNetworks = arrangeRemoteNetworks(['2']); const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - expect(result?.newLocalNetworks).toHaveLength(2); - expect(result?.newLocalNetworks.map((n) => n.chainId)).toStrictEqual([ - '0x1', - '0x2', - ]); + + // Only 1 network needs to be added to local + expect(result?.missingLocalNetworks).toHaveLength(1); + expect(result?.missingLocalNetworks?.[0]?.chainId).toBe('0x2'); + + // No networks are to be removed locally + expect(result?.localNetworksToRemove).toStrictEqual([]); + + // No networks are to be updated locally + expect(result?.localNetworksToUpdate).toStrictEqual([]); // Only 1 network needs to be updated expect(result?.remoteNetworksToUpdate).toHaveLength(1); @@ -302,38 +248,43 @@ describe('findNetworksToUpdate()', () => { }); const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - const newLocalIds = result?.newLocalNetworks?.map((n) => n.chainId) ?? []; + + // Assert - No local networks to add or remove + expect(result?.missingLocalNetworks).toStrictEqual([]); + expect(result?.localNetworksToRemove).toStrictEqual([]); + + // Assert - Local and Remote networks to update + const updateLocalIds = + result?.localNetworksToUpdate?.map((n) => n.chainId) ?? []; const updateRemoteIds = result?.remoteNetworksToUpdate?.map((n) => n.chainId) ?? []; - // Assert - Test Matrix combinations were all tested + + // Check Test Matrix combinations were all tested let testCount = 0; testMatrix.forEach(({ actual }, idx) => { const chainId = `0x${idx}` as const; if (actual === 'Do Nothing') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will not include this, as it is not a network that needs updating on remote + // No lists are updated if nothing changes // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), - ]).toStrictEqual([true, false]); + ]).toStrictEqual([false, false]); } else if (actual === 'Local Wins') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will include this, as we need to update remote + // Only remote is updated if local wins // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), - ]).toStrictEqual([true, true]); + ]).toStrictEqual([false, true]); } else if (actual === 'Remote Wins') { testCount += 1; - // Combined Local Networks will include this - // Updated Remote Networks will not include this, as it is not a network that needs updating on remote + // Only local is updated if remote wins // eslint-disable-next-line jest/no-conditional-expect expect([ - newLocalIds.includes(chainId), + updateLocalIds.includes(chainId), updateRemoteIds.includes(chainId), ]).toStrictEqual([true, false]); } @@ -349,11 +300,17 @@ describe('findNetworksToUpdate()', () => { remoteNetworks[1].d = true; const result = findNetworksToUpdate({ localNetworks, remoteNetworks }); - // Combined Local List is updated - expect(result?.newLocalNetworks).toHaveLength(1); - expect( - result?.newLocalNetworks.find((n) => n.chainId === '0x2'), - ).toBeUndefined(); + + // Assert no remote networks need updating + expect(result?.remoteNetworksToUpdate).toStrictEqual([]); + + // Assert no local networks need to be updated or added + expect(result?.localNetworksToUpdate).toStrictEqual([]); + expect(result?.missingLocalNetworks).toStrictEqual([]); + + // Assert that a network needs to be removed locally (network 0x2) + expect(result?.localNetworksToRemove).toHaveLength(1); + expect(result?.localNetworksToRemove?.[0]?.chainId).toBe('0x2'); // Remote List does not have any networks that need updating expect(result?.remoteNetworksToUpdate).toHaveLength(0); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts index f3cd7da156..4f07912d03 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts @@ -173,35 +173,6 @@ export const getUpdatedNetworkLists = ( }; }; -export const getNewLocalNetworks = (props: { - originalList: NetworkConfiguration[]; - missingLocalNetworks: NetworkConfiguration[]; - localNetworksToUpdate: NetworkConfiguration[]; - localNetworksToRemove: NetworkConfiguration[]; -}) => { - let newList = [...props.originalList]; - newList.push(...props.missingLocalNetworks); - const updateMap = createMap(props.localNetworksToUpdate); - const remoteMap = createMap(props.localNetworksToRemove); - - newList = newList - .map((n) => { - if (remoteMap.has(n.chainId)) { - return undefined; - } - - const updatedNetwork = updateMap.get(n.chainId); - if (updatedNetwork) { - return updatedNetwork; - } - - return n; - }) - .filter((n): n is NetworkConfiguration => n !== undefined); - - return newList; -}; - export const findNetworksToUpdate = (props: FindNetworksToUpdateProps) => { try { const { localNetworks, remoteNetworks } = props; @@ -221,17 +192,11 @@ export const findNetworksToUpdate = (props: FindNetworksToUpdateProps) => { ...updatedNetworks.remoteNetworksToUpdate, ]; - // List of new local networks - const newLocalNetworks = getNewLocalNetworks({ - originalList: localNetworks, + return { + remoteNetworksToUpdate, missingLocalNetworks: missingNetworks.missingLocalNetworks, localNetworksToRemove: updatedNetworks.localNetworksToRemove, localNetworksToUpdate: updatedNetworks.localNetworksToUpdate, - }); - - return { - remoteNetworksToUpdate, - newLocalNetworks, }; } catch { // Unable to perform sync, silently fail From b18119c800f119177a4e7b2705ffa772911f9f20 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 24 Oct 2024 15:36:12 +0100 Subject: [PATCH 3/5] feat: add network sync callbacks mostly to be used for analytics --- .../user-storage/UserStorageController.ts | 32 +++++++++++++++++++ .../network-syncing/controller-integration.ts | 19 +++++++++-- 2 files changed, 48 insertions(+), 3 deletions(-) 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 6d61678769..37ee9327fa 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -143,6 +143,30 @@ type ControllerConfig = { */ onAccountNameUpdated?: (profileId: string) => void; }; + + networkSyncing?: { + /** + * Callback that fires when network sync adds a network + * This is used for analytics. + * @param profileId - ID for a given User (shared cross devices once authenticated) + * @param chainId - Chain ID for the network added (in hex) + */ + onNetworkAdded?: (profileId: string, chainId: string) => void; + /** + * Callback that fires when network sync updates a network + * This is used for analytics. + * @param profileId - ID for a given User (shared cross devices once authenticated) + * @param chainId - Chain ID for the network added (in hex) + */ + onNetworkUpdated?: (profileId: string, chainId: string) => void; + /** + * Callback that fires when network sync deletes a network + * This is used for analytics. + * @param profileId - ID for a given User (shared cross devices once authenticated) + * @param chainId - Chain ID for the network added (in hex) + */ + onNetworkRemoved?: (profileId: string, chainId: string) => void; + }; }; // Messenger Actions @@ -1015,9 +1039,17 @@ export default class UserStorageController extends BaseController< return; } + const profileId = await this.#auth.getProfileId(); + await performMainNetworkSync({ messenger: this.messagingSystem, getStorageConfig: this.#getStorageOptions, + onNetworkAdded: (cId) => + this.#config?.networkSyncing?.onNetworkAdded?.(profileId, cId), + onNetworkUpdated: (cId) => + this.#config?.networkSyncing?.onNetworkUpdated?.(profileId, cId), + onNetworkRemoved: (cId) => + this.#config?.networkSyncing?.onNetworkRemoved?.(profileId, cId), }); } } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 1e8fde240d..10351481bd 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -6,11 +6,19 @@ import { getAllRemoteNetworks } from './services'; import { findNetworksToUpdate } from './sync-all'; import { batchUpdateNetworks, deleteNetwork } from './sync-mutations'; -type NetworkSyncingProps = { +type StartNetworkSyncingProps = { messenger: UserStorageControllerMessenger; getStorageConfig: () => Promise; }; +type PerformMainNetworkSyncProps = { + messenger: UserStorageControllerMessenger; + getStorageConfig: () => Promise; + onNetworkAdded?: (chainId: string) => void; + onNetworkUpdated?: (chainId: string) => void; + onNetworkRemoved?: (chainId: string) => void; +}; + /** * Global in-mem cache to signify that the network syncing is in progress * Ensures that listeners do not fire during main sync (prevent double requests) @@ -27,7 +35,7 @@ let isMainNetworkSyncInProgress = false; * * @param props - parameters used for initializing and enabling network syncing */ -export function startNetworkSyncing(props: NetworkSyncingProps) { +export function startNetworkSyncing(props: StartNetworkSyncingProps) { const { messenger, getStorageConfig } = props; try { messenger.subscribe( @@ -61,7 +69,9 @@ export function startNetworkSyncing(props: NetworkSyncingProps) { * It will fetch local networks and remote networks, then determines which networks (local and remote) to add/update * @param props - parameters used for this main sync */ -export async function performMainNetworkSync(props: NetworkSyncingProps) { +export async function performMainNetworkSync( + props: PerformMainNetworkSyncProps, +) { const { messenger, getStorageConfig } = props; isMainNetworkSyncInProgress = true; try { @@ -92,6 +102,7 @@ export async function performMainNetworkSync(props: NetworkSyncingProps) { networksToUpdate.missingLocalNetworks.forEach((n) => { try { messenger.call('NetworkController:addNetwork', n); + props.onNetworkAdded?.(n.chainId); } catch { // Silently fail, we can try this again on next main sync } @@ -103,6 +114,7 @@ export async function performMainNetworkSync(props: NetworkSyncingProps) { const promises = networksToUpdate.localNetworksToUpdate.map(async (n) => { try { await messenger.call('NetworkController:updateNetwork', n.chainId, n); + props.onNetworkUpdated?.(n.chainId); } catch { // Silently fail, we can try this again on next main sync } @@ -115,6 +127,7 @@ export async function performMainNetworkSync(props: NetworkSyncingProps) { networksToUpdate.localNetworksToRemove.forEach((n) => { try { messenger.call('NetworkController:removeNetwork', n.chainId); + props.onNetworkRemoved?.(n.chainId); } catch { // Silently fail, we can try this again on next main sync } From 94a67bad2525687d1204a27931b907687821390c Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 24 Oct 2024 16:36:57 +0100 Subject: [PATCH 4/5] test: add controller-integration - performMainSync() tests --- .../UserStorageController.test.ts | 16 +- .../controller-integration.test.ts | 299 ++++++++++++++++-- .../network-syncing/controller-integration.ts | 33 +- 3 files changed, 306 insertions(+), 42 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 1a753acd99..accbceb6bb 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -39,6 +39,14 @@ import type { } from './UserStorageController'; import UserStorageController from './UserStorageController'; +// Creates the correct typed call params for mocks +type CallParams = { + [K in AllowedActions['type']]: [ + K, + ...Parameters['handler']>, + ]; +}[AllowedActions['type']]; + const typedMockFn = unknown>() => jest.fn, Parameters>(); @@ -1730,14 +1738,6 @@ function mockUserStorageMessenger(options?: { const mockAccountsUpdateAccountMetadata = jest.fn().mockResolvedValue(true); jest.spyOn(messenger, 'call').mockImplementation((...args) => { - // Creates the correct typed call params for mocks - type CallParams = { - [K in AllowedActions['type']]: [ - K, - ...Parameters['handler']>, - ]; - }[AllowedActions['type']]; - const [actionType, params] = args as unknown as CallParams; if (actionType === 'SnapController:handleRequest') { diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts index 4990d9734a..2eaf8079c0 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts @@ -2,13 +2,41 @@ import type { NotNamespacedBy } from '@metamask/base-controller'; import { ControllerMessenger } from '@metamask/base-controller'; import log from 'loglevel'; -import type { AllowedActions, AllowedEvents } from '..'; +import type { + AllowedActions, + AllowedEvents, + UserStorageControllerMessenger, +} from '..'; import { MOCK_STORAGE_KEY } from '../__fixtures__'; import { waitFor } from '../__fixtures__/test-utils'; import type { UserStorageBaseOptions } from '../services'; -import { createMockNetworkConfiguration } from './__fixtures__/mockNetwork'; -import { startNetworkSyncing } from './controller-integration'; -import * as SyncModule from './sync-mutations'; +import { + createMockNetworkConfiguration, + createMockRemoteNetworkConfiguration, +} from './__fixtures__/mockNetwork'; +import { + performMainNetworkSync, + startNetworkSyncing, +} from './controller-integration'; +import * as ServicesModule from './services'; +import * as SyncAllModule from './sync-all'; +import * as SyncMutationsModule from './sync-mutations'; + +type GetActionHandler = Extract< + AllowedActions, + { type: Type } +>['handler']; + +// Creates the correct typed call params for mocks +type CallParams = { + [K in AllowedActions['type']]: [K, ...Parameters>]; +}[AllowedActions['type']]; + +const typedMockCallFn = < + Type extends AllowedActions['type'], + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Func extends (...args: any[]) => any = GetActionHandler, +>() => jest.fn, Parameters>(); jest.mock('loglevel', () => { const actual = jest.requireActual('loglevel'); @@ -44,12 +72,12 @@ const testMatrix = [ { event: 'NetworkController:networkRemoved' as const, arrangeSyncFnMock: () => - jest.spyOn(SyncModule, 'deleteNetwork').mockResolvedValue(), + jest.spyOn(SyncMutationsModule, 'deleteNetwork').mockResolvedValue(), }, ]; describe.each(testMatrix)( - 'network-syncing/controller-integration - $event', + 'network-syncing/controller-integration - startNetworkSyncing() $event', ({ event, arrangeSyncFnMock }) => { it(`should successfully sync when ${event} is emitted`, async () => { const syncFnMock = arrangeSyncFnMock(); @@ -86,24 +114,240 @@ describe.each(testMatrix)( startNetworkSyncing({ messenger, getStorageConfig }); expect(warnMock).toHaveBeenCalled(); }); + + /** + * Test Utility - arrange mocks and parameters + * @returns the mocks and parameters used when testing `startNetworkSyncing()` + */ + function arrangeMocks() { + const baseMessenger = mockBaseMessenger(); + const messenger = mockUserStorageMessenger(baseMessenger); + const getStorageConfigMock = jest.fn().mockResolvedValue(storageOpts); + + return { + getStorageConfig: getStorageConfigMock, + baseMessenger, + messenger, + }; + } }, ); -/** - * Test Utility - arrange mocks and parameters - * @returns the mocks and parameters used when testing `startNetworkSyncing()` - */ -function arrangeMocks() { - const baseMessenger = mockBaseMessenger(); - const messenger = mockUserStorageMessenger(baseMessenger); - const getStorageConfigMock = jest.fn().mockResolvedValue(storageOpts); +describe('network-syncing/controller-integration - performMainSync()', () => { + it('should do nothing if unable to get storage config', async () => { + const { getStorageConfig, messenger, mockCalls } = arrangeMocks(); + getStorageConfig.mockResolvedValue(null); - return { - getStorageConfig: getStorageConfigMock, - baseMessenger, - messenger, - }; -} + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(getStorageConfig).toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerGetState).not.toHaveBeenCalled(); + }); + + it('should do nothing if unable to calculate networks to update', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue(undefined); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerAddNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerUpdateNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerRemoveNetwork).not.toHaveBeenCalled(); + }); + + it('should update remote networks if there are local networks to add', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue({ + remoteNetworksToUpdate: [createMockRemoteNetworkConfiguration()], + missingLocalNetworks: [], + localNetworksToUpdate: [], + localNetworksToRemove: [], + }); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerAddNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerUpdateNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerRemoveNetwork).not.toHaveBeenCalled(); + }); + + it('should add missing local networks', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue({ + remoteNetworksToUpdate: [], + missingLocalNetworks: [createMockNetworkConfiguration()], + localNetworksToUpdate: [], + localNetworksToRemove: [], + }); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerAddNetwork).toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerUpdateNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerRemoveNetwork).not.toHaveBeenCalled(); + }); + + it('should update local networks', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue({ + remoteNetworksToUpdate: [], + missingLocalNetworks: [], + localNetworksToUpdate: [createMockNetworkConfiguration()], + localNetworksToRemove: [], + }); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerAddNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerUpdateNetwork).toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerRemoveNetwork).not.toHaveBeenCalled(); + }); + + it('should remove local networks', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue({ + remoteNetworksToUpdate: [], + missingLocalNetworks: [], + localNetworksToUpdate: [], + localNetworksToRemove: [createMockNetworkConfiguration()], + }); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerAddNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerUpdateNetwork).not.toHaveBeenCalled(); + expect(mockCalls.mockNetworkControllerRemoveNetwork).toHaveBeenCalled(); + }); + + it('should handle multiple networks to update', async () => { + const { messenger, getStorageConfig, mockSync, mockServices, mockCalls } = + arrangeMocks(); + mockSync.findNetworksToUpdate.mockReturnValue({ + remoteNetworksToUpdate: [ + createMockRemoteNetworkConfiguration(), + createMockRemoteNetworkConfiguration(), + ], + missingLocalNetworks: [ + createMockNetworkConfiguration(), + createMockNetworkConfiguration(), + ], + localNetworksToUpdate: [ + createMockNetworkConfiguration(), + createMockNetworkConfiguration(), + ], + localNetworksToRemove: [ + createMockNetworkConfiguration(), + createMockNetworkConfiguration(), + ], + }); + + await performMainNetworkSync({ messenger, getStorageConfig }); + expect(mockServices.mockBatchUpdateNetworks).toHaveBeenCalledTimes(1); // this is a batch endpoint + expect(mockCalls.mockNetworkControllerAddNetwork).toHaveBeenCalledTimes(2); + expect(mockCalls.mockNetworkControllerUpdateNetwork).toHaveBeenCalledTimes( + 2, + ); + expect(mockCalls.mockNetworkControllerRemoveNetwork).toHaveBeenCalledTimes( + 2, + ); + }); + + /** + * Jest Mock Utility - create suite of mocks for tests + * @returns mocks for tests + */ + function arrangeMocks() { + const baseMessenger = mockBaseMessenger(); + const messenger = mockUserStorageMessenger(baseMessenger); + const getStorageConfigMock = jest + .fn, []>() + .mockResolvedValue(storageOpts); + + const mockCalls = mockMessengerCalls(messenger); + + return { + baseMessenger, + messenger, + getStorageConfig: getStorageConfigMock, + mockCalls, + mockServices: { + mockGetAllRemoveNetworks: jest + .spyOn(ServicesModule, 'getAllRemoteNetworks') + .mockResolvedValue([]), + mockBatchUpdateNetworks: jest + .spyOn(ServicesModule, 'batchUpsertRemoteNetworks') + .mockResolvedValue(), + }, + mockSync: { + findNetworksToUpdate: jest + .spyOn(SyncAllModule, 'findNetworksToUpdate') + .mockReturnValue(undefined), + }, + }; + } + + /** + * Jest Mock Utility - create a mock User Storage Messenger + * @param messenger - The messenger to mock + * @returns messenger call mocks + */ + function mockMessengerCalls(messenger: UserStorageControllerMessenger) { + const mockNetworkControllerGetState = + typedMockCallFn<'NetworkController:getState'>().mockReturnValue({ + selectedNetworkClientId: '', + networksMetadata: {}, + networkConfigurationsByChainId: {}, + }); + + const mockNetworkControllerAddNetwork = + typedMockCallFn<'NetworkController:addNetwork'>(); + + const mockNetworkControllerUpdateNetwork = + typedMockCallFn<'NetworkController:updateNetwork'>(); + + const mockNetworkControllerRemoveNetwork = + typedMockCallFn<'NetworkController:removeNetwork'>(); + + jest.spyOn(messenger, 'call').mockImplementation((...args) => { + const typedArgs = args as unknown as CallParams; + const [actionType] = typedArgs; + + if (actionType === 'NetworkController:getState') { + return mockNetworkControllerGetState(); + } + + if (actionType === 'NetworkController:addNetwork') { + const [, ...params] = typedArgs; + return mockNetworkControllerAddNetwork(...params); + } + + if (actionType === 'NetworkController:updateNetwork') { + const [, ...params] = typedArgs; + return mockNetworkControllerUpdateNetwork(...params); + } + + if (actionType === 'NetworkController:removeNetwork') { + const [, ...params] = typedArgs; + return mockNetworkControllerRemoveNetwork(...params); + } + + throw new Error( + `MOCK_FAIL - unsupported messenger call: ${actionType as string}`, + ); + }); + + return { + mockNetworkControllerGetState, + mockNetworkControllerAddNetwork, + mockNetworkControllerUpdateNetwork, + mockNetworkControllerRemoveNetwork, + }; + } +}); /** * Test Utility - creates a base messenger so we can invoke/publish events @@ -132,7 +376,20 @@ function mockUserStorageMessenger( const messenger = baseMessenger.getRestricted({ name: 'UserStorageController', - allowedActions: [], + allowedActions: [ + 'KeyringController:getState', + 'SnapController:handleRequest', + 'AuthenticationController:getBearerToken', + 'AuthenticationController:getSessionProfile', + 'AuthenticationController:isSignedIn', + 'AuthenticationController:performSignIn', + 'AuthenticationController:performSignOut', + 'NotificationServicesController:disableNotificationServices', + 'NotificationServicesController:selectIsNotificationServicesEnabled', + 'AccountsController:listAccounts', + 'AccountsController:updateAccountMetadata', + 'KeyringController:addNewAccount', + ], allowedEvents, }); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 10351481bd..7c182f1310 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -93,12 +93,18 @@ export async function performMainNetworkSync( }); // Update Remote - if (networksToUpdate?.remoteNetworksToUpdate) { + if ( + networksToUpdate?.remoteNetworksToUpdate && + networksToUpdate.remoteNetworksToUpdate.length > 0 + ) { await batchUpdateNetworks(networksToUpdate?.remoteNetworksToUpdate, opts); } // Add missing local networks - if (networksToUpdate?.missingLocalNetworks) { + if ( + networksToUpdate?.missingLocalNetworks && + networksToUpdate.missingLocalNetworks.length > 0 + ) { networksToUpdate.missingLocalNetworks.forEach((n) => { try { messenger.call('NetworkController:addNetwork', n); @@ -110,20 +116,21 @@ export async function performMainNetworkSync( } // Update local networks - if (networksToUpdate?.localNetworksToUpdate) { - const promises = networksToUpdate.localNetworksToUpdate.map(async (n) => { - try { - await messenger.call('NetworkController:updateNetwork', n.chainId, n); - props.onNetworkUpdated?.(n.chainId); - } catch { - // Silently fail, we can try this again on next main sync - } - }); - await Promise.all(promises); + if ( + networksToUpdate?.localNetworksToUpdate && + networksToUpdate.localNetworksToUpdate.length > 0 + ) { + for (const n of networksToUpdate.localNetworksToUpdate) { + await messenger.call('NetworkController:updateNetwork', n.chainId, n); + props.onNetworkUpdated?.(n.chainId); + } } // Remove local networks - if (networksToUpdate?.localNetworksToRemove) { + if ( + networksToUpdate?.localNetworksToRemove && + networksToUpdate.localNetworksToRemove.length > 0 + ) { networksToUpdate.localNetworksToRemove.forEach((n) => { try { messenger.call('NetworkController:removeNetwork', n.chainId); From 9df8d49ef0986103687f850e1ddfe6824a0edb4c Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 24 Oct 2024 18:31:24 +0100 Subject: [PATCH 5/5] test: add test coverage --- .../controller-integration.test.ts | 148 ++++++++++++------ .../network-syncing/controller-integration.ts | 4 +- .../user-storage/network-syncing/sync-all.ts | 7 + 3 files changed, 108 insertions(+), 51 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts index 2eaf8079c0..4e6d9a510d 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.test.ts @@ -18,6 +18,7 @@ import { performMainNetworkSync, startNetworkSyncing, } from './controller-integration'; +import * as ControllerIntegrationModule from './controller-integration'; import * as ServicesModule from './services'; import * as SyncAllModule from './sync-all'; import * as SyncMutationsModule from './sync-mutations'; @@ -68,70 +69,117 @@ const getEvents = (): ExternalEvents[] => [ 'NetworkController:networkRemoved', ]; -const testMatrix = [ - { - event: 'NetworkController:networkRemoved' as const, - arrangeSyncFnMock: () => - jest.spyOn(SyncMutationsModule, 'deleteNetwork').mockResolvedValue(), - }, -]; +describe('network-syncing/controller-integration - startNetworkSyncing()', () => { + it(`should successfully sync when NetworkController:networkRemoved is emitted`, async () => { + const { baseMessenger, messenger, getStorageConfig, deleteNetworkMock } = + arrangeMocks(); + startNetworkSyncing({ messenger, getStorageConfig }); + baseMessenger.publish( + 'NetworkController:networkRemoved', + createMockNetworkConfiguration(), + ); -describe.each(testMatrix)( - 'network-syncing/controller-integration - startNetworkSyncing() $event', - ({ event, arrangeSyncFnMock }) => { - it(`should successfully sync when ${event} is emitted`, async () => { - const syncFnMock = arrangeSyncFnMock(); - const { baseMessenger, messenger, getStorageConfig } = arrangeMocks(); - startNetworkSyncing({ messenger, getStorageConfig }); - baseMessenger.publish(event, createMockNetworkConfiguration()); + await waitFor(() => { + expect(getStorageConfig).toHaveBeenCalled(); + expect(deleteNetworkMock).toHaveBeenCalled(); + }); + }); - await waitFor(() => { - expect(getStorageConfig).toHaveBeenCalled(); - expect(syncFnMock).toHaveBeenCalled(); - }); + it('should silently fail is unable to authenticate or get storage key', async () => { + const { baseMessenger, messenger, getStorageConfig, deleteNetworkMock } = + arrangeMocks(); + getStorageConfig.mockRejectedValue(new Error('Mock Error')); + startNetworkSyncing({ messenger, getStorageConfig }); + baseMessenger.publish( + 'NetworkController:networkRemoved', + createMockNetworkConfiguration(), + ); + + await waitFor(() => { + expect(getStorageConfig).toHaveBeenCalled(); + expect(deleteNetworkMock).not.toHaveBeenCalled(); }); + }); - it('should silently fail is unable to authenticate or get storage key', async () => { - const syncFnMock = arrangeSyncFnMock(); - const { baseMessenger, messenger, getStorageConfig } = arrangeMocks(); - getStorageConfig.mockRejectedValue(new Error('Mock Error')); - startNetworkSyncing({ messenger, getStorageConfig }); - baseMessenger.publish(event, createMockNetworkConfiguration()); + it('should silently fail if unable to get storage config', async () => { + const { baseMessenger, messenger, getStorageConfig, deleteNetworkMock } = + arrangeMocks(); + getStorageConfig.mockResolvedValue(null); + startNetworkSyncing({ messenger, getStorageConfig }); + baseMessenger.publish( + 'NetworkController:networkRemoved', + createMockNetworkConfiguration(), + ); + await waitFor(() => { expect(getStorageConfig).toHaveBeenCalled(); - expect(syncFnMock).not.toHaveBeenCalled(); + expect(deleteNetworkMock).not.toHaveBeenCalled(); }); + }); - it(`should emit a warning if controller messenger is missing the ${event} event`, async () => { - const { baseMessenger, getStorageConfig } = arrangeMocks(); + it(`should emit a warning if controller messenger is missing the NetworkController:networkRemoved event`, async () => { + const { baseMessenger, getStorageConfig } = arrangeMocks(); - const eventsWithoutNetworkAdded = getEvents().filter((e) => e !== event); - const messenger = mockUserStorageMessenger( - baseMessenger, - eventsWithoutNetworkAdded, - ); + const eventsWithoutNetworkAdded = getEvents().filter( + (e) => e !== 'NetworkController:networkRemoved', + ); + const messenger = mockUserStorageMessenger( + baseMessenger, + eventsWithoutNetworkAdded, + ); + await waitFor(() => { startNetworkSyncing({ messenger, getStorageConfig }); expect(warnMock).toHaveBeenCalled(); }); + }); + + it('should not remove networks if main sync is in progress', async () => { + const { baseMessenger, getStorageConfig, deleteNetworkMock } = + arrangeMocks(); + const messenger = mockUserStorageMessenger(baseMessenger); + + // TODO - replace with jest.replaceProperty once we upgrade jest. + Object.defineProperty( + ControllerIntegrationModule, + 'isMainNetworkSyncInProgress', + { value: true }, + ); + + startNetworkSyncing({ + messenger, + getStorageConfig, + }); + + baseMessenger.publish( + 'NetworkController:networkRemoved', + createMockNetworkConfiguration(), + ); + + expect(getStorageConfig).not.toHaveBeenCalled(); + expect(deleteNetworkMock).not.toHaveBeenCalled(); + }); + + /** + * Test Utility - arrange mocks and parameters + * @returns the mocks and parameters used when testing `startNetworkSyncing()` + */ + function arrangeMocks() { + const baseMessenger = mockBaseMessenger(); + const messenger = mockUserStorageMessenger(baseMessenger); + const getStorageConfigMock = jest.fn().mockResolvedValue(storageOpts); + const deleteNetworkMock = jest + .spyOn(SyncMutationsModule, 'deleteNetwork') + .mockResolvedValue(); - /** - * Test Utility - arrange mocks and parameters - * @returns the mocks and parameters used when testing `startNetworkSyncing()` - */ - function arrangeMocks() { - const baseMessenger = mockBaseMessenger(); - const messenger = mockUserStorageMessenger(baseMessenger); - const getStorageConfigMock = jest.fn().mockResolvedValue(storageOpts); - - return { - getStorageConfig: getStorageConfigMock, - baseMessenger, - messenger, - }; - } - }, -); + return { + getStorageConfig: getStorageConfigMock, + baseMessenger, + messenger, + deleteNetworkMock, + }; + } +}); describe('network-syncing/controller-integration - performMainSync()', () => { it('should do nothing if unable to get storage config', async () => { diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 7c182f1310..dfe022ea1a 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -23,7 +23,9 @@ type PerformMainNetworkSyncProps = { * Global in-mem cache to signify that the network syncing is in progress * Ensures that listeners do not fire during main sync (prevent double requests) */ -let isMainNetworkSyncInProgress = false; +// Exported to help testing +// eslint-disable-next-line import/no-mutable-exports +export let isMainNetworkSyncInProgress = false; /** * Initialize and setup events to listen to for network syncing diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts index 4f07912d03..a1cc414a32 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/sync-all.ts @@ -81,6 +81,8 @@ export const checkWhichNetworkIsLatest = ( : 'Remote Wins'; } + // Unreachable statement + /* istanbul ignore next */ return 'Do Nothing'; }; @@ -141,6 +143,9 @@ export const getUpdatedNetworkLists = ( const localNetwork = localMap.get(chain); const remoteNetwork = remoteMap.get(chain); if (!localNetwork || !remoteNetwork) { + // This should be unreachable as we know the Maps created will have the values + // This is to satisfy types + /* istanbul ignore next */ return; } @@ -202,5 +207,7 @@ export const findNetworksToUpdate = (props: FindNetworksToUpdateProps) => { // Unable to perform sync, silently fail } + // Unreachable statement + /* istanbul ignore next */ return undefined; };