feat(lp-tab): per-token USD amounts + chart stats sidebar#107
feat(lp-tab): per-token USD amounts + chart stats sidebar#107
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Stale comment
This PR replaces LP token-unit display with per-token reserve-derived amounts, adds USD approximations, and adds an LP concentration stats sidebar. It also introduces label-aware pie labels and related tests.
Changed files reviewed:
ui-dashboard/src/app/pool/[poolId]/__tests__/page.test.tsxui-dashboard/src/app/pool/[poolId]/page.tsxui-dashboard/src/components/__tests__/lp-concentration-chart.test.tsui-dashboard/src/components/lp-concentration-chart.tsx
ui-dashboard/src/app/pool/[poolId]/page.tsx🔴 Critical
L1096-L1097,L1140-L1149: USD conversion is enabled wheneverfeedVal !== null, but there is no guard that the pool is actually USD-convertible (i.e. exactly one side inUSDM_SYMBOLS). For non-USDm pairs this treats one raw token reserve as USD and produces incorrect "Total Value".L1140-L1147,L1161-L1174: WhenfeedValis missing,tok0Usd/tok1Usdfall back to raw token amounts and are still rendered as≈ $...for non-USDm symbols. That displays fabricated USD values.🟡 Important
L1081-L1204: New LP token-amount and USD logic has no direct assertions in page-level tests (conversion correctness,showUsdgating, missing-oracle behavior, non-USDm behavior).
ui-dashboard/src/components/lp-concentration-chart.tsx🔴 Critical
L149-L151,L214-L218: Sidebar TVL is computed and rendered even whenfeedValisnull, by summing raw reserves and formatting as dollars. This is incorrect numeric output.🟡 Important
L149-L151: USD derivation logic is duplicated and diverges from the saferpoolTvlUSDsemantics intokens.ts(which explicitly returns0when no USDm side exists). This increases risk of inconsistent financial numbers across views.
ui-dashboard/src/app/pool/[poolId]/__tests__/page.test.tsx🟡 Important
L158-L163: The updated assertion only checks thatShareexists; it does not verify the newly added token columns, per-token amount rendering, USD visibility conditions, or incorrect-USD regression cases.
ui-dashboard/src/components/__tests__/lp-concentration-chart.test.ts🟡 Important
L1-L47: Tests only covertruncateAddressandresolvePieLabel; there is no component-level coverage for stats sidebar calculations (topShare,top3Share,Total TVL) or for oracle-missing/non-USDm branches.Final verdict: 🚫 Needs changes
Sent by Cursor Automation: PR Review
| ? Number(pool.oraclePrice) / 1e24 | ||
| : null; | ||
| const usdmIsToken0 = USDM_SYMBOLS.has(sym0); | ||
| const showUsd = feedVal !== null && hasReserves; |
There was a problem hiding this comment.
showUsd should not be keyed only on feedVal !== null. In non-USDm pools this path still renders a USD total even though one side is not dollar-denominated. Please gate this with an explicit USD-convertibility predicate (for example, exactly one side in USDM_SYMBOLS) and keep this logic shared with existing TVL helpers to avoid drift.
| minimumFractionDigits: 2, | ||
| maximumFractionDigits: 2, | ||
| }); | ||
| const showSubUsd = vUsd !== null && sym !== "USDm"; |
There was a problem hiding this comment.
This renders ≈ $... whenever vUsd !== null, but vUsd is currently set to the raw token amount when feedVal is missing. That produces fabricated dollar values. vUsd should stay null unless an actual conversion happened, and sub-USD rows should render only in that case.
| const hasPoolData = pool && (reserves0Raw > 0 || reserves1Raw > 0); | ||
| const usd0 = feedVal && !usdmIsToken0 ? reserves0Raw * feedVal : reserves0Raw; | ||
| const usd1 = feedVal && usdmIsToken0 ? reserves1Raw * feedVal : reserves1Raw; | ||
| const totalTvl = hasPoolData ? usd0 + usd1 : null; |
There was a problem hiding this comment.
totalTvl is computed whenever reserves exist, even if feedVal is null. In that case usd0/usd1 are raw token values, so the displayed $ TVL is incorrect. Please require a valid conversion path before showing USD TVL (same guard as LP table and/or poolTvlUSD semantics).
|
|
||
| const html = renderToStaticMarkup(<PoolDetailPage />); | ||
| expect(html).toContain("Net LP Tokens"); | ||
| expect(html).toContain("Share"); |
There was a problem hiding this comment.
This test was updated for the new header, but the PR adds substantial new value-conversion behavior. Please add assertions for: token amount columns, USD total visibility only when conversion is valid, and no ≈ $ rendering when oracle price is unavailable.
| }); | ||
| }); | ||
|
|
||
| describe("resolvePieLabel", () => { |
There was a problem hiding this comment.
Coverage currently validates label helpers only. The new sidebar logic (topShare, top3Share, reserve/TVL display) needs component-level tests, especially around oracle-missing and non-USD-convertible pools where this PR currently risks incorrect dollar output.
- Add optional getLabel prop to LpConcentrationChart - Use resolved label for pie slice labels and hover tooltip - Show original address as secondary line in hover when label differs - Wire getLabel from useAddressLabels() in LpsTab - Add useAddressLabels mock to __tests__/page.test.tsx
When an address has no label, hovering showed the truncated address twice (once as label, once as raw address). Now: - labelled address → show name only - unlabelled address → show truncated address only
…+ add unit tests - Extract truncateAddress + resolvePieLabel as exported pure helpers - Narrow getLabel prop to (address: string) => string (chart never passes null; call site wraps with adapter lambda) - Add 6 unit tests covering named label, unlabelled, and no-resolver cases
…lues - LPs table: swap 'Net LP Tokens' for per-token amount columns (sym0, sym1) with inline USD equivalent in muted subtext - Add 'Total Value' column (summed USD) when oracle price is available - Chart header: add stats sidebar next to pie — total LPs, top holder %, top 3 share %, pool reserves per token, total TVL - Extract truncateAddress / resolvePieLabel as named exports for testability - Fix existing test to match updated column names (Share instead of Net LP Tokens)
Plotly merges pie slices with duplicate labels and sums their values, which would silently misrepresent LP concentration when two addresses share the same custom label (e.g. both labelled 'Team Wallet'). Fix: use raw addresses as the key (guaranteed unique) and put the resolved human names in (shown in legend) and (shown in hover). Also: - Remove duplicated truncateAddress, import from shared @/lib/format - Narrow getLabel prop type to (string) => string; call site wraps adapter - Update unit tests: rename _ params to _addr to pass ESLint no-unused-vars, add duplicate-label collision test documenting the labels/text split
… oracle - showUsd now requires hasUsdmSide (XOR: exactly one token is USDm) so non-USDm pairs don't display fabricated dollar values - tok0Usd / tok1Usd now explicitly null when conversion is not valid, preventing raw token amounts being rendered as USD - Chart sidebar totalTvl is null when feedVal is null or no USDm side, so TVL only appears when conversion is meaningful - Import USDM_SYMBOLS in lp-concentration-chart for consistent USD semantics
91984f9 to
778858e
Compare
There was a problem hiding this comment.
Stale comment
This PR adds per-token LP amount estimation, USD conversion, and a new LP concentration stats sidebar in the pool detail view.
The feature direction is good, but there are correctness gaps around invalid oracle-price handling and missing regression coverage for the new calculation/rendering paths.
ui-dashboard/src/app/pool/[poolId]/page.tsx
- 🟡 IMPORTANT (
L1092-L1100,L1148-L1165):feedValis considered valid as long as it is non-null. Ifpool.oraclePriceis malformed or overflows (Number(...)=>NaN/Infinity),showUsdcan stay enabled and USD cells can render≈ $NaN/$NaN. GuardfeedValwithNumber.isFinite(...)(and ideally> 0) before enabling USD rendering/conversion.- 🟡 IMPORTANT (
L1081-L1220): the new per-position token/USD allocation logic is not covered by tests. Current LP tab test only verifies the header text change and ordering. Add cases fortoken0-USDm,token1-USDm, no-USDm pair, and invalid oracle price fallback.
ui-dashboard/src/components/lp-concentration-chart.tsx
- 🟡 IMPORTANT (
L153-L165):totalTvlandfmtUsdassumefeedValis a finite number. With non-finite input, sidebar can display$NaN. Apply the same finite-value guard before TVL math and formatting.- 🟢 SUGGESTION (
L29-L38):resolvePieLabeluses a function-type cast (getLabel as (address: string) => string). This can be simplified by using a single callback type ((address: string | null) => string) end-to-end and removing the cast.
ui-dashboard/src/app/pool/[poolId]/__tests__/page.test.tsx
- 🟡 IMPORTANT (
L163-L171): test assertions do not verify the newly introduced LP token columns, conditionalTotal Valuecolumn, or USD conversion rendering paths.
ui-dashboard/src/components/__tests__/lp-concentration-chart.test.ts
- 🟡 IMPORTANT (
L8-L42): tests still validate onlyresolvePieLabel; there is no coverage for the new stats sidebar (Total LPs,Top holder,Top 3 share, reserves/TVL block).Final verdict: 🚫 Needs changes
Sent by Cursor Automation: PR Review
- replace Plotly legend with explicit custom legend so labels always win - keep raw addresses as slice keys to avoid accidental slice merging - left-align chart + legend block and simplify right-side summary card - use USDM_SYMBOLS consistently for LP sub-USD display - strengthen LP table and chart tests for USD gating + sidebar rendering
There was a problem hiding this comment.
Stale comment
This PR replaces LP provider rows from raw LP token units to per-token amounts, adds conditional USD valuation, and expands the LP concentration chart with a custom legend plus a stats sidebar.
No findings after reviewing all changed files against the critical/important/suggestion criteria.
Residual risk/testing gap: I could not execute the updated Vitest files in this environment because workspace dependencies are not installed (
vitest: not found), so this assessment is static-analysis only.Final verdict: ✅ LGTM
Sent by Cursor Automation: PR Review
- reorder tabs to LPs, Swaps, Reserves, Liquidity, Oracle, OLS - make LPs the default tab when no ?tab= is present - only show OLS when the pool has an active OLS registration - remove stale rebalances tab wiring and update tests for the new default
There was a problem hiding this comment.
This PR adds per-token LP amounts and a richer LP concentration sidebar, and it also changes tab routing/default behavior on the pool detail page. The LP math/chart work is solid overall, but there is a functional regression in tab availability that should block merge.
ui-dashboard/src/app/pool/[poolId]/page.tsx
- 🔴 CRITICAL (
80-92,101-107,328-369): therebalancestab was removed fromTABS, removed from searchable-tab handling, and removed from tab panel rendering. This makes rebalance history unreachable in the UI and breaks existing?tab=rebalancesURLs by forcing fallback to another tab. - 🟡 IMPORTANT (
252-259,1366-1384):OLS_POOLis queried in the parent component to decide tab visibility and queried again insideOlsTab. That duplicates polling/network load and can cause avoidable content flicker for direct?tab=olsnavigation.
ui-dashboard/src/app/pool/[poolId]/page.test.tsx
- 🟡 IMPORTANT (
307-401): rebalance search test coverage was removed from the tab-search suite, so the regression above is no longer guarded by tests.
Final verdict: 🚫 Needs changes
Sent by Cursor Automation: PR Review


What
Improves the LPs tab in pool detail pages.
LP Table
GBPmandUSDm(or whatever the pool's tokens are), showing each LP's proportional share of pool reserves≈ $119.00)LP Concentration Chart
Implementation notes
pool.reserves0/1× LP share (same oracle price logic as ReservesTab)—if reserves not available, hides USD columns if no oracle pricetruncateAddress/resolvePieLabelfor testability (fixes test that was testing inline closures)