-
-
Notifications
You must be signed in to change notification settings - Fork 266
feat: Convert asset balances to human-readable format #7752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/assets-controller/src/data-sources/evm-rpc-services/types/state.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
| accountAddress: Address, | ||
| options?: BalanceFetchOptions, | ||
| ): Promise<BalanceFetchResult> { | ||
| const tokens = this.getTokensToFetch(chainId, accountAddress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User-imported tokens never fetched on RPC-only chains
High Severity
The refactored getTokensToFetch() now reads from assetsBalance to determine which ERC20 tokens to fetch, but this creates a circular dependency. User-imported tokens aren't in assetsBalance until their balance is fetched, but they can't be fetched because they're not in assetsBalance. Additionally, RpcDataSource.fetch() always passes an empty array to fetchBalancesForTokens(), only fetching native token balance and ignoring request.customAssets. This breaks balance fetching for user-imported tokens on RPC-only chains.
Additional Locations (1)
| options?: BalanceFetchOptions, | ||
| ): Promise<BalanceFetchResult> { | ||
| const tokens = this.getTokensToFetch(chainId, accountAddress); | ||
| const tokens = this.getTokensToFetch(chainId, accountId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused formattedBalance computed by BalanceFetcher
Low Severity
BalanceFetcher computes formattedBalance for every balance result (line 286) and includes it in the returned object (line 300). However, RpcDataSource.#handleBalanceUpdate completely ignores formattedBalance and instead uses balance.balance (the raw value) to recompute the human-readable format via #convertToHumanReadable. This wastes computation and creates misleading code where a field is populated but never consumed.
Additional Locations (1)
| ); | ||
|
|
||
| newBalances[balance.assetId] = { | ||
| amount: humanReadableAmount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated detection result processing code in RpcDataSource
Low Severity
The code for building metadata from detected assets and converting detected balances to human-readable format is duplicated nearly identically in two places within RpcDataSource: in #handleDetectionUpdate (lines 530-565) and in detectTokens (lines 1048-1076). Both blocks iterate over result.detectedAssets to build metadata objects and over result.detectedBalances to convert balances using #convertToHumanReadable. This pattern could be extracted into a shared helper method.
Additional Locations (1)
Prithpal-Sooriya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
Lets just keep in mind that if people want to use the string decimals, they need to be careful with BigNumber and >15 decimals to avoid this error:
new BigNumber() number type has more than 15 significant digits
Example sentry log:
https://metamask.sentry.io/issues/7082247853/events/8f9c90d2a5e24c22956de16441731605/?project=273505
Explanation
Current State
The
AssetsControllerwas storing raw balance amounts (e.g.,"39779499997060000"for 0.0398 ETH) in theassetsBalancestate. This made it difficult for consumers to display human-readable values without additional conversion logic. Additionally, native token metadata (symbol, name, decimals) was not being stored inassetsMetadata, which is required for proper balance formatting and display.Solution
This PR updates all data sources to:
assetsMetadatawithdecimals: 18derived fromNetworkControllerchain statusChanges by Data Source:
RpcDataSource:
NetworkControllerchain status (symbol, name, decimals: 18)assetsMetadatastate or falls back toTokenListControllerBigNumberJSbefore returningAccountsApiDataSource:
assetsMetadatain the response and merges it in the middlewareBackendWebsocketDataSource:
asset.decimalsfrom the websocket messageExample Result
After:
References
Checklist
[ ] I've updated the test suite for new or updated code as appropriate
[ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
[ ] I've communicated my changes to consumers by updating changelogs for packages I've changed
[ ] I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them
Note
Converts all asset balances to human-readable amounts and propagates token metadata alongside balance updates.
balance.amountin human-readable format; fiat is computed directly asbalance.amount * price. README and CHANGELOG updatedRpcDataSourceconverts raw balances using decimals, includesassetsMetadata(native fromNetworkController, ERC20 from state or token list), and merges via middlewareBalanceFetcherandTokenDetectornow use minimal messenger adapters, return human-readable balances, and no longer rely on user token lists; tests updated accordinglyassetsMetadata; subscription updates restart polling; disabled-chain balances are retained for historical displayWritten by Cursor Bugbot for commit b8b139c. This will update automatically on new commits. Configure here.