Skip to content

Commit 30c8a38

Browse files
authored
fix: should not update balances when app is locked (#7492)
## Explanation ### Current state and why it needs to change The `AccountTrackerController` previously had inconsistent lock/unlock handling compared to `TokenBalancesController`: 1. **No lock state tracking**: The controller had no way to track whether the keyring was locked, meaning it could attempt balance fetches even when the wallet was locked. 2. **Refresh on unlock**: When `KeyringController:unlock` was fired, it would immediately trigger a full balance refresh for all networks. This behavior was unnecessary since polling continues regardless of lock state. 3. **Refresh all networks on network add**: When a new network was added via `NetworkController:networkAdded`, the controller would refresh balances for **all** networks instead of just the newly added one. ### Solution This PR aligns `AccountTrackerController` with the pattern already established in `TokenBalancesController`: 1. **Added `#isLocked` property**: Tracks whether the keyring is locked, initialized from `KeyringController:getState` in the constructor. 2. **Added `isActive` getter**: Returns `true` when the keyring is unlocked AND the user is onboarded. Balance fetching is skipped when `isActive` is `false`. 3. **Subscribe to `KeyringController:lock`**: Sets `#isLocked = true` when the keyring is locked. 4. **Updated `KeyringController:unlock` subscription**: Now only sets `#isLocked = false` without triggering an immediate refresh. Polling continues as normal and will fetch balances on the next interval. 5. **Optimized `NetworkController:networkAdded` handler**: Now extracts the `networkClientId` from the added network configuration and only refreshes balances for that specific network. ### Breaking changes - `AccountTrackerController` messenger must now allow `KeyringController:getState` action - `AccountTrackerController` messenger must now allow `KeyringController:lock` event ## References - Similar pattern exists in `TokenBalancesController` (`#isUnlocked` property and `isActive` getter) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] 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) - [x] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > AccountTrackerController now tracks keyring lock state to gate balance updates, adds isActive, refreshes only the newly added network on networkAdded, and requires KeyringController:getState and :lock. > > - **AccountTrackerController** > - **Lock state & activity gating**: > - Add `#isLocked` (initialized via `KeyringController:getState`) and `isActive` getter; skip balance updates when inactive (locked or not onboarded). > - Subscribe to `KeyringController:lock`/`unlock` to update lock state; unlock no longer triggers immediate refresh. > - **Network addition behavior**: > - On `NetworkController:networkAdded`, refresh balances only for the newly added network’s `networkClientId`. > - **Messenger contract (BREAKING)**: > - Requires `KeyringController:getState` action and `KeyringController:lock` event in allowed actions/events. > - **Tests & docs**: > - Update tests to publish full `NetworkConfiguration` and assert `isActive` transitions. > - Update `CHANGELOG.md` to document breaking changes and behavior tweaks. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 6c2de6a. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 023d8c0 commit 30c8a38

File tree

3 files changed

+78
-32
lines changed

3 files changed

+78
-32
lines changed

packages/assets-controllers/CHANGELOG.md

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

1212
- Export `selectAllAssets` ([#7496](https://github.com/MetaMask/core/pull/7496))
1313

14+
### Changed
15+
16+
- **BREAKING:** `AccountTrackerController` now requires `KeyringController:getState` action and `KeyringController:lock` event in addition to existing allowed actions and events ([#7492](https://github.com/MetaMask/core/pull/7492))
17+
- Added `#isLocked` property to track keyring lock state, initialized from `KeyringController:getState`
18+
- Added `isActive` getter that returns `true` when keyring is unlocked and user is onboarded
19+
- Balance updates are now skipped when controller is not active (locked or not onboarded)
20+
- `KeyringController:unlock` event now only updates lock state without triggering immediate refresh
21+
- `KeyringController:lock` event sets controller to inactive state
22+
- `AccountTrackerController` now only refreshes balances for the newly added network on `NetworkController:networkAdded` event instead of all networks ([#7492](https://github.com/MetaMask/core/pull/7492))
23+
1424
## [94.1.0]
1525

1626
### Added

packages/assets-controllers/src/AccountTrackerController.test.ts

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -388,10 +388,14 @@ describe('AccountTrackerController', () => {
388388
stakedBalances: {},
389389
});
390390

391-
messenger.publish(
392-
'NetworkController:networkAdded',
393-
{} as NetworkConfiguration,
394-
);
391+
messenger.publish('NetworkController:networkAdded', {
392+
chainId: '0x1',
393+
blockExplorerUrls: [],
394+
name: 'Mainnet',
395+
nativeCurrency: 'ETH',
396+
defaultRpcEndpointIndex: 0,
397+
rpcEndpoints: [{ networkClientId: 'mainnet' }],
398+
} as unknown as NetworkConfiguration);
395399

396400
await clock.tickAsync(1);
397401

@@ -402,29 +406,23 @@ describe('AccountTrackerController', () => {
402406
);
403407
});
404408

405-
it('refreshes addresses when keyring is unlocked', async () => {
409+
it('sets isActive to true when keyring is unlocked', async () => {
406410
await withController(
407411
{
408412
selectedAccount: ACCOUNT_1,
409413
listAccounts: [ACCOUNT_1],
410414
},
411415
async ({ controller, messenger }) => {
412-
mockedGetTokenBalancesForMultipleAddresses.mockResolvedValueOnce({
413-
tokenBalances: {
414-
'0x0000000000000000000000000000000000000000': {
415-
[ADDRESS_1]: new BN('abcdef', 16),
416-
},
417-
},
418-
stakedBalances: {},
419-
});
416+
// Verify controller is active initially (unlocked by default in tests)
417+
expect(controller.isActive).toBe(true);
420418

421-
messenger.publish('KeyringController:unlock');
422-
423-
await clock.tickAsync(1);
419+
// Lock the keyring
420+
messenger.publish('KeyringController:lock');
421+
expect(controller.isActive).toBe(false);
424422

425-
expect(
426-
controller.state.accountsByChainId['0x1'][CHECKSUM_ADDRESS_1].balance,
427-
).toBe('0xabcdef');
423+
// Unlock the keyring
424+
messenger.publish('KeyringController:unlock');
425+
expect(controller.isActive).toBe(true);
428426
},
429427
);
430428
});
@@ -2033,6 +2031,11 @@ async function withController<ReturnValue>(
20332031
mockNetworkState,
20342032
);
20352033

2034+
messenger.registerActionHandler(
2035+
'KeyringController:getState',
2036+
jest.fn().mockReturnValue({ isUnlocked: true }),
2037+
);
2038+
20362039
const accountTrackerMessenger = new Messenger<
20372040
'AccountTrackerController',
20382041
AllAccountTrackerControllerActions,
@@ -2050,12 +2053,14 @@ async function withController<ReturnValue>(
20502053
'PreferencesController:getState',
20512054
'AccountsController:getSelectedAccount',
20522055
'AccountsController:listAccounts',
2056+
'KeyringController:getState',
20532057
],
20542058
events: [
20552059
'AccountsController:selectedEvmAccountChange',
20562060
'TransactionController:unapprovedTransactionAdded',
20572061
'TransactionController:transactionConfirmed',
20582062
'NetworkController:networkAdded',
2063+
'KeyringController:lock',
20592064
'KeyringController:unlock',
20602065
],
20612066
});

packages/assets-controllers/src/AccountTrackerController.ts

Lines changed: 44 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@ import {
1515
toChecksumHexAddress,
1616
} from '@metamask/controller-utils';
1717
import EthQuery from '@metamask/eth-query';
18-
import type { KeyringControllerUnlockEvent } from '@metamask/keyring-controller';
18+
import type {
19+
KeyringControllerGetStateAction,
20+
KeyringControllerLockEvent,
21+
KeyringControllerUnlockEvent,
22+
} from '@metamask/keyring-controller';
1923
import type { InternalAccount } from '@metamask/keyring-internal-api';
2024
import type { Messenger } from '@metamask/messenger';
2125
import type {
@@ -192,7 +196,8 @@ export type AllowedActions =
192196
}
193197
| AccountsControllerGetSelectedAccountAction
194198
| NetworkControllerGetStateAction
195-
| NetworkControllerGetNetworkClientByIdAction;
199+
| NetworkControllerGetNetworkClientByIdAction
200+
| KeyringControllerGetStateAction;
196201

197202
/**
198203
* The event that {@link AccountTrackerController} can emit.
@@ -217,6 +222,7 @@ export type AllowedEvents =
217222
| TransactionControllerUnapprovedTransactionAddedEvent
218223
| TransactionControllerTransactionConfirmedEvent
219224
| NetworkControllerNetworkAddedEvent
225+
| KeyringControllerLockEvent
220226
| KeyringControllerUnlockEvent;
221227

222228
/**
@@ -256,6 +262,9 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
256262

257263
readonly #isOnboarded: () => boolean;
258264

265+
/** Track if the keyring is locked */
266+
#isLocked = true;
267+
259268
/**
260269
* Creates an AccountTracker instance.
261270
*
@@ -331,6 +340,9 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
331340
this.#fetchingEnabled = fetchingEnabled;
332341
this.#isOnboarded = isOnboarded;
333342

343+
const { isUnlocked } = this.messenger.call('KeyringController:getState');
344+
this.#isLocked = !isUnlocked;
345+
334346
this.setIntervalLength(interval);
335347

336348
this.messenger.subscribe(
@@ -345,16 +357,25 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
345357
(event): string => event.address,
346358
);
347359

348-
this.messenger.subscribe('NetworkController:networkAdded', () => {
349-
this.refresh(this.#getNetworkClientIds()).catch(() => {
350-
// Silently handle refresh errors
351-
});
352-
});
360+
this.messenger.subscribe(
361+
'NetworkController:networkAdded',
362+
(networkConfiguration) => {
363+
const { networkClientId } =
364+
networkConfiguration.rpcEndpoints[
365+
networkConfiguration.defaultRpcEndpointIndex
366+
];
367+
this.refresh([networkClientId]).catch(() => {
368+
// Silently handle refresh errors
369+
});
370+
},
371+
);
353372

354373
this.messenger.subscribe('KeyringController:unlock', () => {
355-
this.refresh(this.#getNetworkClientIds()).catch(() => {
356-
// Silently handle refresh errors
357-
});
374+
this.#isLocked = false;
375+
});
376+
377+
this.messenger.subscribe('KeyringController:lock', () => {
378+
this.#isLocked = true;
358379
});
359380

360381
this.messenger.subscribe(
@@ -392,6 +413,16 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
392413
this.#registerMessageHandlers();
393414
}
394415

416+
/**
417+
* Whether the controller is active (keyring is unlocked and user is onboarded).
418+
* When locked or not onboarded, balance updates should be skipped.
419+
*
420+
* @returns Whether the controller should perform balance updates.
421+
*/
422+
get isActive(): boolean {
423+
return !this.#isLocked && this.#isOnboarded();
424+
}
425+
395426
#syncAccounts(newChainIds: string[]): void {
396427
const accountsByChainId = cloneDeep(this.state.accountsByChainId);
397428
const { selectedNetworkClientId } = this.messenger.call(
@@ -645,7 +676,7 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
645676

646677
this.#syncAccounts(chainIds);
647678

648-
if (!this.#fetchingEnabled() || !this.#isOnboarded()) {
679+
if (!this.#fetchingEnabled() || !this.isActive) {
649680
return;
650681
}
651682

@@ -815,8 +846,8 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
815846
): Promise<
816847
Record<string, { balance: string; stakedBalance?: StakedBalance }>
817848
> {
818-
// Skip balance fetching if not onboarded to avoid unnecessary RPC calls during onboarding
819-
if (!this.#isOnboarded()) {
849+
// Skip balance fetching if locked or not onboarded to avoid unnecessary RPC calls
850+
if (!this.isActive) {
820851
return {};
821852
}
822853

0 commit comments

Comments
 (0)