fix: remove ws balance freshness guard#9273
Open
salimtb wants to merge 22 commits into
Open
Conversation
The polling-source freshness lock is unnecessary alongside fetch-then-subscribe and Accounts API cache bypass on forceUpdate.
…iably Improve BackendWebSocketService routing for wildcard channels and stale subscription IDs, harden BackendWebsocketDataSource subscribe/notify flow, and use update-mode force refresh so partial API snapshots do not leave stale token balances in state.
salimtb
changed the title
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 |
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. |
…tyService Wire AssetsController to AccountActivityService:balanceUpdated so unified assetsBalance updates when AccountActivityService owns the websocket subscription. Share account-activity payload conversion with BackendWebsocketDataSource, skip slow RPC for Accounts API chains, and add opt-in WS routing debug logs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…assignment' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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
There was a problem hiding this comment.
3.5K Controller 💀
FUTURE - A lot of then internals can be gutted and moved into separate files (for better testability).
E.g:
- Move pipelines/middleware construction into separate file
- Move state update logic (merge vs update vs full) into a separate file
- Subscription handlers can also be moved into a separate file (you can pass in messenger as inputs to functions).
- Traversing or shape manipulation can also be decoupled.
Force-refresh the sender account via getAssets when a transaction confirms, and harden RpcDataSource post-tx RPC refresh to pass the request and apply update mode when pushing balance snapshots to the controller.
…k switch Subscribe to NetworkController:networkDidChange so switching the selected network re-fetches supported chains, re-subscribes data sources, and force-fetches balances for the newly selected chain.
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 4c62dd9. Configure here.
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. |
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
3 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
UI PR: MetaMask/metamask-extension#43829
What is the current state of things and why does it need to change?
Token balances in
@metamask/assets-controllercould stay stale after transactions, account switches, or WebSocket reconnects. Several independent issues contributed:WebSocket notifications were not reaching
BackendWebsocketDataSource— Clients subscribe on wildcard channels (account-activity.v1.eip155:0:0x…) but the server sends notifications on specific chains (…eip155:42161:0x…).BackendWebSocketServicematched channels exactly, so post-confirm balance pushes were silently dropped even though frames appeared in the Network tab.Stale
subscriptionIdafter reconnect/resubscribe — Notifications could arrive with a server subscription id that no longer matched the local map. The subscription callback was skipped and channel fallback was unreliable.Race conditions on account switch — Overlapping subscribe/unsubscribe and fetch work could interleave, dropping notifications or applying responses out of order.
Stale balances from cached/partial fetches —
AccountsApiDataSourceused TanStack Query’s 60s balance cache even onforceUpdate, and force-refresh pipelines used merge semantics that never removed tokens absent from partial API responses (e.g. USDC still shown after a swap when the API only returned ETH).Websocket freshness guard blocked corrections — A 120s guard prevented polling/API updates from overwriting recent WS balances. That helped in some send scenarios but also blocked legitimate corrections (e.g. receiver account showing wrong balance after account switch).
What is the solution your changes offer and how does it work?
@metamask/core-backend—BackendWebSocketServiceeip155:0:0xaddrmatcheseip155:42161:0xaddrfor both subscription callbacks and channel callbacks.subscriptionIdlookup fails, resolve the handler by matching the notification channel against registered subscription channels.channel/subscriptionIdfrom nesteddatawhen the server wraps notifications.@metamask/assets-controller—BackendWebsocketDataSourceaddChannelCallbackper channel as a fallback when subscription-id routing fails.onAssetsUpdatefrom stored subscription state when notifications arrive with stale ids.@metamask/assets-controller—AssetsController#accountRefreshMutexserializes overlapping refresh work.@metamask/assets-controller— balance update modesupdatemode: patches balance amounts only; does not remove tokens or overwrite metadata/prices. Used forgetAssets({ forceUpdate: true })and Accounts API / Snap fetch responses when the API returns a partial snapshot (e.g. only ETH after a swap).mergeremains the default for event-driven updates (WS, polling).fullremains for responses that should be authoritative for a chain scope.AccountsApiDataSource{ staleTime: 0, gcTime: 0 }to the API client whenrequest.forceUpdateis true.End-to-end flow after a confirmed swap:
Note
Medium Risk
Touches live balance state, WebSocket routing, and a breaking messenger event requirement; incorrect merge or routing could show wrong balances but changes are well-tested.
Overview
Fixes stale token balances after transactions, account/network switches, and WebSocket reconnects by improving how real-time and polled balance updates reach unified
assetsBalancestate.AssetsControllerremoves the 120-second websocket freshness guard andsourceId-based filtering so API/RPC/polling can overwrite recent WS values again. It subscribes toAccountActivityService:balanceUpdated(breaking: messenger must allow this event) andTransactionController:transactionConfirmedfor force-refreshes, handlesNetworkController:networkDidChangeby refreshing data-sourceactiveChains, re-subscribing, and force-fetching the selected EVM chain, and migrates several subscriptions to*:stateChangedevents. Force-update pipelines usemergewithoutsourceId; the slow Snap/RPC path runs only for chains the fast Accounts API path did not already handle successfully.BackendWebSocketServiceadds account-activity wildcard channel matching (eip155:0↔ specific chain), subscription lookup by channel whensubscriptionIdis stale, nested notification normalization, and safer routing when subscriptions lack callbacks.BackendWebsocketDataSourceregisters handlers before the subscribe handshake, cleans up on subscribe failure, and sharesprocessAccountActivityBalanceUpdateswith the controller path.AccountsApiDataSource/RpcDataSourceexposerefreshActiveChainsfor network switches.Reviewed by Cursor Bugbot for commit 5332676. Bugbot is set up for automated code reviews on this repo. Configure here.