diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 69e72da088..f2de7fe175 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **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 - 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..9ef3d7b091 100644 --- a/packages/assets-controller/src/AssetsController.test.ts +++ b/packages/assets-controller/src/AssetsController.test.ts @@ -2853,6 +2853,228 @@ 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..0b64766af7 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,21 @@ export class AssetsController extends BaseController< readonly #accountActivityDataSource: AccountActivityDataSource; + /** + * 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, + { kind: 'pending' | 'arrived'; timeout: NodeJS.Timeout } + >(); + readonly #accountsApiDataSource: AccountsApiDataSource; readonly #snapDataSource: SnapDataSource; @@ -1216,6 +1241,41 @@ export class AssetsController extends BaseController< this.#accountTreeInitialized = false; this.#updateActive(); }); + + // 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) => { + 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 +1326,61 @@ 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; + } + + // WebSocket confirmation may have arrived before this handler ran. + 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 +4130,12 @@ export class AssetsController extends BaseController< // Stop all active subscriptions this.#stop(); + // Clear pending `#wsConfirmations` timers. + for (const { timeout } of this.#wsConfirmations.values()) { + clearTimeout(timeout); + } + this.#wsConfirmations.clear(); + if (this.#unsubscribeBasicFunctionality) { this.#unsubscribeBasicFunctionality(); this.#unsubscribeBasicFunctionality = null;