fix: improve assets controller snap accounts subscription and websocket#8430
Draft
fix: improve assets controller snap accounts subscription and websocket#8430
Conversation
…ountTree initializes late - Convert `#selectedAccounts` getter to `#getSelectedAccounts()` method with a fallback to `AccountsController:getSelectedAccount` when the AccountTree returns empty (covers the window before the tree is initialized) - Add `#handleAccountTreeStateChange()`: when `AccountTreeController:stateChange` fires and subscriptions already exist (created before snap accounts arrived), re-subscribe and re-fetch so snap/BTC/SOL accounts are included - Add `AccountsControllerGetSelectedAccountAction` to `AllowedActions` and `@metamask/accounts-controller` as a dependency to support the fallback - Add mock for `AccountsController:getSelectedAccount` in test helper Previously, data sources reported their active chains before the AccountTree was initialized, causing subscriptions to be created with only the fallback EVM account. When the tree later fired `stateChange` with 4 accounts (EVM + snap accounts), `#start()` returned early due to its idempotency guard, so snap accounts were never subscribed or fetched.
…okenDataSource Custom assets (user-imported tokens in `customAssets` state) now bypass both the EVM occurrence-count filter and the non-EVM Blockaid bulk scan. Only auto-detected tokens are subject to spam filtering.
…n websocket is disconnected or disabled BackendWebsocketDataSource now only claims chains when the websocket is actually connected. On disconnect, chains are released so the chain-claiming loop assigns them to AccountsApiDataSource for polling. On reconnect, chains are reclaimed from the stored supportedChains list.
salimtb
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
salimtb
changed the title
packages/assets-controller/src/data-sources/BackendWebsocketDataSource.ts
Outdated
Show resolved
Hide resolved
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
|
@metamaskbot publish-preview |
packages/assets-controller/src/data-sources/BackendWebsocketDataSource.ts
Show resolved
Hide resolved
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
|
@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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d5ac722. Configure here.
| this.updateActiveChains([], (updatedChains) => | ||
| this.#onActiveChainsUpdated(this.getName(), updatedChains, previous), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Disconnect handler creates subscriptions while controller is stopped
High Severity
When the WebSocket disconnects after #stop() has already been called (e.g., because the keyring is locked), #handleDisconnect calls updateActiveChains([], callback) and fires #onActiveChainsUpdated. This triggers #handleActiveChainsUpdate → #subscribeAssets(), which has no shouldRun guard, creating new entries in AssetsController.#activeSubscriptions even though the controller was intentionally stopped. When the user later unlocks the wallet, #start() sees #activeSubscriptions.size > 0 and returns early — bypassing the force-refresh getAssets() call, #ensureNativeBalancesDefaultZero(), and #lastKnownAccountIds initialization. Before this PR, #handleDisconnect did not call updateActiveChains, so this code path did not exist.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d5ac722. Configure here.
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
1 participant
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
Current state
AssetsControllerhad three related issues that prevented balances from loading correctly in certain scenarios:Snap accounts not subscribed after startup: On app launch,
#start()was called beforeAccountTreeController.init()had finished building the account tree, so snap accounts weren't yet in the selected account group. Because#start()is idempotent (it returns early when subscriptions already exist), snap accounts were never picked up when they became available, leaving Solana, Tron, and Bitcoin balances missing.Custom assets filtered by spam heuristics:
TokenDataSourceapplied theMIN_TOKEN_OCCURRENCESthreshold (EVM ERC-20) and Blockaid bulk scan (non-EVM fungible tokens) to all tokens uniformly. This meant tokens manually imported by the user and stored incustomAssetsstate were incorrectly filtered out as potential spam.No polling fallback when WebSocket is disabled:
BackendWebsocketDataSourceandAccountsApiDataSourceboth call the samefetchV2SupportedNetworksAPI to determine which chains they support. BecauseBackendWebsocketDataSourcehas higher priority in the chain-claiming loop, it would claim all supported chains at initialization — even when the WebSocket never connected. This leftAccountsApiDataSourcewith no chains to poll, so users with the WebSocket disabled or with a broken connection received no balance updates.Solution
Re-subscribe on AccountTree state change: Added
#handleAccountTreeStateChange()which subscribes toAccountTreeController:stateChange. When the account tree updates (e.g. after snap accounts are hydrated), the method forces a full re-subscription and re-fetch with the complete account list. AddedAccountsController:getSelectedAccountas a fallback in#getSelectedAccounts()for cases where the account tree is not yet initialized.Custom assets bypass spam filters:
TokenDataSourcenow readscustomAssetsfrom state and builds acustomAssetIdsset. Tokens in that set skip theoccurrences >= 3EVM filter and are excluded from the non-EVM Blockaid scan input, ensuring user-imported assets always appear.Connection-aware chain claiming in BackendWebsocketDataSource: Added an
#isConnectedflag (initiallyfalse).#initializeActiveChainsand#refreshActiveChainsnow store supported chains in#supportedChainsbut only callupdateActiveChainswhen connected. On disconnect,activeChainsis cleared so the chain-claiming loop re-assigns those chains toAccountsApiDataSourcefor polling. On reconnect, chains are restored from#supportedChainsbefore pending WebSocket subscriptions are reprocessed.References
Checklist
Note
Medium Risk
Changes asset subscription/chain-claiming behavior and adds onboarding-based RPC suppression, which can affect when and how balances update across multiple chains and account types.
Overview
Fixes multi-chain balance tracking edge cases by making
AssetsControllerre-subscribe/re-fetch when the account tree later gains accounts (e.g. snap accounts), usingAccountsController:getSelectedAccountas a startup fallback and tracking#lastKnownAccountIdsto avoid redundant work.Improves data-source coordination:
BackendWebsocketDataSourcenow only claims chains while connected, releases them on disconnect soAccountsApiDataSourcecan poll as fallback, and reclaims on reconnect (dropping stale pending subscriptions).TokenDataSourcenow exempts user-importedcustomAssetsfrom occurrence/Blockaid spam filtering.Adds an optional
isOnboardedhook plumbed intoRpcDataSourcesofetch/subscribebecome no-ops before onboarding completes, preventing premature on-chain RPC calls; tests, changelog, and dependency graph are updated accordingly.Reviewed by Cursor Bugbot for commit d5ac722. Bugbot is set up for automated code reviews on this repo. Configure here.