feat(balances): migrate account balances to the v2 indexer endpoint - #935
feat(balances): migrate account balances to the v2 indexer endpoint#935aristidesstaffieri wants to merge 14 commits into
Conversation
Adds a v2 balances path behind the new use_balances_v2 remote-config flag
(defaults to on; Amplitude can flip it off to roll back to v1 without a
release). Ports the browser extension's feat/balances-v2-migration work,
adapted to mobile's zustand/axios architecture.
- fetchBalancesV2 POSTs to freighter-backend-v2 /accounts/balances and
routes there on PUBLIC/TESTNET when the flag is on; Futurenet stays on v1
- mapAccountBalancesV2 normalizes the snake_case v2 wire shape into the
app's BalanceMap: native -> "XLM", classic/SAC -> "CODE:ISSUER" (SAC kept
classic-shaped so display doesn't double-scale the pre-formatted amount),
SEP-41 -> Soroban shape with raw i128 + decimals, LP -> "<poolId>:lp"
- addBlockaidScanResults stamps benign defaults and merges mainnet bulk
scan verdicts client-side via scanBulkTokens, matching the v1 backend's
server-side payload; scan failures never break balances
- v2 needs no contract_ids: the wallet-backend returns all indexed
holdings (trustlines, SACs, SEP-41s, pool shares) automatically
|
iOS Simulator preview build is ready: https://github.com/stellar/freighter-mobile/releases/tag/untagged-d6062dd951af5aacfa4c (SDF collaborators only — install instructions in the release description) |
…ployed The v2 /accounts/balances endpoint currently returns 500 on prod and staging for every network, so defaulting the flag to on broke balance fetching (and the send/swap e2e suites) wherever remote config falls back to code defaults. Default to the v1 path and let Amplitude flip the flag on once the wallet-backend indexer is live.
There was a problem hiding this comment.
Pull request overview
Migrates balance fetching to the indexed v2 backend behind a remotely controlled rollout flag.
Changes:
- Adds v2 balance routing and response normalization.
- Adds client-side Blockaid scanning for v2 balances.
- Adds remote-config integration and unit coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/backend.ts |
Routes supported networks to v2. |
src/helpers/mapAccountBalancesV2.ts |
Maps v2 balance wire types. |
src/helpers/addBlockaidScanResults.ts |
Adds Blockaid verdicts. |
src/ducks/remoteConfig.ts |
Defines the rollout flag. |
src/ducks/balances.ts |
Passes the runtime flag. |
__tests__/services/backend.test.ts |
Tests endpoint routing. |
__tests__/helpers/mapAccountBalancesV2.test.ts |
Tests balance mapping. |
__tests__/helpers/addBlockaidScanResults.test.ts |
Tests scan merging. |
__tests__/ducks/remoteConfig.test.ts |
Tests flag behavior. |
__tests__/ducks/balances.test.ts |
Updates store expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The v2 backend contract returns one entry per requested address — unfunded accounts arrive as a matching entry with is_funded: false, never as an omission. Mapping an absent entry to "unfunded" masked malformed/partial responses and could replace a funded user's balances with the unfunded UI. Throw instead, so the store's error path keeps the existing balances, and tighten mapAccountBalancesV2 to require an account entry.
Pre-merge review — balances v2 migrationReviewed against a whole-system checklist (invariants, flag state machine, replaced-behavior parity, error paths, test fidelity) and cross-checked against the sibling extension PR (freighter#2906). This is a faithful, well-tested port — and on two design calls it's the safer of the two platforms (flag defaults OFF, and missing-account is rejected rather than rendered as unfunded — nice). The blockers below are mostly shared with the extension (both clients copied the same wire assumption, so checking the two clients against each other looks clean — the bug is against the backend). Verdict: safe to merge; must not enable the flag until #1 and #2 are resolved. 1. SEP-41 token balances are double-scaled by 1e7 — CRITICAL (shared with extension)CRITICAL — merge blocker for enabling the flag. Every SEP-41 token balance would render 10,000,000× too small, and spendable/send math is wrong the same way. TL;DR: The mapper treats the SEP-41 Steps to reproduce:
Detailed explanation (for agents)Root cause: wallet-backend's GraphQL resolver serializes the SEP-41 balance with The client mapper treats it as raw +
Also note Caveat / confirm before acting: freighter-backend-v2's own comment says "raw i128", contradicting its resolver. The static evidence (String128 ÷1e7) is unambiguous, but please capture one real v2 response for a SEP-41 holder and add it as a recorded golden fixture before flipping the flag — it settles the contradiction and would have caught this. Suggested fixes:
2. Flag ON against a failing v2 = no balances, no fallback — HIGH (shared with extension)HIGH — not a merge blocker (default-OFF protects you today), but a rollout landmine. TL;DR: Defaulting the flag OFF is the right call and keeps users safe on v1 until the backend is ready — good. But there's no fallback: the moment the flag is flipped ON while the endpoint is still returning 500 (per the PR body, it 500s on prod + staging today), every pubnet/testnet user gets an error state with no balances, recoverable only by flipping the flag back and waiting for the next config poll / restart. Detailed explanation (for agents)Root cause: Suggested fix: fall back to v1 on v2 5xx/network error (response shapes are identical by design, so it's a small change), or stage the rollout on a small segment. Decide this symmetrically with the extension — a fallback on only one platform is itself a divergence. If fail-closed is deliberate, note it in the PR body. 3. User-added custom tokens can silently disappear on the v2 path — HIGH (shared with extension)HIGH — not a merge blocker; needs an explicit decision before rollout. TL;DR: The comment says indexed contract tokens "come back automatically", but v2 only returns SEP-41 balances the indexer has already ingested for that holder. A token the user explicitly added (v1 queried it directly) vanishes from the list, with no error, if the indexer hasn't ingested that contract/holder pair yet. Detailed explanation (for agents)Root cause: the v2 path sends no Suggested fix: verify indexer coverage for the add-token flow before rollout, or diff 4. Client-side bulk scan runs uncached on every 30s poll and blocks balance render — MEDIUMMEDIUM — mobile-specific; the right caching chokepoint already exists in the repo. TL;DR: Detailed explanation (for agents)Root cause: Suggested fixes:
5. Freshly-funded account can render as "unfunded" under indexer lag — MEDIUMMEDIUM — not client-fixable; flag it as a known/accepted rollout risk. TL;DR: v2 6. The flag-ON duck wiring and the 500 path are untested — MEDIUMMEDIUM — test gap on the one new load-bearing line. TL;DR: Tests only ever assert Detailed explanation (for agents)The load-bearing line with no red-on-break coverage: 7. SAC balances would be 1e7-inflated — MEDIUM (latent; shared with extension)MEDIUM — latent, not currently reachable. Mirror image of #1. TL;DR: SAC amounts arrive raw (DB passthrough, not ÷1e7), but the mapper treats them as pre-formatted decimals, so they'd display 1e7× too large. Unreachable today because SAC balances key on a Detailed explanation (for agents)Root cause: LOW / NIT — additional findings
Verified solid (coverage)
🤖 Generated with Claude Code (Fable) — cross-platform pre-merge review, verified against wallet-backend + freighter-backend-v2 source. |
This is incorrect, wallet backend already scales the value during ingestion.
|
Same concern as on the extension - so is the suggestion that in the case of v2 failures that we try v1? I don't think we intend to rely on v1 for much longer and will aim to deprecate it so my gut reaction is that we should treat v2 as a critical dependency and ensure reliability there. |
This is incorrect, by design v2 relies on the wallet backend to index contract tokens. |
addBlockaidScanResults called scanBulkTokens raw on every 30s balance poll, adding a sequential HTTP round-trip before balances render and firing a BLOCKAID_BULK_TOKEN_SCAN analytics event per poll on mainnet. Use the blockaidTokenScans duck's scanBulkWithCache instead (disk-backed, 30-min TTL, per-token) — the same chokepoint the swap flow uses, with a compatible CODE-ISSUER key format so both surfaces share cache entries. Warm polls become a local read with no network call and no analytics event; real scans drop to one per token per 30 minutes. Tests now mock the raw scan API + storage underneath the real duck so the cache path is exercised, with a new case pinning that fresh cache entries produce zero scanBulkTokens calls.
This is fixed by 27f88e7 |
Im not sure what we can do about this, the clients rely on the indexer as the source of truth here and the SLO is that it is caught up to the tip at all times. |
Review feedback flagged that the duck tests only ever asserted useV2: false — a hardcoded value at the fetchBalances call site would have passed the suite. Add a test that flips use_balances_v2 on after render and asserts the duck passes useV2: true, pinning the read-at-call-time behavior. Reset the flag in beforeEach so it can't leak between tests. Also pin that a v2 server error propagates without silently falling back to v1, which would mask indexer outages behind v1 data.
this is fixed by 5846c4a |
This is incorrect for the same reason as issue 1 is |
freighter-backend-v2#138 renamed `balance` to `total` and added
server-derived `key` and `token` fields to every balance entry, so the
mapper no longer derives asset identity client-side.
- mapAccountBalancesV2.ts: rename `balance` → `total` in the wire types;
add `V2Token`/`V2TokenIssuer` and `key`/`token` to the base, narrowed
per variant (trustline type verbatim on CLASSIC, no `type` on SEP-41,
no token on LP entries)
- mapper: pass server `key`/`token` through verbatim and drop
`classicAssetType`; the native entry alone is still re-keyed from the
server's "native" to "XLM", the app convention
- tests: fixtures updated to the new wire shape; the code-length type
derivation test is now a key/token pass-through assertion; backend
routing fixture carries `key`/`token`/`total`
Native XLM max-send is overstated for accounts with open sell offersMEDIUM — not a merge blocker. Pre-existing logic (not introduced by this PR), but worth fixing here since the sibling extension PR (freighter#2906) already handles this correctly, and this PR is touching the balance-mapping plumbing. TL;DR: When computing spendable XLM, the app subtracts the base reserve and the fee but not the XLM locked by the account's open sell offers (selling liabilities). So for an account with active sell offers, "send max" offers more XLM than is actually spendable, and the transaction fails at submit. Steps to reproduce:
Detailed explanation (for agents)Root cause: the native branch of
And the v2 mapper passes the bare base reserve through, without folding liabilities in:
How the extension does it (the pattern to mirror): it keys off
Why Suggested fix (mirror the extension, v1-safe):
|
The v1 balances endpoint took the user's locally saved custom-token contract
IDs as `contract_ids` hints and returned a contract-token balance only for an
ID it was handed. The v2 endpoint takes account addresses alone and lets the
wallet-backend indexer decide what comes back, and the migration chose to
trust it. That is wrong for zero-balance tokens: the indexer only knows about
tokens an account holds a balance for, so a SEP-41 token added through Add
Token with no balance never comes back and silently disappears. It also broke
the "already added" state, which hasExistingTrustline infers from balances
alone — so the token showed as not added and could be added repeatedly.
Merge the local list back in client-side. injectLocalTokenBalances runs in
fetchBalancesV2 between the mapper and the Blockaid scan and adds an entry for
every local contract ID the response omits, resolving metadata and the real
balance through getTokenDetails. The duck already reads the local list for the
v1 path, so the contract IDs are simply threaded through rather than re-read
from storage. Merging before the scan means locally added tokens get Blockaid
verdicts, as they did on v1. Skipped for unfunded accounts, where the
not-funded UI renders instead of balances.
Dedupe takes two checks. Matching on token_id covers SAC and SEP41 entries,
whose token_id is the contract, but a CLASSIC balance's token_id is the
CODE:ISSUER asset string — so a locally added SAC already on screen as a
trustline is caught by resolving the SAC's name to CODE:ISSUER and skipping if
that key exists. This needed a stricter SAC check than the existing
isSacContract, which only verifies that a name parses as CODE:ISSUER and so
would dedupe away any SEP-41 token that happens to be named that way. The new
isSacContractForAsset derives the SAC address and compares it to the contract
in hand; the loose helper and its history-mapper callers are untouched.
Removal is now narrower than before by design: the result carries
localOnlyTokenIds, and SimpleBalancesList offers removal only for those.
Once the backend returns a token on its own, dropping the local entry would
not stop it coming back, so it is hide-only — a new CannotRemoveType variant
explains that. Classic trustline removal is unchanged, since the gate only
fires for CUSTOM_TOKEN, and the v1 path reports its full local list, so
behavior with the flag off is untouched.
Also wires shouldFetchBalance through getTokenDetails. It was already declared
on GetTokenDetailsParams but never forwarded as should_fetch_balance, so every
merged token would have rendered as zero. Left off by default: the balance
costs the backend an extra contract call that other callers do not need.
Blockaid verdicts are dropped for tokens with a hyphen in the symbol (fails open to "Benign")Medium — reachable on demand by anyone who deploys a SEP-41 token, and it fails open rather than closed. Your call on merge-blocking; the fix is a few lines. TL;DR: We ask Blockaid about tokens using one naming format and match the answers back using another, and the conversion back is ambiguous when a token's symbol contains a hyphen. For those tokens the scan result is silently thrown away — and because every token is pre-stamped "Benign" before scanning, a token Blockaid flagged as malicious will display as safe. A scammer can trigger this deliberately by putting a hyphen in their token's symbol. Steps to reproduce:
The same token with a hyphen-free symbol badges correctly. Detailed explanation (for agents)Root cause: Outbound — correct: freighter-mobile/src/helpers/addBlockaidScanResults.ts Lines 62 to 67 in f4520c4 Inbound — eats the wrong hyphen, so the lookup misses and the verdict is discarded by the freighter-mobile/src/helpers/addBlockaidScanResults.ts Lines 69 to 74 in f4520c4 Traced concretely: Why it fails open rather than closed: every entry is stamped with the benign default before the scan runs, so a dropped verdict is not a missing badge — it is an affirmative "Benign" claim. freighter-mobile/src/helpers/addBlockaidScanResults.ts Lines 33 to 42 in f4520c4 Why it's attacker-reachable: classic asset codes are alphanumeric, so they can't hit this. But the Soroban key is freighter-mobile/src/helpers/mapAccountBalancesV2.ts Lines 221 to 231 in f4520c4 So suppressing your own spam/scam badge costs one character in a token name. Deterministic repro: call Suggested fixes (in increasing order of depth):
Note on provenance: the v1 path has the same first-occurrence pattern, so this is inherited rather than newly introduced. It's worth fixing here anyway — v2 is new code, and the pre-stamped benign default makes the failure mode a false assurance rather than an absent one. Cross-platform: the extension has the identical defect on its own v2 path, at |
|
Spendable XLM was computed as total - reserve - fee, ignoring selling
liabilities. Accounts with an open XLM sell offer were offered a "max"
larger than they could actually send, and the transaction failed at
submit with tx_insufficient_balance / op_underfunded.
Two changes, both required:
- mapAccountBalancesV2: the v2 server reports `minimum_balance` as the
pure base reserve and keeps selling liabilities as a separate
subtrahend. Fold them together so the mapped `minimumBalance` matches
the v1 contract, which already folds both.
- calculateSpendableAmount: use the server-derived `minimumBalance`
instead of recomputing (2 + subentryCount) * BASE_RESERVE locally.
Both balance paths now agree on its meaning, so this is correct on v1
and v2 without touching v1. The local formula stays as a fallback when
the field is absent, keeping a reserve-safe floor.
Keying off `minimumBalance` rather than `available` is deliberate:
`available` is defined inconsistently across paths (v1 native does not
subtract the reserve), and this helper is shared by both.
Side effect worth noting: the server value also reflects sponsorship
netting (num_sponsoring / num_sponsored) that the local formula ignored,
so sponsoring accounts will see a lower max and sponsored accounts a
higher one.
Two isAmountSpendable fixtures declared minimumBalance: 1 while passing
subentryCount: 3 (a 2.5 reserve). The field was inert before this change
so nothing caught the contradiction; made coherent, assertions unchanged.
Outbound, a balance-map key `<symbol>:<contractId>` was converted to the
Blockaid id `<symbol>-<contractId>`. Inbound, the id was converted back
with `replace("-", ":")`, which replaces only the first occurrence. Once
the symbol itself contains a hyphen the wrong one is consumed, the
lookup misses, and the verdict is discarded.
This fails open, not closed: every entry is pre-stamped with a benign
default before the scan runs, so a dropped verdict renders as an
affirmative "Benign" rather than an absent badge. A SEP-41 `symbol()`
returns an unconstrained String and the v2 backend builds the key as
`symbol + ":" + tokenID`, so suppressing your own spam/scam badge costs
one character in a token name. Classic asset codes are alphanumeric and
cannot reach it.
Rather than correcting the split, drop the round-trip: record which
balance key produced each Blockaid id when building the address list and
look the verdict up by that map. `scanBulkWithCache` keys its results by
exactly the strings it is given, on both the cache and network paths, so
the mapping is correct by construction and there is no parsing rule left
to drift.
Also log any returned asset id that matches no balance. A discarded
verdict was previously indistinguishable from a clean scan, which is why
this could regress unnoticed.
The v2 balances contract-violation error interpolated the account's public key into the thrown message. That message becomes the Sentry issue title verbatim, which bypasses sanitizeLogData: the redaction walk is key-based and its Error branch returns `message` untouched. The key shipped to Sentry regardless of the user's analytics opt-in, breaking the promise PII_FIELDS_LOWER is there to keep. It also broke grouping — a unique title per account meant one Sentry issue per affected user instead of one issue with N events, leaving the error un-alertable during exactly the rollout it was meant to signal. Drop the key from the message and pass it through the logApiError extras instead, where the redactor can reach it. Mirrors the existing pattern in walletKitUtil's disconnectAllSessions, which documents this same failure mode. The message also flows into the balances store's `error` state, but that value is only read as a truthy flag — both render sites print the static `balancesList.error` translation — so there is no UI exposure to fix.
this is fixed in d654485 |
the mapping has changed in e9d51e6 to not assume any format |
thanks, this is fixed in d0c3654 |
What
Migrates account balance fetching to the freighter-backend-v2 indexer endpoint (
POST /accounts/balances), behind a newuse_balances_v2remote-config flag. This ports the browser extension'sfeat/balances-v2-migrationwork, adapted to mobile's zustand/axios architecture.fetchBalancesV2POSTs to freighter-backend-v2/accounts/balancesand is used on PUBLIC/TESTNET when the flag is on; Futurenet stays on v1.mapAccountBalancesV2normalizes the snake_case v2 wire shape into the app'sBalanceMap:"XLM""CODE:ISSUER"(SAC kept classic-shaped so display doesn't double-scale the pre-formatted amount)"<poolId>:lp"addBlockaidScanResultsstamps benign defaults and merges mainnet bulk scan verdicts client-side viascanBulkTokens, matching the payload the v1 backend builds server-side. Scan failures never break balances.contract_idsparam: the wallet-backend returns all indexed holdings (trustlines, SACs, SEP-41s, pool shares) automatically.Why
The v1 account-balances endpoint is being replaced by the wallet-backend indexer, which serves balances from indexed data instead of querying Horizon/RPC per request. Gating the switch behind remote config lets us roll it out (and roll it back) without shipping a new app version.
Release blockers
This PR can merge, but the v2 path must not be enabled in production until:
use_balances_v2feature flag is turned on in Amplitude for the rollout (the in-app default is off, so v1 stays in use until then).POST /accounts/balancesreturns 500 on both prod and staging for all networks.Known limitations
scanBulkTokensround trip on mainnet.Checklist
PR structure
Testing
Release