From d2f5dffe96c93db4d2f163077f5a1f5210d76d6c Mon Sep 17 00:00:00 2001 From: Kriys94 Date: Mon, 31 Aug 2026 16:56:47 +0200 Subject: [PATCH] fix(assets-controller): avoid race between ws message and AccountsAPI call --- packages/assets-controller/CHANGELOG.md | 1 + .../src/AssetsController.test.ts | 55 +++++++++++++++++++ .../assets-controller/src/AssetsController.ts | 52 +++++++----------- 3 files changed, 77 insertions(+), 31 deletions(-) diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 87faf66c63..c713058d1b 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Clean up spam assets on keyring unlock, gated behind the `assetsUnifyState` remote feature flag's `useUnlockCleanup` property (disabled unless the flag explicitly enables it) ([#9973](https://github.com/MetaMask/core/pull/9973)) +- Skip transaction-driven force `getAssets` refreshes on chains where AccountActivity already provides live WebSocket balance updates, avoiding a race with the Accounts API that could overwrite correct balances ([#10030](https://github.com/MetaMask/core/pull/10030)) ## [14.0.2] diff --git a/packages/assets-controller/src/AssetsController.test.ts b/packages/assets-controller/src/AssetsController.test.ts index febeb0d73b..59e80e4442 100644 --- a/packages/assets-controller/src/AssetsController.test.ts +++ b/packages/assets-controller/src/AssetsController.test.ts @@ -2853,6 +2853,61 @@ describe('AssetsController', () => { }); }); + it('force refreshes assets when unapproved transaction is added', async () => { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + messenger.publish('TransactionController:unapprovedTransactionAdded', { + chainId: '0xa4b1', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + + expect(getAssetsSpy).toHaveBeenCalledWith( + [expect.objectContaining({ id: MOCK_ACCOUNT_ID })], + { + chainIds: ['eip155:42161'], + forceUpdate: true, + }, + ); + + getAssetsSpy.mockRestore(); + }); + }); + + it('does not force refresh assets on transaction events for AccountActivity-active chains', async () => { + await withController(async ({ controller, messenger }) => { + const getAssetsSpy = jest + .spyOn(controller, 'getAssets') + .mockResolvedValue({}); + + messenger.publish('AccountActivityService:statusChanged', { + chainIds: ['eip155:42161'], + status: 'up', + }); + + await flushPromises(); + + messenger.publish('TransactionController:unapprovedTransactionAdded', { + chainId: '0xa4b1', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + messenger.publish('TransactionController:transactionConfirmed', { + chainId: '0xa4b1', + txParams: { from: '0x1234567890123456789012345678901234567890' }, + }); + + await flushPromises(); + + expect(getAssetsSpy).not.toHaveBeenCalled(); + + 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 6cc89ac5df..8754c27d3d 100644 --- a/packages/assets-controller/src/AssetsController.ts +++ b/packages/assets-controller/src/AssetsController.ts @@ -1196,22 +1196,24 @@ export class AssetsController extends BaseController< this.#updateActive(); }); - // Subscribe to unapproved transactions - TXs that need confirmation - // Ensures that balances for the account making transaction are updated (e.g. for gas estimations) + // Subscribe to unapproved transactions - TXs that need confirmation. + // Ensures balances for the account making the transaction are updated + // (e.g. for gas estimations), except on AccountActivity-active chains. this.messenger.subscribe( 'TransactionController:unapprovedTransactionAdded', (transactionMeta: TransactionMeta) => { - this.#onUnapprovedTransactionAdded(transactionMeta); + this.#refreshAssetsForTransaction(transactionMeta); }, ); // Post-tx refresh via the full fetch pipeline (Accounts API + RPC fallback). + // Skipped for chains covered by AccountActivity (real-time WS updates). // RpcDataSource also listens for transactionConfirmed, but only refreshes // chains it owns via an active subscription. this.messenger.subscribe( 'TransactionController:transactionConfirmed', (transactionMeta: TransactionMeta) => { - this.#onTransactionConfirmed(transactionMeta); + this.#refreshAssetsForTransaction(transactionMeta); }, ); // Start tracking only after the account tree is fully built. Unlock can @@ -1227,42 +1229,30 @@ export class AssetsController extends BaseController< }); } - #onUnapprovedTransactionAdded(transactionMeta: TransactionMeta): void { + /** + * Force-refresh assets for the account/chain of a transaction, unless the + * chain is already covered by AccountActivity (real-time WebSocket balances). + * + * @param transactionMeta - The transaction that triggered the refresh. + */ + #refreshAssetsForTransaction(transactionMeta: TransactionMeta): void { const hexChainId = transactionMeta.chainId; if (!hexChainId) { return; } const caipChainId = `eip155:${parseInt(hexChainId, 16)}` as ChainId; - const fromAddress = transactionMeta.txParams.from?.toLowerCase(); - if (!fromAddress) { - return; - } - - const matchedAccount = this.#getSelectedAccounts().find( - (account) => account.address.toLowerCase() === fromAddress, - ); - if (!matchedAccount) { - return; - } - this.getAssets([matchedAccount], { - chainIds: [caipChainId], - forceUpdate: true, - }).catch((error) => { - log('Failed to refresh assets after unapproved transaction added', { - error, - }); - }); - } - - #onTransactionConfirmed(transactionMeta: TransactionMeta): void { - const hexChainId = transactionMeta.chainId; - if (!hexChainId) { + // AccountActivity pushes live balance updates for its active chains; a + // force getAssets would be redundant and can race the WebSocket path. + if ( + this.#accountActivityDataSource + .getActiveChainsSync() + .includes(caipChainId) + ) { return; } - const caipChainId = `eip155:${parseInt(hexChainId, 16)}` as ChainId; const fromAddress = transactionMeta.txParams.from?.toLowerCase(); if (!fromAddress) { return; @@ -1279,7 +1269,7 @@ export class AssetsController extends BaseController< chainIds: [caipChainId], forceUpdate: true, }).catch((error) => { - log('Failed to refresh assets after transaction confirmed', { error }); + log('Failed to refresh assets after transaction event', { error }); }); }