diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 7de43a0fc6..0b9972ca5b 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -37,7 +37,7 @@ "count": 1 }, "n/no-sync": { - "count": 22 + "count": 29 } }, "packages/account-tree-controller/src/backup-and-sync/service/index.ts": { diff --git a/packages/account-tree-controller/CHANGELOG.md b/packages/account-tree-controller/CHANGELOG.md index c60a1b49fb..0cecc619a5 100644 --- a/packages/account-tree-controller/CHANGELOG.md +++ b/packages/account-tree-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- The `Multichain Account Syncing - Full` trace is now only emitted when a backup-and-sync run actually mutates local or remote state (rolled-back local changes do not count) ([#9343](https://github.com/MetaMask/core/pull/9343)) - Bump `@metamask/keyring-api` from `^23.1.0` to `^23.3.0` ([#9249](https://github.com/MetaMask/core/pull/9249)) - Bump `@metamask/multichain-account-service` from `^11.0.0` to `^11.1.0` ([#9264](https://github.com/MetaMask/core/pull/9264)) - Bump `@metamask/accounts-controller` from `^39.0.3` to `^39.0.4` ([#9349](https://github.com/MetaMask/core/pull/9349)) diff --git a/packages/account-tree-controller/src/AccountTreeController.ts b/packages/account-tree-controller/src/AccountTreeController.ts index 5640662e9f..e37b2dc152 100644 --- a/packages/account-tree-controller/src/AccountTreeController.ts +++ b/packages/account-tree-controller/src/AccountTreeController.ts @@ -22,6 +22,7 @@ import { } from './backup-and-sync/analytics'; import { BackupAndSyncService } from './backup-and-sync/service'; import type { BackupAndSyncContext } from './backup-and-sync/types'; +import { createSyncMutationTracker } from './backup-and-sync/utils'; import type { AccountGroupObject, AccountTypeOrderKey } from './group'; import { ACCOUNT_TYPE_TO_SORT_ORDER, @@ -1803,6 +1804,7 @@ export class AccountTreeController extends BaseController< controllerStateUpdateFn: this.update.bind(this), traceFn: this.#trace.bind(this), groupIdToWalletId: this.#groupIdToWalletId, + mutationTracker: createSyncMutationTracker(), }; } } diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts index ebbda948e3..f93f254fb2 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.test.ts @@ -2,11 +2,15 @@ import { AccountWalletType } from '@metamask/account-api'; import { BackupAndSyncService } from '.'; import type { AccountGroupObject } from '../../group'; +import type { AccountTreeControllerState } from '../../types'; import type { AccountWalletEntropyObject } from '../../wallet'; +import { TraceName } from '../analytics'; import { getProfileId } from '../authentication'; +import { performLegacyAccountSyncing, syncWalletMetadata } from '../syncing'; import type { BackupAndSyncContext } from '../types'; +import { getAllGroupsFromUserStorage } from '../user-storage'; // We only need to import the functions we actually spy on -import { getLocalEntropyWallets } from '../utils'; +import { createStateSnapshot, getLocalEntropyWallets } from '../utils'; // Mock the sync functions and all external dependencies jest.mock('../syncing'); @@ -20,6 +24,26 @@ const mockGetProfileId = getProfileId as jest.MockedFunction< >; const mockGetLocalEntropyWallets = getLocalEntropyWallets as jest.MockedFunction; +const mockSyncWalletMetadata = syncWalletMetadata as jest.MockedFunction< + typeof syncWalletMetadata +>; +const mockPerformLegacyAccountSyncing = + performLegacyAccountSyncing as jest.MockedFunction< + typeof performLegacyAccountSyncing + >; +const mockGetAllGroupsFromUserStorage = + getAllGroupsFromUserStorage as jest.MockedFunction< + typeof getAllGroupsFromUserStorage + >; +const mockCreateStateSnapshot = createStateSnapshot as jest.MockedFunction< + typeof createStateSnapshot +>; + +// `jest.mock('../utils')` auto-mocks every export, including the tracker +// factory. Grab the real one so tests use the actual tracker behaviour rather +// than a hand-rolled duplicate. +const { createSyncMutationTracker } = + jest.requireActual('../utils'); describe('BackupAndSync - Service - BackupAndSyncService', () => { let mockContext: BackupAndSyncContext; @@ -57,6 +81,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { }, traceFn: jest.fn().mockImplementation((_config, fn) => fn()), groupIdToWalletId: new Map(), + mutationTracker: createSyncMutationTracker(), } as unknown as BackupAndSyncContext; // Default setup - backup and sync enabled @@ -65,6 +90,11 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Setup default mock returns mockGetLocalEntropyWallets.mockReturnValue([]); mockGetProfileId.mockResolvedValue('test-profile-id'); + // Return a truthy snapshot so the per-wallet rollback path runs (the real + // implementation always returns a snapshot object). + mockCreateStateSnapshot.mockReturnValue( + {} as ReturnType, + ); backupAndSyncService = new BackupAndSyncService(mockContext); }); @@ -325,33 +355,188 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Should have updated controller state to mark sync in progress and then completed expect(mockContext.controllerStateUpdateFn).toHaveBeenCalled(); - // Should have called traceFn to wrap the sync operation - expect(mockContext.traceFn).toHaveBeenCalled(); - // The key difference: full sync should call getLocalEntropyWallets expect(mockGetLocalEntropyWallets).toHaveBeenCalled(); }); - it('awaits the ongoing promise if a second call is made during sync', async () => { - // Mock some local wallets for the full sync to process + it('emits a backdated AccountSyncFull span when the sync mutates local state', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + // Empty remote groups makes the wallet run complete cleanly (push + skip) + // without hitting the rollback path. + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); + + // Simulate a local write happening during the sync by having a mocked + // helper report a mutation through the context. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setLocalWrite(true); + }); + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), + }, + expect.any(Function), + ); + // The traced callback is empty (the span is backdated, work already ran). + const [, tracedCallback] = (mockContext.traceFn as jest.Mock).mock + .calls[0]; + expect(tracedCallback()).toBeUndefined(); + }); + + it('clears the in-progress flag even when the trace emit fails', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setLocalWrite(true); + }); + // Tracing is best-effort: a rejected trace must not fail the sync nor + // leave the controller stuck mid-sync. + (mockContext.traceFn as jest.Mock).mockRejectedValue( + new Error('trace boom'), + ); + + expect(await backupAndSyncService.performFullSync()).toBeUndefined(); + + // Replay every state update; the in-progress flag must end up cleared. + const state = { + isAccountTreeSyncingInProgress: true, + } as AccountTreeControllerState; + for (const [updater] of (mockContext.controllerStateUpdateFn as jest.Mock) + .mock.calls) { + updater(state); + } + expect(state.isAccountTreeSyncingInProgress).toBe(false); + }); + + it('emits an AccountSyncFull span for a durable remote write even if the wallet is rolled back', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // A remote push happens, then the wallet fails and is rolled back. Remote + // writes are durable, so the run must still emit. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setRemoteWrite(true); + throw new Error('boom'); + }); + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), + }, + expect.any(Function), + ); + }); + + it("does not emit an AccountSyncFull span when a wallet's local changes are rolled back", async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // A local write happens, then the wallet fails and is rolled back. The + // local change is reverted, so the run must not emit. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setLocalWrite(true); + throw new Error('boom'); + }); + + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).not.toHaveBeenCalled(); + }); + + it('emits an AccountSyncFull span when the run throws after doing durable work', async () => { mockGetLocalEntropyWallets.mockReturnValue([ { id: 'entropy:wallet-1', metadata: { entropy: { id: 'test-entropy-id' } }, } as unknown as AccountWalletEntropyObject, + { + id: 'entropy:wallet-2', + metadata: { entropy: { id: 'test-entropy-id-2' } }, + } as unknown as AccountWalletEntropyObject, ]); + mockGetAllGroupsFromUserStorage.mockResolvedValue([]); - // Make traceFn actually async to simulate real sync work - let resolveTrace: (() => void) | undefined; - const tracePromise = new Promise((resolve) => { - resolveTrace = resolve; + // Wallet 1 performs a durable remote write and completes; wallet 2's + // legacy sync then fails and aborts the whole run. + mockSyncWalletMetadata.mockImplementation(async (context) => { + context.mutationTracker?.setRemoteWrite(true); }); - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - fn(); - return tracePromise; + mockPerformLegacyAccountSyncing + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('legacy boom')); + + await expect(backupAndSyncService.performFullSync()).rejects.toThrow( + 'Legacy syncing failed', + ); + + // The span is still recorded despite the failure. + expect(mockContext.traceFn).toHaveBeenCalledTimes(1); + expect(mockContext.traceFn).toHaveBeenCalledWith( + { + name: TraceName.AccountSyncFull, + startTime: expect.any(Number), }, + expect.any(Function), ); + }); + + it('does not emit an AccountSyncFull span when the sync is a no-op', async () => { + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // No mocked helper reports a mutation, so the sync is a no-op. + await backupAndSyncService.performFullSync(); + + expect(mockContext.traceFn).not.toHaveBeenCalled(); + }); + + it('awaits the ongoing promise if a second call is made during sync', async () => { + // Mock some local wallets for the full sync to process + mockGetLocalEntropyWallets.mockReturnValue([ + { + id: 'entropy:wallet-1', + metadata: { entropy: { id: 'test-entropy-id' } }, + } as unknown as AccountWalletEntropyObject, + ]); + + // Make the sync work stay pending so the second call lands mid-sync. + let resolveSync: (() => void) | undefined; + const syncPromise = new Promise((resolve) => { + resolveSync = resolve; + }); + mockSyncWalletMetadata.mockImplementation(async () => { + await syncPromise; + }); // Start first sync const firstSyncPromise = backupAndSyncService.performFullSync(); @@ -362,8 +547,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // Both promises should be the same reference expect(firstSyncPromise).toStrictEqual(secondSyncPromise); - // Resolve the trace to complete the sync - resolveTrace?.(); + // Resolve the sync work to complete the run + resolveSync?.(); // Both should resolve to the same value const [firstResult, secondResult] = await Promise.all([ @@ -385,15 +570,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track how many times the actual sync logic runs - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Fire multiple syncs rapidly const promises = [ backupAndSyncService.performFullSync(), @@ -409,9 +585,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all(promises); // Should only have executed the sync logic once - expect(syncExecutionCount).toBe(1); - - // getLocalEntropyWallets should only be called once + // (getLocalEntropyWallets runs once per sync run) expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // All promises should resolve successfully to the same value @@ -429,15 +603,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track how many times the actual sync logic runs - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Start first sync and wait for it to complete const firstSyncPromise = backupAndSyncService.performFullSync(); await firstSyncPromise; @@ -452,9 +617,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondSyncPromise; // Should have executed the sync logic twice (once for each call) - expect(syncExecutionCount).toBe(2); - - // getLocalEntropyWallets should be called twice (once for each sync) + // (getLocalEntropyWallets runs once per sync run) expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // Both promises should resolve successfully @@ -471,15 +634,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Perform first sync const firstSyncPromise = backupAndSyncService.performFullSync(); @@ -493,7 +647,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all([firstSyncPromise, atLeastOncePromise]); // Should only have executed once - expect(syncExecutionCount).toBe(1); + // (getLocalEntropyWallets runs once per sync run) + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); }); @@ -525,22 +680,12 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - const syncPromise = backupAndSyncService.performFullSyncAtLeastOnce(); expect(syncPromise).toBeInstanceOf(Promise); await syncPromise; - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); @@ -553,15 +698,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Fire multiple calls rapidly const promises = [ backupAndSyncService.performFullSyncAtLeastOnce(), @@ -577,7 +713,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all(promises); // Should only have executed the sync logic once - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // All promises should resolve successfully to the same value @@ -595,15 +730,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Start first sync and wait for it to complete const firstSyncPromise = backupAndSyncService.performFullSyncAtLeastOnce(); @@ -620,7 +746,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondSyncPromise; // Should only have executed the sync logic once (no new sync created) - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // Both promises should resolve successfully @@ -637,22 +762,12 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Multiple sequential calls await backupAndSyncService.performFullSyncAtLeastOnce(); await backupAndSyncService.performFullSyncAtLeastOnce(); await backupAndSyncService.performFullSyncAtLeastOnce(); // Should only have executed once, regardless of how many times it's called - expect(syncExecutionCount).toBe(1); expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); }); @@ -665,15 +780,6 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { } as unknown as AccountWalletEntropyObject, ]); - // Track sync execution - let syncExecutionCount = 0; - (mockContext.traceFn as jest.Mock).mockImplementation( - (_: unknown, fn: () => unknown) => { - syncExecutionCount += 1; - return fn(); - }, - ); - // Call performFullSyncAtLeastOnce first const atLeastOncePromise = backupAndSyncService.performFullSyncAtLeastOnce(); @@ -687,7 +793,8 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await Promise.all([atLeastOncePromise, fullSyncPromise]); // Should only have executed once - expect(syncExecutionCount).toBe(1); + // (getLocalEntropyWallets runs once per sync run) + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(1); // Now call performFullSync again after completion const secondFullSyncPromise = backupAndSyncService.performFullSync(); @@ -698,7 +805,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { await secondFullSyncPromise; // Should have executed twice now (one for each performFullSync call) - expect(syncExecutionCount).toBe(2); + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // But performFullSyncAtLeastOnce should still return the original promise const laterAtLeastOncePromise = @@ -707,7 +814,7 @@ describe('BackupAndSync - Service - BackupAndSyncService', () => { // And should not trigger another sync await laterAtLeastOncePromise; - expect(syncExecutionCount).toBe(2); // Still only 2 + expect(mockGetLocalEntropyWallets).toHaveBeenCalledTimes(2); // Still only 2 }, 15000); // Increase timeout to 15 seconds }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/service/index.ts b/packages/account-tree-controller/src/backup-and-sync/service/index.ts index 852af9daad..749a113c38 100644 --- a/packages/account-tree-controller/src/backup-and-sync/service/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/service/index.ts @@ -359,6 +359,12 @@ export class BackupAndSyncService { // 3. Execute multichain account syncing let stateSnapshot: StateSnapshot | undefined; + // Capture the local-write flag so it can be reverted together with + // the state snapshot if this wallet is rolled back. Remote writes are + // durable and intentionally excluded. + const localWriteBeforeWallet = + this.#context.mutationTracker?.getLocalWrite() ?? false; + try { // 3.1 Wallet syncing // Create a state snapshot before processing each wallet for potential rollback @@ -416,6 +422,11 @@ export class BackupAndSyncService { ); } restoreStateFromSnapshot(this.#context, stateSnapshot); + // Revert the local-write flag too, so a rolled-back wallet does + // not keep the run marked as having changed local state. + this.#context.mutationTracker?.setLocalWrite( + localWriteBeforeWallet, + ); backupAndSyncLogger( `Rolled back state changes for wallet ${wallet.id}`, ); @@ -442,21 +453,37 @@ export class BackupAndSyncService { }); }; - // Execute the big sync function with tracing and ensure state cleanup + // Execute the big sync function and ensure state cleanup. + // The sync runs untraced so we can decide afterwards whether it did any + // real work; the span is then backdated to preserve the real duration. + const { mutationTracker } = this.#context; + mutationTracker?.reset(); + const startTime = Date.now(); try { - await this.#context.traceFn( - { - name: TraceName.AccountSyncFull, - }, - bigSyncFn, - ); + await bigSyncFn(); } finally { - // Always reset state, regardless of success or failure this.#context.controllerStateUpdateFn( (state: AccountTreeControllerState) => { state.isAccountTreeSyncingInProgress = false; }, ); + + if (mutationTracker?.hasOccurred()) { + try { + await this.#context.traceFn( + { + name: TraceName.AccountSyncFull, + startTime, + }, + () => undefined, + ); + } catch (traceError) { + backupAndSyncLogger( + 'Failed to emit AccountSyncFull trace:', + traceError, + ); + } + } } } diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts index 686769a399..2e546f2d5b 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.test.ts @@ -47,9 +47,11 @@ describe('BackupAndSync - Syncing - Group', () => { let mockContext: BackupAndSyncContext; let mockLocalGroup: AccountGroupMultichainAccountObject; let mockWallet: AccountWalletEntropyObject; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { mockGetLocalGroupsForEntropyWallet.mockReturnValue([]); + mockSetLocalWrite = jest.fn(); mockContext = { controller: { @@ -71,6 +73,9 @@ describe('BackupAndSync - Syncing - Group', () => { call: jest.fn(), }, emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + setLocalWrite: mockSetLocalWrite, + }, } as unknown as BackupAndSyncContext; mockLocalGroup = { @@ -134,6 +139,7 @@ describe('BackupAndSync - Syncing - Group', () => { expect(mockContext.messenger.call).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('emits analytics events for successful creations', async () => { @@ -155,6 +161,7 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); }); it('only emits analytics for newly created groups, not pre-existing ones', async () => { @@ -192,6 +199,8 @@ describe('BackupAndSync - Syncing - Group', () => { action: BackupAndSyncAnalyticsEvent.GroupAdded, profileId: 'test-profile', }); + // setLocalWrite(true) should fire once per newly created group (1 and 2). + expect(mockSetLocalWrite).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts index 2a6c90284e..273625027d 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/group.ts @@ -71,6 +71,7 @@ export const createMultichainAccountGroupsBatch = async ( const didGroupAlreadyExist = existingGroupIndices.has(group.groupIndex); if (!didGroupAlreadyExist) { + context.mutationTracker?.setLocalWrite(true); context.emitAnalyticsEventFn({ action: analyticsAction, profileId, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts index d4e10da5ae..00ee21d029 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.test.ts @@ -35,13 +35,19 @@ const mockCreateMultichainAccountGroupsBatch = describe('BackupAndSync - Syncing - Legacy', () => { let mockContext: BackupAndSyncContext; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { + mockSetLocalWrite = jest.fn(); + mockContext = { controller: { setAccountGroupName: jest.fn(), }, emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + setLocalWrite: mockSetLocalWrite, + }, } as unknown as BackupAndSyncContext; }); @@ -151,6 +157,8 @@ describe('BackupAndSync - Syncing - Legacy', () => { 'Legacy Account 2', true, ); + // Two renames -> two mutations marked. + expect(mockSetLocalWrite).toHaveBeenCalledTimes(2); }); it('skips legacy accounts with missing name or address', async () => { @@ -198,6 +206,7 @@ describe('BackupAndSync - Syncing - Legacy', () => { ); expect(mockContext.controller.setAccountGroupName).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('emits analytics event on completion', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts index 616fc2f393..b0749889d1 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/legacy.ts @@ -88,6 +88,7 @@ export const performLegacyAccountSyncing = async ( ); if (localGroup) { context.controller.setAccountGroupName(localGroup.id, n, true); + context.mutationTracker?.setLocalWrite(true); context.emitAnalyticsEventFn({ action: BackupAndSyncAnalyticsEvent.LegacyGroupRenamed, diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts index ea97990f40..072ceefded 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.test.ts @@ -6,13 +6,18 @@ describe('BackupAndSync - Syncing - Metadata', () => { let mockContext: BackupAndSyncContext; let mockApplyLocalUpdate: jest.Mock; let mockValidateUserStorageValue: jest.Mock; + let mockSetLocalWrite: jest.Mock; beforeEach(() => { mockApplyLocalUpdate = jest.fn(); mockValidateUserStorageValue = jest.fn().mockReturnValue(true); + mockSetLocalWrite = jest.fn(); mockContext = { emitAnalyticsEventFn: jest.fn(), + mutationTracker: { + setLocalWrite: mockSetLocalWrite, + }, } as unknown as BackupAndSyncContext; }); @@ -33,6 +38,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('applies user storage value when it is more recent and valid', async () => { @@ -50,6 +56,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('new'); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); expect(mockContext.emitAnalyticsEventFn).toHaveBeenCalledWith({ action: BackupAndSyncAnalyticsEvent.GroupRenamed, profileId: 'test-profile', @@ -68,6 +75,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('returns true when user storage value is invalid', async () => { @@ -84,6 +92,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(true); expect(mockApplyLocalUpdate).not.toHaveBeenCalled(); expect(mockContext.emitAnalyticsEventFn).not.toHaveBeenCalled(); + expect(mockSetLocalWrite).not.toHaveBeenCalled(); }); it('applies user storage value when no local metadata exists', async () => { @@ -97,6 +106,7 @@ describe('BackupAndSync - Syncing - Metadata', () => { expect(result).toBe(false); expect(mockApplyLocalUpdate).toHaveBeenCalledWith('remote'); + expect(mockSetLocalWrite).toHaveBeenCalledTimes(1); }); it('does not emit analytics when no analytics config provided', async () => { diff --git a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts index c148de7a04..c3d5787666 100644 --- a/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts +++ b/packages/account-tree-controller/src/backup-and-sync/syncing/metadata.ts @@ -62,6 +62,7 @@ export async function compareAndSyncMetadata({ if ((isUserStorageMoreRecent || !localMetadata) && isUserStorageValueValid) { // User storage is more recent and valid, apply it locally await applyLocalUpdate(userStorageValue as T); + context.mutationTracker?.setLocalWrite(true); // Emit analytics event if provided if (analytics) { diff --git a/packages/account-tree-controller/src/backup-and-sync/types.ts b/packages/account-tree-controller/src/backup-and-sync/types.ts index 2816be6035..7f6449c063 100644 --- a/packages/account-tree-controller/src/backup-and-sync/types.ts +++ b/packages/account-tree-controller/src/backup-and-sync/types.ts @@ -77,6 +77,28 @@ export type LegacyUserStorageSyncedAccount = Infer< typeof LegacyUserStorageSyncedAccountSchema >; +/** + * Tracks whether the current full sync run performed a real write, so the + * service can gate emission of the full-sync trace. + * + * Writes are split by durability: + * - Remote writes (pushes to user storage) are durable and always count. + * - Local writes can be reverted by a per-wallet rollback, so the service + * reads the flag before a wallet and writes it back if that wallet rolls back. + */ +export type SyncMutationTracker = { + /** Sets (or clears) the durable remote-write flag — a push to user storage. */ + setRemoteWrite: (value: boolean) => void; + /** Gets the local-write flag — a write a per-wallet rollback would revert. */ + getLocalWrite: () => boolean; + /** Sets, clears, or restores the local-write flag. */ + setLocalWrite: (value: boolean) => void; + /** Whether any write (remote or local) is currently recorded. */ + hasOccurred: () => boolean; + /** Clears all recorded writes at the start of a sync run. */ + reset: () => void; +}; + export type BackupAndSyncContext = { messenger: AccountTreeControllerMessenger; controller: AccountTreeController; @@ -84,6 +106,7 @@ export type BackupAndSyncContext = { traceFn: TraceCallback; groupIdToWalletId: Map; emitAnalyticsEventFn: (event: BackupAndSyncEmitAnalyticsEventParams) => void; + mutationTracker?: SyncMutationTracker; }; export type LegacyAccountSyncingContext = { diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts index e364fb2a7e..deaab84014 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.test.ts @@ -61,12 +61,18 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { let mockContext: BackupAndSyncContext; let mockWallet: AccountWalletEntropyObject; let mockGroup: AccountGroupMultichainAccountObject; + let mockSetRemoteWrite: jest.Mock; beforeEach(() => { + mockSetRemoteWrite = jest.fn(); + mockContext = { messenger: { call: jest.fn(), }, + mutationTracker: { + setRemoteWrite: mockSetRemoteWrite, + }, } as unknown as BackupAndSyncContext; mockWallet = { @@ -204,6 +210,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedWallet), 'test-entropy-id', ); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); @@ -436,6 +443,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { JSON.stringify(formattedGroup), 'test-entropy-id', ); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); @@ -466,6 +474,7 @@ describe('BackupAndSync - UserStorage - NetworkOperations', () => { ], 'test-entropy-id', ); + expect(mockSetRemoteWrite).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts index 838f7b8e61..52cd55e9e3 100644 --- a/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts +++ b/packages/account-tree-controller/src/backup-and-sync/user-storage/network-operations.ts @@ -82,12 +82,14 @@ export const pushWalletToUserStorage = async ( backupAndSyncLogger(`Pushing wallet to user storage: ${stringifiedWallet}`); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performSetStorage', `${USER_STORAGE_WALLETS_FEATURE_KEY}.${USER_STORAGE_WALLETS_FEATURE_ENTRY_KEY}`, stringifiedWallet, entropySourceId, ); + context.mutationTracker?.setRemoteWrite(true); + return result; }); }; @@ -193,12 +195,14 @@ export const pushGroupToUserStorage = async ( backupAndSyncLogger(`Pushing group to user storage: ${stringifiedGroup}`); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performSetStorage', `${USER_STORAGE_GROUPS_FEATURE_KEY}.${formattedGroup.groupIndex}`, stringifiedGroup, entropySourceId, ); + context.mutationTracker?.setRemoteWrite(true); + return result; }); }; @@ -232,12 +236,14 @@ export const pushGroupToUserStorageBatch = async ( `Pushing groups to user storage: ${entries.map(([_, value]) => value).join(', ')}`, ); - return await context.messenger.call( + const result = await context.messenger.call( 'UserStorageController:performBatchSetStorage', USER_STORAGE_GROUPS_FEATURE_KEY, entries, entropySourceId, ); + context.mutationTracker?.setRemoteWrite(true); + return result; }); }; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/index.ts b/packages/account-tree-controller/src/backup-and-sync/utils/index.ts index 7e83644ae2..f84fb0dafd 100644 --- a/packages/account-tree-controller/src/backup-and-sync/utils/index.ts +++ b/packages/account-tree-controller/src/backup-and-sync/utils/index.ts @@ -1,2 +1,3 @@ export * from './controller'; export { toErrorMessage } from './errors'; +export { createSyncMutationTracker } from './mutation-tracker'; diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts new file mode 100644 index 0000000000..43444baffe --- /dev/null +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.test.ts @@ -0,0 +1,63 @@ +import { createSyncMutationTracker } from './mutation-tracker'; + +describe('BackupAndSync - Utils - createSyncMutationTracker', () => { + it('starts with no recorded writes', () => { + const tracker = createSyncMutationTracker(); + + expect(tracker.hasOccurred()).toBe(false); + expect(tracker.getLocalWrite()).toBe(false); + }); + + it('records remote writes', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + + expect(tracker.hasOccurred()).toBe(true); + }); + + it('records local writes', () => { + const tracker = createSyncMutationTracker(); + + tracker.setLocalWrite(true); + + expect(tracker.hasOccurred()).toBe(true); + expect(tracker.getLocalWrite()).toBe(true); + }); + + it('clears all writes on reset', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + tracker.setLocalWrite(true); + tracker.reset(); + + expect(tracker.hasOccurred()).toBe(false); + expect(tracker.getLocalWrite()).toBe(false); + }); + + it('reverts local writes to a captured value', () => { + const tracker = createSyncMutationTracker(); + + const before = tracker.getLocalWrite(); + tracker.setLocalWrite(true); + expect(tracker.hasOccurred()).toBe(true); + + tracker.setLocalWrite(before); + + expect(tracker.hasOccurred()).toBe(false); + }); + + it('keeps durable remote writes when local writes are reverted', () => { + const tracker = createSyncMutationTracker(); + + tracker.setRemoteWrite(true); + const before = tracker.getLocalWrite(); + tracker.setLocalWrite(true); + + tracker.setLocalWrite(before); + + // Local write is gone, but the durable remote write still counts. + expect(tracker.hasOccurred()).toBe(true); + }); +}); diff --git a/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts new file mode 100644 index 0000000000..aa0d3e74be --- /dev/null +++ b/packages/account-tree-controller/src/backup-and-sync/utils/mutation-tracker.ts @@ -0,0 +1,26 @@ +import type { SyncMutationTracker } from '../types'; + +/** + * Creates a tracker that records whether the current full sync run performed a + * real write. + * + * @returns A fresh mutation tracker. + */ +export const createSyncMutationTracker = (): SyncMutationTracker => { + let remoteWrite = false; + let localWrite = false; + return { + setRemoteWrite: (value: boolean): void => { + remoteWrite = value; + }, + getLocalWrite: (): boolean => localWrite, + setLocalWrite: (value: boolean): void => { + localWrite = value; + }, + hasOccurred: (): boolean => remoteWrite || localWrite, + reset: (): void => { + remoteWrite = false; + localWrite = false; + }, + }; +};