Repository navigation
fix(assets-controllers): report failed native/staked balance reads as unsuccessful in RpcBalanceFetcher - #10671
Open
Kshot3000 wants to merge 2 commits into
Conversation
… unsuccessful in RpcBalanceFetcher RpcBalanceFetcher hard-coded success: true with a zero value for native and staked entries whose balance was missing from the multicall result. A missing entry can only mean the read failed (successful reads record an entry, including an explicit BN(0) for a genuine zero), so a failed read was indistinguishable from a confirmed zero balance and overwrote the previously known balance with 0x0 via TokenBalancesController and AccountTrackerController. - Native and staked entries with no recorded balance are now reported with success: false and no value, matching the ERC-20 branch and the AccountsApiBalanceFetcher failure shape. - getStakedBalancesForAddresses now records an explicit BN(0) for addresses whose shares were read successfully and are zero, so a genuine zero staked balance stays distinguishable from a failed read. - Tests: new/updated regression coverage in the fetcher and multicall suites; TokenBalancesController mocks updated to the explicit-zero contract (three failure-simulation tests now assert no fabricated zero entries); oxlint suppression counts refreshed for the fetcher test file. Fixes MetaMask#10043
This branch has not been deployed
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
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:
RpcBalanceFetcher(packages/assets-controllers) builds native and stakedProcessedBalanceentries for every account it was asked about, hard-codingsuccess: trueand substitutingnew BN('0')whenever the multicall result has no entry for that account/token.That conflates two different situations. As #10043 documents, the multicall layer records a native entry only for a successful read — a genuine on-chain zero comes back as an explicit
BN(0)— and the fallback paths behave the same way. A missing entry can therefore only mean the read failed. Reporting it as a successful zero makesTokenBalancesControllerforward it toAccountTrackerController.updateNativeBalances, which persistsbalance = '0x0': one failed RPC read erases the user's previously known balance. The staked branch has the same flaw, with an extra twist:getStakedBalancesForAddressesdropped successful zero-share reads entirely, so "zero shares" and "read failed" were indistinguishable at that layer too.Solution:
rpc-balance-fetcher.ts: a native or staked entry with no recorded balance is now reported as{ success: false }with no value — the same shape the ERC-20 branch in the same function already produces for a null balance, and the same shapeAccountsApiBalanceFetcheruses for failed per-account fetches.TokenBalancesControlleralready skips unsuccessful entries in all three of its consumers (token balances state, native updates, staked updates), so a failed read now leaves the previously known balance untouched, while a genuine zero (an explicitBN(0)entry) still flows through as a successful zero.multicall.ts:getStakedBalancesForAddressesnow records an explicitBN(0)for every address whose shares call succeeded with zero shares. Only failed reads (failed shares call, failedconvertToAssets, or a thrown error) leave an address out, which is what makes the fetcher's rule sound. The staked fallback (getStakedBalancesFallback) already recorded an entry per successful read, so both paths now share one contract: entry present ⟺ read succeeded.Test-suite changes (please read before judging the diff size):
TokenBalancesControllertests drive the realRpcBalanceFetcherwith mockedgetTokenBalancesForMultipleAddressesresults that omitted native/staked entries while expecting the synthesized zero entries in state. Those mocks now return explicit zero entries where the scenario is a healthy read of a zero balance (their expectations are unchanged). Three tests that simulate failed reads instead had their expectations updated to assert that no zero entries are fabricated: "does not update balances when … multi-account contract failed", "sets balance to 0 … when balance fetcher fails completely" (tracked ERC-20 tokens are still seeded to zero by the controller; native/staking entries are no longer added), and "should handle missing staked balances gracefully". One event test was renamed to match its new premise (a returned zero staked balance).Verification (all local, per AGENTS.md TDD):
@metamask/assets-controllerssuite: 45/45 suites, 1810/1810 tests pass.tsc --build packages/assets-controllers/tsconfig.build.json: clean.oxlinton all touched files: 0 errors. The new fetcher tests use the same patterns as the existing ones in that file (e.g. calls onjest.requireMockresults), so the file's counts inoxlint-suppressions.jsonwere refreshed (17→24 id-length, 13→16 no-unsafe-call, 21→24 no-unsafe-member-access).oxfmt --checkon touched files: clean. Changelog entry added under[Unreleased]→Fixed.This PR was prepared with AI assistance; all changes were verified by running the test suites, type build, and linters above.
References
Checklist
@metamask/assets-controllersCHANGELOG[Unreleased]→FixedNote
Medium Risk
Changes balance-fetch semantics for native and staked assets, which affects what users see when RPC reads fail; behavior is intentionally stricter and should preserve prior balances instead of clearing them.
Overview
Fixes a bug where failed native or staked RPC/multicall reads were treated as successful zero balances, which could wipe a user’s last known balance in the wallet.
RpcBalanceFetchernow reports missing native or staked entries as{ success: false }(no value), aligned with ERC-20 handling and explicitBN(0)for real zeros.getStakedBalancesForAddressesnow includes an explicit zero for each address whose shares read succeeded with zero shares, so “no entry” means failure only.Tests and changelog are updated: mocks that meant “healthy zero” return explicit zeros; scenarios that simulate failed reads no longer expect fabricated native/staking zeros in
TokenBalancesControllerstate.Reviewed by Cursor Bugbot for commit 3c6d3a1. Bugbot is set up for automated code reviews on this repo. Configure here.