Conversation
![ripple-code-reviewer[bot]](https://avatars.githubusercontent.com/u/3165523?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured fix that scopes AMM TVL/volume to XRP-based pools across the rankings list, stat tiles, historical-trends chart, and the individual pool page, with careful undefined-vs-0 handling for derived USD values and a necessary cache-key fix. One reliability gap: fetchAggregatedStats now makes two independent parallel LOS calls but discards the successful XRP-only rollup (with real TVL/volume data) whenever the all-pools call fails, even though before this change failure handling was atomic on a single call.
There was a problem hiding this comment.
Well-structured fix that scopes AMM TVL/volume to XRP-based pools across the rankings list, aggregated stats, historical-trends cache, and the individual pool page, with the undefined-vs-0 suppression pattern applied consistently everywhere it's needed (AuctionCard, TablePicker, MarketDataCard, chart gating) and a cache-key fix for the XRP-only vs all-pools historical trends split. I didn't find correctness or security bugs in the added code — the merge logic in fetchAggregatedStats, the xrp_only param threading, and the isXrpBased derivation in AMMPool/index.tsx all match the stated design and are covered by the new tests. Two minor, low-risk points worth a look below.
There was a problem hiding this comment.
This is a well-scoped, carefully tested fix. The core changes — xrp_only gating on the pool list/aggregated stats/chart, undefined-vs-0 suppression in AuctionCard/MarketDataCard/TablePicker, and the historical-trends cache-key fix — all check out against the stated intent and are covered by new tests. I only found one edge-case resilience gap worth a look; everything else in the changed lines is correct.
There was a problem hiding this comment.
This is a well-scoped, carefully-tested fix for the AMM TVL overstatement bug. The core logic changes (XRP-only gating in AMMPool/index.tsx, MarketDataCard, AuctionCard's reordered null-check, TablePicker's USD suppression, and the historical-trends cache-key fix in amms.js) all look correct on inspection of the added lines, and match the stated design rationale (suppress with undefined/null rather than 0, check ledger balances before the LOS flag, keep counts unscoped while gating value figures). I did not find a clear, high-confidence bug introduced by the diff. The one item below is a design/availability tradeoff worth a quick sanity check rather than a definite defect.
There was a problem hiding this comment.
Well-scoped, carefully-reasoned fix for the AMM TVL/volume accuracy bug. The gating logic (isXrpBased ordering, undefined-vs-0 suppression, historical-trends cache key fix) is correctly implemented and matches the PR's own detailed rationale, and I couldn't find a correctness or security bug in the added lines. The one thing worth a second look is that fetchAggregatedStats now fans out to two upstream LOS calls per request with no caching layer (unlike /historical-trends, which does cache), so every hit to this route doubles load on /amms/aggregated.
There was a problem hiding this comment.
Well-scoped, well-tested fix: server now merges two LOS aggregates (all-pool counts + XRP-only value figures), the pool list is XRP-only, and derived USD values (auction price, holder value, deposit/withdraw USD) are consistently suppressed with undefined/null rather than 0. The historical-trends cache key correctly now includes the xrp_only flag. I traced the isXrpBased gating logic (ledger balance checked before the LOS flag) and the AuctionCard reordering fix and both match the stated intent with no regressions found. Only a minor documentation defect stood out.
There was a problem hiding this comment.
This is a well-scoped, carefully reasoned fix for the AMM TVL/volume accuracy bug. The server-side aggregation split (fetchAggregate/fetchAggregatedStats), the AuctionCard reorder fixing the '$0.00 with suppressed TVL' bug, the MarketDataCard/TablePicker gating via undefined/null (never 0), and the historical-trends cache-key fix are all correctly implemented and match the stated intent. Test coverage specifically targets the negative cases (no $0.00, no NaN) called out in the description. I did not find functional bugs in the added/changed lines — only a minor doc-comment clarity nit.
… redirect instead of erroring
There was a problem hiding this comment.
The core AMM TVL fix (server-side xrp_only gating, mixed-scope stat tiles, cache-key fix, and the isXrpBased suppression chain across MarketDataCard/AuctionCard/TablePicker) is implemented consistently and matches the PR description well — undefined is used correctly instead of 0 throughout, and the ledger-first/flag-fallback ordering in isXrpBased is correct. The one notable concern is an unrelated, undocumented change to the generic getAccountInfo fallback logic in rippled.ts that broadens scope beyond AMM/vault pseudo-accounts.
|
|
||
| if (resp.error_message) { | ||
| throw new Error(resp.error_message, 500) | ||
| log.warn( |
There was a problem hiding this comment.
Fallback on all errors masks transient failures with incomplete signer_lists. Check AMMID before returning:
const fallback = await getAccountRoot(rippledSocket, account)
if (fallback?.AMMID) {
return fallback
}
throw new Error(resp.error_message, 500)
1 participant
High Level Overview of Change
Stops the explorer showing AMM TVL, volume and fees for pools that can't be priced reliably, and fixes AMM account pages that show the generic error page.
Requires ripple/ledger-object-service#135, which adds
xrp_only,is_xrp_basedand the XRP-only aggregate. Merge only after that aggregate's backfill has caught up in prod.--. The chart and every USD value derived from those figures are hidden.getAccountInfofalls back toledger_entrywhenaccount_infofails, so AMM accounts redirect to their pool page again.Context of Change
Token/token pools dominate the current rankings page: they hold 8 of the top 10 spots, and the top pool reports $354M TVL with a single liquidity provider. A token/token pool has no asset with a market price, so its TVL can be overstated by orders of magnitude. The typical case is an issuer valuing its own freely-minted token. Sorting by TVL put those pools at the top of the rankings and inflated the stat tiles. Counts are accurate for every pool, so those tiles stay all-pools.
Clio 2.8.0 on s1/s2.ripple.com returns
internalforaccount_infoon pseudo-accounts.ledger_entryreturns the same AccountRoot,AMMIDincluded. It doesn't return signer lists, but pseudo-accounts can't have any.Type of Change
Before / After
The "before" column is prod, which runs the old code until this PR is deployed.
--Test Plan
Automated
Run all unit tests
Manual
On dev (https://livenet.dev.ripplex.io):
--for TVL, volume, fees and APR. It shows no chart and no≈ $values, and$0.00andNaNappear nowhere.Follow-ups
getAccountInfofallback once s1/s2 run a Clio with the fix.