Skip to content

Commit e88bcf2

Browse files
authored
fix/account-tracker-prevent-empty-state-update (#5942)
## Explanation This change prevents AccountTrackerController from overwriting cached balances with empty or unchanged state during refresh operations. Currently, if all balance requests for a chain fail or return undefined (e.g., due to a temporary network issue), the controller updates the state with an empty object, erasing previous data. Additionally, even when the new balance data is identical to the current state, an update is still triggered, resulting in unnecessary stateChange emissions and potential UI re-renders. This update introduces guards within the refresh method to: - Skip updates if the computed accountsForChain object is empty. - Skip updates if the resulting balances are identical to the current state. - Prevent stale or partial data from overriding valid cached values. - Reduce unnecessary re-renders and state emissions. These changes help improve performance and stability, particularly under poor network conditions or when balance polling is frequent. <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References * Fixes [#15472](MetaMask/metamask-mobile#15472) <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- THIS SECTION IS NO LONGER NEEDED. The process for updating changelogs has changed. Please consult the "Updating changelogs" section of the Contributing doc for more. --> ### `@metamask/assets-controllers` UPDATE: Prevent AccountTrackerController from updating state with empty or unchanged data during balance refresh. Adds checks to avoid overwriting valid cached balances and reduce unnecessary stateChange emissions. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent e7f75c1 commit e88bcf2

File tree

2 files changed

+54
-19
lines changed

2 files changed

+54
-19
lines changed

packages/assets-controllers/CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- Prevented `AccountTrackerController` from updating state with empty or unchanged account balance data during refresh ([#5942](https://github.com/MetaMask/core/pull/5942))
13+
- Added guards to skip state updates when fetched balances are empty or identical to existing state
14+
- Reduces unnecessary `stateChange` emissions and preserves previously-cached balances under network failure scenarios
15+
1016
## [68.1.0]
1117

1218
### Added

packages/assets-controllers/src/AccountTrackerController.ts

Lines changed: 48 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import { StaticIntervalPollingController } from '@metamask/polling-controller';
2424
import type { PreferencesControllerGetStateAction } from '@metamask/preferences-controller';
2525
import { assert } from '@metamask/utils';
2626
import { Mutex } from 'async-mutex';
27-
import { cloneDeep } from 'lodash';
27+
import { cloneDeep, isEqual } from 'lodash';
2828

2929
import type {
3030
AssetsContractController,
@@ -193,13 +193,18 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
193193

194194
this.messagingSystem.subscribe(
195195
'AccountsController:selectedEvmAccountChange',
196-
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
197-
// eslint-disable-next-line @typescript-eslint/no-misused-promises
198-
() => this.refresh(this.#getNetworkClientIds()),
196+
(newAddress, prevAddress) => {
197+
if (newAddress !== prevAddress) {
198+
// Making an async call for this new event
199+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
200+
this.refresh(this.#getNetworkClientIds());
201+
}
202+
},
203+
(event): string => event.address,
199204
);
200205
}
201206

202-
private syncAccounts(newChainId: string) {
207+
private syncAccounts(newChainIds: string[]) {
203208
const accountsByChainId = cloneDeep(this.state.accountsByChainId);
204209
const { selectedNetworkClientId } = this.messagingSystem.call(
205210
'NetworkController:getState',
@@ -213,12 +218,15 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
213218

214219
const existing = Object.keys(accountsByChainId?.[currentChainId] ?? {});
215220

216-
if (!accountsByChainId[newChainId]) {
217-
accountsByChainId[newChainId] = {};
218-
existing.forEach((address) => {
219-
accountsByChainId[newChainId][address] = { balance: '0x0' };
220-
});
221-
}
221+
// Initialize new chain IDs if they don't exist
222+
newChainIds.forEach((newChainId) => {
223+
if (!accountsByChainId[newChainId]) {
224+
accountsByChainId[newChainId] = {};
225+
existing.forEach((address) => {
226+
accountsByChainId[newChainId][address] = { balance: '0x0' };
227+
});
228+
}
229+
});
222230

223231
// Note: The address from the preferences controller are checksummed
224232
// The addresses from the accounts controller are lowercased
@@ -249,9 +257,11 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
249257
});
250258
});
251259

252-
this.update((state) => {
253-
state.accountsByChainId = accountsByChainId;
254-
});
260+
if (!isEqual(this.state.accountsByChainId, accountsByChainId)) {
261+
this.update((state) => {
262+
state.accountsByChainId = accountsByChainId;
263+
});
264+
}
255265
}
256266

257267
/**
@@ -327,11 +337,17 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
327337
);
328338
const releaseLock = await this.#refreshMutex.acquire();
329339
try {
340+
const chainIds = networkClientIds.map((networkClientId) => {
341+
const { chainId } = this.#getCorrectNetworkClient(networkClientId);
342+
return chainId;
343+
});
344+
345+
this.syncAccounts(chainIds);
346+
330347
// Create an array of promises for each networkClientId
331348
const updatePromises = networkClientIds.map(async (networkClientId) => {
332349
const { chainId, ethQuery } =
333350
this.#getCorrectNetworkClient(networkClientId);
334-
this.syncAccounts(chainId);
335351
const { accountsByChainId } = this.state;
336352
const { isMultiAccountBalancesEnabled } = this.messagingSystem.call(
337353
'PreferencesController:getState',
@@ -394,15 +410,28 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
394410
// Wait for all networkClientId updates to settle in parallel
395411
const allResults = await Promise.allSettled(updatePromises);
396412

397-
// Update the state once all networkClientId updates are completed
413+
// Build a _copy_ of the current state and track whether anything changed
414+
const nextAccountsByChainId: AccountTrackerControllerState['accountsByChainId'] =
415+
cloneDeep(this.state.accountsByChainId);
416+
let hasChanges = false;
417+
398418
allResults.forEach((result) => {
399419
if (result.status === 'fulfilled') {
400420
const { chainId, accountsForChain } = result.value;
401-
this.update((state) => {
402-
state.accountsByChainId[chainId] = accountsForChain;
403-
});
421+
// Only mark as changed if the incoming data differs
422+
if (!isEqual(nextAccountsByChainId[chainId], accountsForChain)) {
423+
nextAccountsByChainId[chainId] = accountsForChain;
424+
hasChanges = true;
425+
}
404426
}
405427
});
428+
429+
// 👇🏻 call `update` only when something is new / different
430+
if (hasChanges) {
431+
this.update((state) => {
432+
state.accountsByChainId = nextAccountsByChainId;
433+
});
434+
}
406435
} finally {
407436
releaseLock();
408437
}

0 commit comments

Comments
 (0)