feat(assets-controller): clean and optimize data fetching#7950
feat(assets-controller): clean and optimize data fetching#7950
Conversation
94338fd to
789b774
Compare
| }; | ||
| const fallbackRequests = partitionChainsBySource( | ||
| fallbackRequest, | ||
| sources, |
There was a problem hiding this comment.
Fallback retries the same failed source
Medium Severity
The fallback round calls partitionChainsBySource(fallbackRequest, sources) with the same sources array used in round 1. Since partitionChainsBySource assigns each chain to the first source that supports it, the source that originally failed gets the same chains again in the fallback. The stated intent — letting lower-priority sources try failed chains — is not achieved because the partitioning logic is identical both rounds.
| merged.detectedAssets = { | ||
| ...(merged.detectedAssets ?? {}), | ||
| ...response.detectedAssets, | ||
| }; |
There was a problem hiding this comment.
Shallow merge of detectedAssets loses per-account arrays
Low Severity
mergeDataResponses uses a shallow object spread for detectedAssets, which replaces per-account arrays rather than concatenating them. If two responses contain detectedAssets for the same accountId, the first response's array is silently discarded. This is inconsistent with how assetsBalance is correctly deep-merged per account (lines 26–35). Since detectedAssets is Record<AccountId, Caip19AssetId[]>, the per-account arrays need to be concatenated (with deduplication), not replaced.
| */ | ||
| #subscribeAssets(): void { | ||
| if (this.#selectedAccounts.length === 0) { | ||
| if (this.#selectedAccounts.length === 0 || this.#enabledChains.size === 0) { |
There was a problem hiding this comment.
handleBasicFunctionalityChange bypasses new UI+keyring lifecycle guard
Low Severity
handleBasicFunctionalityChange calls #stop() then #subscribeAssets() directly, bypassing the new #updateActive() guard that requires both #uiOpen and #keyringUnlocked to be true. This creates subscriptions even when the controller should be inactive according to the new combined lifecycle model. It could call #updateActive() instead to respect the new invariant.


Explanation
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?
Parallel middlewares (
@metamask/assets-controller)createParallelBalanceMiddleware: Runs balance data sources (Accounts API, Snap, RPC) in parallel with chain partitioning and limited concurrency (p-limit, concurrency 3). Failed chains get a fallback round. Responses are merged viamergeDataResponses.createParallelMiddleware: Runs TokenDataSource and PriceDataSource in parallel for the same request and merges their responses.Both use the new
DataResponse.updateModeso merged results preserve'full'when any source uses it.Full vs merge update modes
DataResponsegets an optionalupdateMode:'full'|'merge'(typeAssetsUpdateMode).updateModeis omitted is'merge'.ClientController integration
@metamask/assets-controlleradds a dependency on@metamask/client-controllerand subscribes toClientController:stateChange.Method-action-types script
scripts/generate-method-action-types.tsno longer takes a per-package path. It discovers all controllers withMESSENGER_EXPOSED_METHODSunderpackages/. Thegenerate-method-action-typesscript andtsxdevDependency are removed from individual packages (e.g. assets-controller, client-controller, analytics-controller, etc.) so generation is centralized.Are there any changes whose purpose might not be obvious to those unfamiliar with the domain?
mergeDataResponses: Merges multipleDataResponseobjects (e.g. from parallel balance sources). If any response hasupdateMode: 'full', the merged result is'full'; otherwise it defaults to'merge'.ClientController:getStateandClientController:stateChangeon the assets-controller messenger; otherwise asset tracking will not run when the UI is open.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?
scripts/generate-method-action-types.tsaffects the whole monorepo, so all packages that previously had a localgenerate-method-action-typesscript and used it with a path were updated to drop that script and thetsxdevDependency.assets-controllersCHANGELOG was updated to reflect dependency/version bumps (e.g.network-enablement-controller) that are part of this work.If you had to upgrade a dependency, why did you do so?
@metamask/client-controller: New dependency for assets-controller to gate asset tracking on UI visibility.p-limit: Added to assets-controller to cap concurrency in the parallel balance middleware.tsxfrom several packages: script execution is centralized; the root or a single package can run the generator.Integration PR in extension: MetaMask/metamask-extension#40233
References
Checklist
Note
Medium Risk
Touches core balance/price aggregation and start/stop lifecycle behavior; concurrency and new
updateModesemantics could cause missing/overwritten state if edge cases aren’t covered.Overview
Optimizes assets data fetching and state application. The
AssetsControllernow runs balance sources in parallel with chain partitioning and a fallback retry, and runs token metadata + price enrichment in parallel via newParallelMiddlewareutilities (with concurrency limits viap-limit).Adds
DataResponse.updateMode('full' | 'merge') and updates state writes to respect it (initial/force fetch is treated as full while most subscription updates are merge, preserving custom assets during full refreshes). Asset tracking is now gated by both UI-open and keyring-unlocked state via@metamask/client-controller, and tests/data sources were updated accordingly (including early returns when no accounts/chains).Written by Cursor Bugbot for commit 7e0e148. This will update automatically on new commits. Configure here.