feat: use aggregate3 or accountAPI to fetch balances #6232
Conversation
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
from
9d24a01 to
28235f7
Compare
salimtb
changed the title
salimtb
changed the title
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
2 times, most recently
from
1ca38cc to
a830bd8
Compare
salimtb
added
team-assets
area-performance
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (includeStakedAssets) { | ||
| await this.refreshStakedBalances(networkClientIds); | ||
| } | ||
| if (includeNativeAssets) { | ||
| await this.refreshNativeBalances(networkClientIds); | ||
| } |
There was a problem hiding this comment.
Nice separation here. Only caveat is that both of these have independent state updates. Meaning we could end up with multiple state updates (if we need to update staked assets then native assets).
This is a minor nit, we can clean up any additional state updates in a future design.
There was a problem hiding this comment.
(discuss) why are we including changes in the AccountTrackerController? I thought the new design would have everything in the TokenBalancesController?
| * @param includeStaked - Whether to include staked balances | ||
| * @returns Map of token address to map of user address to balance | ||
| */ | ||
| const processBalanceResults = ( |
There was a problem hiding this comment.
Guessing these changes are the same as this PR:
#6212
| chainId: ChainIdHex; | ||
| }; | ||
|
|
||
| export type BalanceFetcher = { |
There was a problem hiding this comment.
Is this meant to be a shared interface between different BalanceFetchers?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
2 times, most recently
from
89a94d5 to
cd0d817
Compare
| expect(refreshSpy).toHaveBeenCalledTimes(5); | ||
| expect(refreshSpy).toHaveBeenNthCalledWith(3, [networkClientId1]); | ||
| expect(refreshSpy).toHaveBeenNthCalledWith(3, [networkClientId1]); | ||
| expect(refreshSpy).toHaveBeenCalledTimes(3); |
There was a problem hiding this comment.
Bug: Duplicate Spy Call Verification
A test expects refreshSpy.toHaveBeenNthCalledWith(3, [networkClientId1]) twice consecutively. This is logically impossible as a spy call can only occur once for a given call number, causing the test to fail. The second expectation should likely target a different call number or arguments.
| account: acct as ChecksumAddress, | ||
| token: checksum(tokenAddr), | ||
| chainId, | ||
| }); |
There was a problem hiding this comment.
Bug: Null Balance Handling Fails
The RpcBalanceFetcher's fetch method incorrectly casts null balance values to BN type. While the success flag correctly reflects a null balance, the value property is unconditionally cast as BN. This creates ProcessedBalance objects where success is false but value is null (typed as BN), violating the value?: BN type definition and potentially causing runtime errors.
| } | ||
| if (includeNativeAssets) { | ||
| await this.refreshNativeBalances(networkClientIds); | ||
| } |
There was a problem hiding this comment.
Bug: Balance Refresh Flags Required
The _executePoll method in AccountTrackerController no longer updates account balances by default. It now conditionally calls refreshStakedBalances and refreshNativeBalances only if includeStakedAssets or includeNativeAssets flags are explicitly set to true in AccountTrackerPollingInput. As these flags are optional, existing callers that do not specify them will result in undefined values, causing no balance updates to occur. This is a breaking change from the previous behavior where _executePoll always refreshed all balances.
There was a problem hiding this comment.
Bug: Multicall Contract Missing on Fraxtal Chain
The multicall contract address for Fraxtal (chain ID 0xfc / 252) was unintentionally removed from the MULTICALL_CONTRACT_BY_CHAINID map. This breaks multicall functionality for Fraxtal users, forcing them to rely on less efficient individual RPC calls.
packages/assets-controllers/src/multicall.ts#L200-L201
core/packages/assets-controllers/src/multicall.ts
Lines 200 to 201 in 01db552
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
from
01db552 to
8fbdbfa
Compare
There was a problem hiding this comment.
Bug: Fraxtal Network Multicall Support Broken
The entry for Fraxtal (chain ID 0xfc) was unintentionally removed from the MULTICALL_CONTRACT_BY_CHAINID map. This breaks multicall support for the Fraxtal network, forcing it to fall back to less efficient individual RPC calls. The removal appears accidental, as noted by a PR reviewer's comment "was this meant to be nuked?".
packages/assets-controllers/src/multicall.ts#L200-L201
core/packages/assets-controllers/src/multicall.ts
Lines 200 to 201 in 8fbdbfa
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
from
8fbdbfa to
4830bdb
Compare
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
from
4830bdb to
23561b1
Compare
There was a problem hiding this comment.
Bug: Multicall Contract Missing for Fraxtal Mainnet
The multicall contract address for Fraxtal Mainnet (chain ID 0xfc / 252) was accidentally removed from MULTICALL_CONTRACT_BY_CHAINID. This omission, questioned by a PR reviewer, disables multicall functionality for Fraxtal, causing getTokenBalancesForMultipleAddresses to fall back to less efficient individual RPC calls.
packages/assets-controllers/src/multicall.ts#L200-L201
core/packages/assets-controllers/src/multicall.ts
Lines 200 to 201 in 23561b1
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
salimtb
force-pushed
the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
from
23561b1 to
494b74f
Compare
salimtb
deleted the
feat/use-aggregate3-on-balances-state-fetch-account-api
branch
|
@metamaskbot publish-preview |
You're absolutely right! Let me update that section to be more precise about the fallback strategy:
Explanation
Current State and Problem
Previously, the TokenBalancesController fetched token balances using individual RPC calls for each token-account combination. This approach was inefficient and slow, especially for users with multiple accounts and many tokens across different networks. Each balance check required a separate
balanceOfcall, leading to:Solution
This PR implements a two-tier balance fetching strategy with intelligent fallback:
aggregate3function for efficient batch RPC calls when:Key Technical Changes
Smart Fallback Architecture
TokenBalancesControllerfirst attempts to fetch balances via the Accounts API for supported networksaggregate3Accounts API Integration
SUPPORTED_NETWORKS_ACCOUNTS_API_V4)Aggregate3 Fallback Implementation
When the Accounts API can't handle certain networks, the system uses Multicall3's
aggregate3function to:balanceOfcalls into a single RPC requestPerformance Optimizations
Technical Details
The fallback mechanism ensures comprehensive network coverage:
aggregate3(still much faster than individual calls)The
aggregate3function provides significant improvements over individual RPC calls:allowFailure: true)Impact
References
Addresses performance issues with token balance fetching by implementing Accounts API integration with aggregate3 RPC fallback for comprehensive and efficient balance retrieval across all networks.
Checklist