From b35c66a054830a5fad8ebfd00f7b66b497f75c82 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 24 Aug 2026 17:25:16 +0200 Subject: [PATCH 1/4] fix(assets-controller): skip redundant Accounts API refetch when WebSocket already confirmed the transaction When a transaction is confirmed, AssetsController forces an Accounts API balance refetch even if AccountActivityService's WebSocket already delivered the same balance update for that chain. Now it waits briefly for a matching transactionUpdated event before falling back to the API call, handling both possible arrival orders of the two signals. --- packages/assets-controller/CHANGELOG.md | 4 + .../src/AssetsController.test.ts | 216 ++++++++++++++++++ .../assets-controller/src/AssetsController.ts | 124 +++++++++- 3 files changed, 339 insertions(+), 5 deletions(-) diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 69e72da088..b310ca5718 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Skip the forced Accounts API balance refetch on `TransactionController:transactionConfirmed` when `AccountActivityService` (WebSocket) already reports the transaction's chain as active: `AssetsController` now waits up to 1.5s for a matching `AccountActivityService:transactionUpdated` event (correlated by transaction hash) before falling back to the API call, avoiding a redundant HTTP request when the WebSocket already delivered the same balance update + ### Fixed - Fix `AccountsApiDataSource` polling being served stale cached balances on roughly every other tick, since the 60s balances cache outlived the 30s poll interval ([#9926](https://github.com/MetaMask/core/pull/9926)) diff --git a/packages/assets-controller/src/AssetsController.test.ts b/packages/assets-controller/src/AssetsController.test.ts index febeb0d73b..ce9feb9ee8 100644 --- a/packages/assets-controller/src/AssetsController.test.ts +++ b/packages/assets-controller/src/AssetsController.test.ts @@ -2853,6 +2853,222 @@ describe('AssetsController', () => { }); }); + it('does not force refresh when transaction hash is missing, chain not WS-active', async () => { + // Default AccountActivityDataSource state has no active chains, so the + // WS gate should be skipped and the API refetch should happen right away. + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + + expect(getAssetsSpy).toHaveBeenCalledWith( + [expect.objectContaining({ id: MOCK_ACCOUNT_ID })], + { + chainIds: ['eip155:42161'], + forceUpdate: true, + }, + ); + + getAssetsSpy.mockRestore(); + }); + }); + + it('skips the forced Accounts API refetch when the WebSocket already confirmed the same transaction', async () => { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + // Mark the chain as WS-active (AccountActivityService reported "up"). + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + hash: '0xDEADBEEF', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + // The WebSocket delivers the matching transaction confirmation before + // the bounded wait times out. + messenger.publish('AccountActivityService:transactionUpdated', { + id: '0xdeadbeef', + chain: 'eip155:42161', + status: 'confirmed', + timestamp: Date.now(), + from: '0x1234567890123456789012345678901234567890', + to: '0x9876543210987654321098765432109876543210', + }); + + await flushPromises(); + + expect(getAssetsSpy).not.toHaveBeenCalled(); + + getAssetsSpy.mockRestore(); + }); + }); + + it('skips the forced Accounts API refetch when the WebSocket update arrives before transactionConfirmed (race)', async () => { + // Regression test: `AccountActivityService:transactionUpdated` and + // `TransactionController:transactionConfirmed` are independent + // signals that can arrive in either order. If the WS message arrives + // first, a naive "listen only for future events" wait would miss it. + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + // The WebSocket delivers the matching transaction confirmation + // *before* transactionConfirmed fires. + messenger.publish('AccountActivityService:transactionUpdated', { + id: '0xdeadbeef', + chain: 'eip155:42161', + status: 'confirmed', + timestamp: Date.now(), + from: '0x1234567890123456789012345678901234567890', + to: '0x9876543210987654321098765432109876543210', + }); + + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + hash: '0xDEADBEEF', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + + expect(getAssetsSpy).not.toHaveBeenCalled(); + + getAssetsSpy.mockRestore(); + }); + }); + + it('does not double-refetch when transactionConfirmed fires twice for the same transaction', async () => { + // Regression test: a second `transactionConfirmed` for the same hash + // used to leave the first wait's timer running unclaimed, which could + // fire later and either double-refetch or clear the wrong wait. + jest.useFakeTimers({ doNotFake: ['setImmediate', 'nextTick'] }); + + try { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + const confirmedEvent = { + chainId: '0xa4b1', + hash: '0xdeadbeef', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }; + messenger.publish('TransactionController:transactionConfirmed', confirmedEvent); + messenger.publish('TransactionController:transactionConfirmed', confirmedEvent); + + await flushPromises(); + await jest.advanceTimersByTimeAsync(1_500); + await flushPromises(); + + expect(getAssetsSpy).toHaveBeenCalledTimes(1); + + getAssetsSpy.mockRestore(); + }); + } finally { + jest.useRealTimers(); + } + }); + + it('falls back to the forced Accounts API refetch when the WebSocket confirmation times out', async () => { + jest.useFakeTimers({ doNotFake: ['setImmediate', 'nextTick'] }); + + try { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + // Mark the chain as WS-active, but never publish a matching + // transactionUpdated event, simulating a dropped/late message. + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + hash: '0xdeadbeef', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + expect(getAssetsSpy).not.toHaveBeenCalled(); + + await jest.advanceTimersByTimeAsync(1_500); + await flushPromises(); + + expect(getAssetsSpy).toHaveBeenCalledWith( + [expect.objectContaining({ id: MOCK_ACCOUNT_ID })], + { + chainIds: ['eip155:42161'], + forceUpdate: true, + }, + ); + + getAssetsSpy.mockRestore(); + }); + } finally { + jest.useRealTimers(); + } + }); + + it('does not wait for the WebSocket when the confirmed transaction has no hash', async () => { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + // Chain is WS-active, but there's no hash to correlate against. + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + + expect(getAssetsSpy).toHaveBeenCalledWith( + [expect.objectContaining({ id: MOCK_ACCOUNT_ID })], + { + chainIds: ['eip155:42161'], + forceUpdate: true, + }, + ); + + getAssetsSpy.mockRestore(); + }); + }); + it('publishes balanceChanged event when balance updates', async () => { await withController(async ({ controller, messenger }) => { const balanceChangedHandler = jest.fn(); diff --git a/packages/assets-controller/src/AssetsController.ts b/packages/assets-controller/src/AssetsController.ts index 65a2cdba70..0e28fe325d 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -20,7 +20,9 @@ import type { ApiPlatformClient, AccountActivityServiceBalanceUpdatedEvent, AccountActivityServiceStatusChangedEvent, + AccountActivityServiceTransactionUpdatedEvent, SupportedCurrency, + Transaction as WsTransaction, } from '@metamask/core-backend'; import type { KeyringControllerLockEvent, @@ -210,6 +212,12 @@ const MESSENGER_EXPOSED_METHODS = [ /** Default polling interval hint for data sources (30 seconds) */ const DEFAULT_POLLING_INTERVAL_MS = 30_000; +/** + * How long to wait for a WebSocket transaction confirmation before falling + * back to a forced Accounts API refetch (see `#refreshAssetsAfterTransactionConfirmed`). + */ +const WS_TRANSACTION_CONFIRMATION_TIMEOUT_MS = 1_500; + // ============================================================================ // TRACE NAMES — used in Sentry spans (search these strings in Discover) // ============================================================================ @@ -372,6 +380,8 @@ type AllowedEvents = // AccountActivityService (real-time balance updates + chain status for unified assets) | AccountActivityServiceBalanceUpdatedEvent | AccountActivityServiceStatusChangedEvent + // AccountActivityService (skip redundant Accounts API refetch on tx confirm) + | AccountActivityServiceTransactionUpdatedEvent // AccountsApiDataSource subscribes to react to Snaps → AssetsController // migration flag changes (which gate the chains it surfaces as active) | RemoteFeatureFlagControllerStateChangeEvent; @@ -803,6 +813,19 @@ export class AssetsController extends BaseController< readonly #accountActivityDataSource: AccountActivityDataSource; + /** + * Coordinates the race between `transactionConfirmed` and the WS + * `transactionUpdated` push for a given transaction, keyed by + * `${chain}:${txId}`. Whichever arrives first records an entry; whichever + * arrives second consumes it. A `pending` entry means we're about to force + * an Accounts API refetch unless the WS beats the clock; an `arrived` + * entry means the WS already confirmed and is just waiting to be noticed. + */ + readonly #wsConfirmations = new Map< + string, + { kind: 'pending' | 'arrived'; timeout: NodeJS.Timeout } + >(); + readonly #accountsApiDataSource: AccountsApiDataSource; readonly #snapDataSource: SnapDataSource; @@ -1216,6 +1239,42 @@ export class AssetsController extends BaseController< this.#accountTreeInitialized = false; this.#updateActive(); }); + + // Got here first? `transactionConfirmed` hasn't fired yet, so just note + // that the WS confirmed and let it find out later. Got here second? Then + // `transactionConfirmed` is already waiting on a timer - cancel it. + this.messenger.subscribe( + 'AccountActivityService:transactionUpdated', + ({ chain, id }: WsTransaction) => { + const key = `${chain}:${id.toLowerCase()}`; + if (this.#takeWsConfirmation(key) !== 'pending') { + this.#wsConfirmations.set(key, { + kind: 'arrived', + timeout: setTimeout( + () => this.#wsConfirmations.delete(key), + WS_TRANSACTION_CONFIRMATION_TIMEOUT_MS, + ), + }); + } + }, + ); + } + + /** + * Removes and returns the kind of an existing `#wsConfirmations` entry, + * clearing its timeout so it can be safely replaced or dropped. + * + * @param key - The `${chain}:${txId}` key to look up. + * @returns The entry's kind, or `undefined` if there was no entry. + */ + #takeWsConfirmation(key: string): 'pending' | 'arrived' | undefined { + const existing = this.#wsConfirmations.get(key); + if (!existing) { + return undefined; + } + clearTimeout(existing.timeout); + this.#wsConfirmations.delete(key); + return existing.kind; } #onUnapprovedTransactionAdded(transactionMeta: TransactionMeta): void { @@ -1266,11 +1325,60 @@ export class AssetsController extends BaseController< return; } - this.getAssets([matchedAccount], { - chainIds: [caipChainId], - forceUpdate: true, - }).catch((error) => { - log('Failed to refresh assets after transaction confirmed', { error }); + this.#refreshAssetsAfterTransactionConfirmed( + matchedAccount, + caipChainId, + transactionMeta.hash, + ); + } + + /** + * Refreshes balances for a confirmed transaction. Skips the forced + * Accounts API call if the chain's WebSocket is active and confirms the + * same transaction within {@link WS_TRANSACTION_CONFIRMATION_TIMEOUT_MS}, + * since that push already delivered the same balance update. + * + * @param account - The account whose balances should be refreshed. + * @param chainId - The CAIP-2 chain ID the transaction was confirmed on. + * @param transactionHash - The confirmed transaction's hash, used to match + * against the WebSocket. If undefined, the API is always called. + */ + #refreshAssetsAfterTransactionConfirmed( + account: InternalAccount, + chainId: ChainId, + transactionHash: string | undefined, + ): void { + const forceRefetch = (): void => { + this.getAssets([account], { chainIds: [chainId], forceUpdate: true }).catch( + (error) => { + log('Failed to refresh assets after transaction confirmed', { + error, + }); + }, + ); + }; + + const isChainWsActive = this.#accountActivityDataSource + .getActiveChainsSync() + .includes(chainId); + + if (!transactionHash || !isChainWsActive) { + forceRefetch(); + return; + } + + // The WS may have already confirmed this before we got here. + const key = `${chainId}:${transactionHash.toLowerCase()}`; + if (this.#takeWsConfirmation(key) === 'arrived') { + return; + } + + this.#wsConfirmations.set(key, { + kind: 'pending', + timeout: setTimeout(() => { + this.#wsConfirmations.delete(key); + forceRefetch(); + }, WS_TRANSACTION_CONFIRMATION_TIMEOUT_MS), }); } @@ -4020,6 +4128,12 @@ export class AssetsController extends BaseController< // Stop all active subscriptions this.#stop(); + // Cancel any pending WS/API race timers (see `#wsConfirmations`). + for (const { timeout } of this.#wsConfirmations.values()) { + clearTimeout(timeout); + } + this.#wsConfirmations.clear(); + if (this.#unsubscribeBasicFunctionality) { this.#unsubscribeBasicFunctionality(); this.#unsubscribeBasicFunctionality = null; From c74599db247ab494e3461847b34d38811d546724 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 24 Aug 2026 17:28:13 +0200 Subject: [PATCH 2/4] docs(assets-controller): mark transactionUpdated subscription as breaking in changelog Hosts with restricted messengers (e.g. the mobile app) must delegate AccountActivityService:transactionUpdated for AssetsController to see it, so this is a breaking change to the messenger contract, not just an internal behavior tweak. --- packages/assets-controller/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index b310ca5718..f2de7fe175 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Skip the forced Accounts API balance refetch on `TransactionController:transactionConfirmed` when `AccountActivityService` (WebSocket) already reports the transaction's chain as active: `AssetsController` now waits up to 1.5s for a matching `AccountActivityService:transactionUpdated` event (correlated by transaction hash) before falling back to the API call, avoiding a redundant HTTP request when the WebSocket already delivered the same balance update +- **BREAKING:** Subscribe to `AccountActivityService:transactionUpdated` (typed as `AccountActivityServiceTransactionUpdatedEvent`) to skip the forced Accounts API balance refetch on `TransactionController:transactionConfirmed` when `AccountActivityService` (WebSocket) already reports the transaction's chain as active and confirms the same transaction (correlated by transaction hash) within 1.5s of the confirmation, avoiding a redundant HTTP request when the WebSocket already delivered the same balance update ([#9940](https://github.com/MetaMask/core/pull/9940)) + - Hosts that restrict which events flow through the `AssetsController` messenger must now also delegate `AccountActivityService:transactionUpdated` ### Fixed From 8085d929e7831bbd638949aa2f3f0bd7435d544e Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 24 Aug 2026 18:13:10 +0200 Subject: [PATCH 3/4] docs(assets-controller): tighten WS confirmation comments Replace informal inline comments with clearer, consistent wording. --- .../assets-controller/src/AssetsController.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/assets-controller/src/AssetsController.ts b/packages/assets-controller/src/AssetsController.ts index 0e28fe325d..093b08c59b 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -814,12 +814,14 @@ export class AssetsController extends BaseController< readonly #accountActivityDataSource: AccountActivityDataSource; /** - * Coordinates the race between `transactionConfirmed` and the WS - * `transactionUpdated` push for a given transaction, keyed by - * `${chain}:${txId}`. Whichever arrives first records an entry; whichever - * arrives second consumes it. A `pending` entry means we're about to force - * an Accounts API refetch unless the WS beats the clock; an `arrived` - * entry means the WS already confirmed and is just waiting to be noticed. + * Tracks coordination between `TransactionController:transactionConfirmed` + * and `AccountActivityService:transactionUpdated` for the same transaction, + * keyed by `${chain}:${txId}`. + * + * - `pending`: confirmation arrived first; a timed Accounts API refetch is + * scheduled unless the WebSocket reports the transaction first. + * - `arrived`: the WebSocket reported the transaction first; the entry + * expires if confirmation never arrives. */ readonly #wsConfirmations = new Map< string, @@ -1240,9 +1242,8 @@ export class AssetsController extends BaseController< this.#updateActive(); }); - // Got here first? `transactionConfirmed` hasn't fired yet, so just note - // that the WS confirmed and let it find out later. Got here second? Then - // `transactionConfirmed` is already waiting on a timer - cancel it. + // Cancel a scheduled refetch when the WebSocket confirms first, or record + // that the WebSocket confirmed before `transactionConfirmed` fires. this.messenger.subscribe( 'AccountActivityService:transactionUpdated', ({ chain, id }: WsTransaction) => { @@ -1367,7 +1368,7 @@ export class AssetsController extends BaseController< return; } - // The WS may have already confirmed this before we got here. + // WebSocket confirmation may have arrived before this handler ran. const key = `${chainId}:${transactionHash.toLowerCase()}`; if (this.#takeWsConfirmation(key) === 'arrived') { return; @@ -4128,7 +4129,7 @@ export class AssetsController extends BaseController< // Stop all active subscriptions this.#stop(); - // Cancel any pending WS/API race timers (see `#wsConfirmations`). + // Clear pending `#wsConfirmations` timers. for (const { timeout } of this.#wsConfirmations.values()) { clearTimeout(timeout); } From a516b09c374d948138d95759051228c88a352b99 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 24 Aug 2026 18:39:24 +0200 Subject: [PATCH 4/4] fix: linting --- .../src/AssetsController.test.ts | 10 ++++++++-- .../assets-controller/src/AssetsController.ts | 15 ++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/assets-controller/src/AssetsController.test.ts b/packages/assets-controller/src/AssetsController.test.ts index ce9feb9ee8..9ef3d7b091 100644 --- a/packages/assets-controller/src/AssetsController.test.ts +++ b/packages/assets-controller/src/AssetsController.test.ts @@ -2979,8 +2979,14 @@ describe('AssetsController', () => { hash: '0xdeadbeef', txParams: { from: '0x1234567890123456789012345678901234567890' }, }; - messenger.publish('TransactionController:transactionConfirmed', confirmedEvent); - messenger.publish('TransactionController:transactionConfirmed', confirmedEvent); + messenger.publish( + 'TransactionController:transactionConfirmed', + confirmedEvent, + ); + messenger.publish( + 'TransactionController:transactionConfirmed', + confirmedEvent, + ); await flushPromises(); await jest.advanceTimersByTimeAsync(1_500); diff --git a/packages/assets-controller/src/AssetsController.ts b/packages/assets-controller/src/AssetsController.ts index 093b08c59b..0b64766af7 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -1350,13 +1350,14 @@ export class AssetsController extends BaseController< transactionHash: string | undefined, ): void { const forceRefetch = (): void => { - this.getAssets([account], { chainIds: [chainId], forceUpdate: true }).catch( - (error) => { - log('Failed to refresh assets after transaction confirmed', { - error, - }); - }, - ); + this.getAssets([account], { + chainIds: [chainId], + forceUpdate: true, + }).catch((error) => { + log('Failed to refresh assets after transaction confirmed', { + error, + }); + }); }; const isChainWsActive = this.#accountActivityDataSource