fix(assets-controller): clean accountTree event handlers - #10059
Merged
Conversation
Kriys94
force-pushed
the
fix/AccountTreeAndAssetsController
branch
3 times, most recently
from
September 1, 2026 21:11
fc856d2 to
28e76aa
Compare
Kriys94
force-pushed
the
fix/AccountTreeAndAssetsController
branch
from
September 1, 2026 21:14
28e76aa to
a38f058
Compare
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Contributor
Author
|
Preview builds in extension: MetaMask/metamask-extension#45953 |
Contributor
Author
|
Preview builds in mobile: MetaMask/metamask-mobile#35560 |
salimtb
reviewed
Sep 2, 2026
| * @returns True when the controller has been initialized. | ||
| */ | ||
| isInitialized(): boolean { | ||
| return this.#initialized; |
Contributor
There was a problem hiding this comment.
hhmmm we do this through the messenger now , this is better
salimtb
reviewed
Sep 2, 2026
| */ | ||
| #isUiOpen(): boolean { | ||
| return clientControllerSelectors.selectIsUiOpen( | ||
| this.messenger.call('ClientController:getState'), |
Contributor
There was a problem hiding this comment.
ooh we replace everything with new actions now, could be good to export specific action instead of using getState here
Contributor
Author
There was a problem hiding this comment.
Yeah that's better but minor because uiOpen is the only state of the Controller right now
salimtb
reviewed
Sep 2, 2026
| * `:stateChange` events during `init()` are ignored — startup is driven by | ||
| * `:initialized` instead, so we do not fetch on every tree mutation. | ||
| */ | ||
| #handleAccountTreeStateChange(): void { |
Contributor
There was a problem hiding this comment.
can you quick me help me to understand why you're removing this function ?
Contributor
Author
There was a problem hiding this comment.
Sure, selectedAccountGroupChange should be enough. We were listening stageChange to cover the case when a new non-EVM account was added with Snaps. Now we dont cover this case because there is almost no users that install third party non-EVM Snaps in runtime. And we win simplification of the codebase
ccharly
reviewed
Sep 2, 2026
salimtb
previously approved these changes
Sep 2, 2026
ccharly
approved these changes
Sep 2, 2026
salimtb
approved these changes
Sep 2, 2026
juanmigdr
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Mobile
AccountTreeController:initializedwas triggered at startExtension:
Context / motivation
Mobile and extension do not trigger
AccountTreeController:initializedat the same point in the lifecycle:AccountTreeController.init()at startup viaAccountTreeInitService.initializeAccountTree()(during authentication/login), so:initializedfires early in the app session.init()during controller construction (see FIXME inaccount-tree-controller-init.ts— the keyring must be unlocked first). Instead, it callsAccountTreeController:initlater in#initAccountsAfterUnlock()after unlock.Both clients delegate
AccountTreeController:initializedtoAssetsController, but the event timing differs.AssetsControllercould not reliably gate asset tracking on that event alone — mirroring#accountTreeInitializedfrom it was fragile across clients.This PR adds
AccountTreeController:isInitializedand hasAssetsControllerread lifecycle state on demand so tracking starts/stops consistently regardless of when each client runsinit().Additional cleanup
While making that change, we also removed other sources of drift and complexity:
AssetsControllerpreviously cached#uiOpen,#keyringUnlocked, and#accountTreeInitializedinside event handlers. Those flags could get out of sync if events were missed, reordered, or read before handlers ran. UI open and keyring unlock now useClientController:getStateandKeyringController:isUnlockedon demand as well.AccountTreeController:stateChangesubscription: Used to detect mid-session tree mutations (e.g. a snap account added to the current group), with#handleAccountTreeStateChange,#lastKnownAccountIds, and overlap/diff logic to avoid redundant re-subscriptions duringinit(). That path is removed; explicit group switches stay onselectedAccountGroupChange, and snap accounts added mid-session without a group change are picked up on the next tracking restart.Solution
@metamask/account-tree-controllerAccountTreeController:isInitializedso consumers can query whetherinit()has completed (returnsfalseagain afterclearState()until the next successfulinit()).@metamask/assets-controller#isUiOpen()→ClientController:getState#isKeyringUnlocked()→KeyringController:isUnlocked#isAccountTreeInitialized()→AccountTreeController:isInitialized#handleAccountTreeStateChange/#lastKnownAccountIds.AccountTreeController:stateChange.AccountTreeController:selectedAccountGroupChangefor explicit group switches.AccountTreeController:initialized/:uninitializedto trigger#updateActive()(but gate on authoritative state via the actions above).Trade-off: Snap accounts added mid-session without a group change are no longer picked up live via
:stateChange; they are picked up on the next tracking restart (unlock / UI open cycle).Non-obvious details
#updateActive()still reacts toClientController:stateChange,KeyringController:lock/:unlock, and account-tree init events — but each gate now re-reads authoritative controller state instead of trusting cached flags.#handleAccountGroupChangedstill ignores events while#activeSubscriptions.size === 0, so init-timeselectedAccountGroupChangereplays do not race with:initialized-owned startup.ClientController:stateChanged(not:stateChange) soClientController:getStatereflects UI-open during lifecycle tests.References
app/scripts/messenger-client-init/messengers/assets/assets-controller-messenger.tsapp/core/Engine/messengers/assets-controller/assets-controller-messenger.tsAccountTreeController:initialized/:uninitialized(core#9892)Changelog
@metamask/account-tree-controllerAccountTreeController:isInitializedaction@metamask/assets-controllerAssetsControllerMessengernow requiresAccountTreeController:isInitialized,ClientController:getState, andKeyringController:isUnlockedAccountTreeController:stateChange— remove it from delegated events if listed explicitlyselectedAccountGroupChangefor account-group switches; snap accounts added mid-session are picked up on restartManual testing
Checklist
Note
Medium Risk
Breaking messenger wiring for extension/mobile plus a deliberate behavior change for mid-session account-tree updates; lifecycle gating touches when balances subscribe but not auth or funds logic directly.
Overview
Adds
AccountTreeController:isInitializedso consumers can tell whetherinit()finished (and it flips false again afterclearState()until the nextinit()).AssetsControllerno longer caches UI open, keyring unlock, or account-tree readiness from events.#updateActive()now gates tracking by callingClientController:getState,KeyringController:isUnlocked, andAccountTreeController:isInitializedwhen it runs. It still listens toAccountTreeController:initialized/:uninitializedand client/keyring events, but only to re-evaluate those gates.Breaking for hosts: the assets messenger must delegate the three new actions and can drop
AccountTreeController:stateChangefrom allowed events. Mid-session account-tree mutations (e.g. snap added to the current group without a group switch) no longer trigger live refresh viastateChange;selectedAccountGroupChangestill refreshes on explicit group switches, with snap pickup deferred until the next tracking restart (lock/unlock or UI cycle).Reviewed by Cursor Bugbot for commit fc7cfd3. Bugbot is set up for automated code reviews on this repo. Configure here.