fix: get 7d and 1d volume from pools, both in v2 and v3#2572
fix: get 7d and 1d volume from pools, both in v2 and v3#2572gustavobftorres wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds snapshot-based volume computation and a Balancer API fallback when subgraph queries fail, batches snapshot requests, hardens error handling around price/LM APR and tests, and refactors per-chain job execution with added per-chain rejection logging. Returned pool objects now include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TopLvl as TopLvl
participant Subgraph as Subgraph
participant BalancerAPI as Balancer API (Snapshots)
participant Pools as PoolsProcessor
participant PriceSvc as TokenPriceService
TopLvl->>Subgraph: fetch current pools (+prior/prior7d)
alt Subgraph returns valid pools
Subgraph-->>TopLvl: pools (+dynamicData.volume24h)
TopLvl->>Pools: map pools, attach dynamic volumes when finite
else Subgraph fails or empty
Subgraph-->>TopLvl: error/empty
TopLvl->>BalancerAPI: fetch pool list + snapshots (batched)
BalancerAPI-->>TopLvl: pool list + snapshots (or unavailable sentinel)
TopLvl->>Pools: build pools using snapshot-derived volumeUsd1d/7d
end
Pools->>PriceSvc: fetch token prices & LM APRs (try/catch)
PriceSvc-->>Pools: prices / aprs (or defaults on error)
Pools-->>TopLvl: enriched pool objects (include volumeUsd1d, volumeUsd7d, aprLM, rewardTokens)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/adaptors/balancer-v2/index.js`:
- Around line 428-431: The volumeDelta function currently turns an absent prior
value into NaN (via Number(undefined)) causing new pools to show 0; update
volumeDelta (function name: volumeDelta) to treat a missing prior as 0 before
computing the difference (e.g., coerce volumePrior undefined/null to 0), then
compute delta = Number(volumeNow) - Number(coercedPrior) and finally clamp to
non-negative only on the final delta (using Number.isFinite check and delta >
0). This preserves cumulative volume when the historical row is missing while
still preventing negative or non-finite results.
In `@src/adaptors/balancer-v3/index.ts`:
- Around line 73-92: The current catch in the batch loop swallows errors from
request(BALANCER_API_URL, snapshotsQuery, ...) and then seeds every pool in that
batch with an empty array via snapshotsByPoolId, which yields incorrect zero
volumes; either rethrow the error to let the outer chain-level handler drop the
chain, or record a clear "volume unavailable" sentinel per pool instead of [] so
downstream code (which computes volumeUsd7d) can detect unavailable data. Update
the try/catch around request in buildSnapshotsQuery usage: on error either throw
the caught error (preserving stack) or set
snapshotsByPoolId.set(poolId.toLowerCase(), VOLUME_UNAVAILABLE_SENTINEL) for
each pool in the batch (define a unique sentinel such as null or an object like
{unavailable:true}) and adjust downstream checks to treat that sentinel as
unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85c20818-d657-4efd-8c5e-222e90a68bf1
📒 Files selected for processing (2)
src/adaptors/balancer-v2/index.jssrc/adaptors/balancer-v3/index.ts
…ity raise a error about it
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/adaptors/beforeTests.js (1)
58-59: Fail fast whenapy()returns the wrong shape.Coercing non-array results to
[]turns a broken adapter contract into downstream “0 pools / missing link / empty project” noise. Throw here with the adapter name so CI surfaces the real failure at setup time.♻️ Proposed fix
global.adapter = adapter; const apyRaw = await module.apy(timestamp); - global.apy = (Array.isArray(apyRaw) ? apyRaw : []).sort( + if (!Array.isArray(apyRaw)) { + throw new TypeError(`${adapter}.apy() must return an array`); + } + global.apy = apyRaw.sort( (a, b) => b.tvlUsd - a.tvlUsd );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/beforeTests.js` around lines 58 - 59, The code currently coerces non-array apy() results to [] which hides adapter contract failures; update the setup to validate that apyRaw from module.apy(timestamp) is an Array and if not immediately throw an Error that includes the adapter identifier (e.g., module.name or module.adapterName) and the received type/value; replace the current coercion in the global.apy assignment so you only assign sorted apy when Array.isArray(apyRaw) is true, otherwise throw with a clear message referencing apyRaw, module.apy and the adapter name so CI fails fast.src/adaptors/test.js (1)
69-72: Short-circuit the first-pool checks after the Line 26 emptiness assertion.A zero-pool adapter now fails three times in the same suite (
length,url, andproject), which makes CI noisier than it needs to be. Let the explicit emptiness test own that failure and skip these two first-pool-dependent assertions whenapy.length === 0.♻️ Proposed fix
test("Check if link to the pool's page exist", () => { + if (apy.length === 0) return; const poolsLink = apy[0]?.url || poolsUrl; expect(typeof poolsLink).toBe('string'); }); @@ test('All pools should have the same project field matching the adapter name and a known protocol slug', () => { + if (apy.length === 0) return; const projectNames = [...new Set(apy.map((p) => p.project))]; expect(projectNames).toEqual([adapter]); const firstProject = apy[0]?.project; expect(typeof firstProject).toBe('string'); expect(protocols).toContain(firstProject);Also applies to: 182-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/test.js` around lines 69 - 72, The tests that inspect the first pool (the "Check if link to the pool's page exist" test using apy, poolsLink, poolsUrl) should early-return when the collection is empty: after the explicit emptiness assertion for apy (the test that verifies apy.length === 0), add a guard in the dependent tests so they skip when apy.length === 0 (e.g., if (apy.length === 0) return); apply the same short-circuit to the other first-pool-dependent tests later in the file (the block around the project/url checks at the second occurrence) so only the emptiness test fails for zero-pool adapters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/adaptors/beforeTests.js`:
- Around line 58-59: The code currently coerces non-array apy() results to []
which hides adapter contract failures; update the setup to validate that apyRaw
from module.apy(timestamp) is an Array and if not immediately throw an Error
that includes the adapter identifier (e.g., module.name or module.adapterName)
and the received type/value; replace the current coercion in the global.apy
assignment so you only assign sorted apy when Array.isArray(apyRaw) is true,
otherwise throw with a clear message referencing apyRaw, module.apy and the
adapter name so CI fails fast.
In `@src/adaptors/test.js`:
- Around line 69-72: The tests that inspect the first pool (the "Check if link
to the pool's page exist" test using apy, poolsLink, poolsUrl) should
early-return when the collection is empty: after the explicit emptiness
assertion for apy (the test that verifies apy.length === 0), add a guard in the
dependent tests so they skip when apy.length === 0 (e.g., if (apy.length === 0)
return); apply the same short-circuit to the other first-pool-dependent tests
later in the file (the block around the project/url checks at the second
occurrence) so only the emptiness test fails for zero-pool adapters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8064f90a-ba81-480f-a012-4f88c6999833
📒 Files selected for processing (3)
.github/workflows/test.ymlsrc/adaptors/beforeTests.jssrc/adaptors/test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/adaptors/balancer-v3/index.ts (1)
167-184:⚠️ Potential issue | 🟠 MajorDon't let
nullvolume24hsuppress the snapshot fallback.Line 167 turns
nullinto0, and Lines 181-184 treat that as authoritative, sovolumeUsd1dcan be reported as0even when the latest snapshot has non-zero volume. This also diverges from the V2 path insrc/adaptors/balancer-v2/index.js:574-581, which only trusts dynamic 24h volume when it is positive.Proposed fix
- const dynamicVolume24h = Number(pool.dynamicData?.volume24h); + const rawDynamicVolume24h = pool.dynamicData?.volume24h; + const dynamicVolume24h = + rawDynamicVolume24h == null ? undefined : Number(rawDynamicVolume24h); const poolData = { pool: pool.address, chain: utils.formatChain(chainString), project: 'balancer-v3', symbol: utils.formatSymbol(pool.symbol), @@ - if (Number.isFinite(dynamicVolume24h)) { + if ( + dynamicVolume24h !== undefined && + Number.isFinite(dynamicVolume24h) && + dynamicVolume24h > 0 + ) { poolData.volumeUsd1d = dynamicVolume24h; } else if (volumeUsd1dFromSnapshots !== undefined) { poolData.volumeUsd1d = volumeUsd1dFromSnapshots; + } else if ( + dynamicVolume24h !== undefined && + Number.isFinite(dynamicVolume24h) + ) { + poolData.volumeUsd1d = dynamicVolume24h; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/balancer-v3/index.ts` around lines 167 - 184, The code converts pool.dynamicData?.volume24h to a Number (dynamicVolume24h) which turns null into 0, and then the if (Number.isFinite(dynamicVolume24h)) branch treats 0 as authoritative; change the logic so you only trust dynamic 24h volume when the raw value is present and positive. Inspect pool.dynamicData?.volume24h (or a rawVolume variable) and only set poolData.volumeUsd1d from dynamicVolume24h when rawVolume != null/undefined and Number(rawVolume) > 0; otherwise fall back to volumeUsd1dFromSnapshots as before (references: pool.dynamicData?.volume24h, dynamicVolume24h, poolData.volumeUsd1d, volumeUsd1dFromSnapshots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/adaptors/balancer-v3/index.ts`:
- Around line 167-184: The code converts pool.dynamicData?.volume24h to a Number
(dynamicVolume24h) which turns null into 0, and then the if
(Number.isFinite(dynamicVolume24h)) branch treats 0 as authoritative; change the
logic so you only trust dynamic 24h volume when the raw value is present and
positive. Inspect pool.dynamicData?.volume24h (or a rawVolume variable) and only
set poolData.volumeUsd1d from dynamicVolume24h when rawVolume != null/undefined
and Number(rawVolume) > 0; otherwise fall back to volumeUsd1dFromSnapshots as
before (references: pool.dynamicData?.volume24h, dynamicVolume24h,
poolData.volumeUsd1d, volumeUsd1dFromSnapshots).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 898573a9-193d-41eb-8c5d-66c2040c5e02
📒 Files selected for processing (1)
src/adaptors/balancer-v3/index.ts
…plify code based on that diagnostic
|
hi @gustavobftorres, thank you for your changes, please separate CI / test changes into separate PR |
|
The balancer-v2 adapter exports pools: Test Suites: 1 passed, 1 total |
| return pools | ||
| .filter((pool) => pool.id || pool.address) | ||
| .map((pool) => { | ||
| const poolId = pool.id || pool.address; |
There was a problem hiding this comment.
pls remove the pool.address fallback. The pool field should only have one value else historical data can be lost as a new pool value = new entry
| // balancer splits off a pct cut of swap fees to the protocol, get pct value: | ||
| const swapFeePercentage = | ||
| ( | ||
| let swapFeePercentage = 1; |
There was a problem hiding this comment.
onchain value is 0.5, should this be the default rather than 1?
| token: tokenAddress, | ||
| tvlUsd: toNumber(pool.dynamicData?.totalLiquidity), | ||
| apyBase: 0, | ||
| apyReward: 0, |
There was a problem hiding this comment.
setting apyBase/Reward to 0 is misleading, they may not be 0. I would opt to drop pool than set to 0 due to incomplete data
| chain: utils.formatChain(chainString), | ||
| project: 'balancer-v3', | ||
| symbol: utils.formatSymbol(pool.symbol || ''), | ||
| tvlUsd: toNumber(dynamicData.totalLiquidity), |
There was a problem hiding this comment.
Previously Number(pool.dynamicData.totalLiquidity) would emit NaN for
malformed values and utils.keepFinite filtered the pool out. Now toNumber(...) coerces NaN to 0,
and the same pattern on apyBase/apyReward (lines 339–340) does likewise. Pools that previously
got dropped will now appear with tvlUsd: 0 / apyBase: 0 — same problem as the V2 fallback. Either
keep the NaN-flows-through behavior, or pair these coercions with a guard that drops pools where the core fields all coerced to 0
0xkr3p
left a comment
0xkr3p
left a comment
There was a problem hiding this comment.
hey @gustavobftorres, thanks for the PR, just a few comments to resolve pls
2 participants
Add Balancer V2/V3
volumeUsd1dandvolumeUsd7dto Yields adaptersSummary
This PR fills missing 1d/7d volume fields for Balancer pools in DefiLlama Yields so
show7dVolume=truedisplays values for Balancer V2 and Balancer V3 pools.What changed
1) Balancer V2 adapter (
src/adaptors/balancer-v2/index.js)totalSwapVolume(current and prior-block queries).utils.getBlocks(..., 604800).volumeUsd1d = totalSwapVolume(now) - totalSwapVolume(1dAgo)volumeUsd7d = totalSwapVolume(now) - totalSwapVolume(7dAgo)0.volumeUsd1dandvolumeUsd7din adapter pool output.2) Balancer V3 adapter (
src/adaptors/balancer-v3/index.ts)poolGetPoolsflow for TVL/APY data.dynamicData.volume24hto base pool query.poolGetSnapshotswithrange: THIRTY_DAYS.SNAPSHOT_BATCH_SIZE = 25) to reduce request count.timestampdescending.volumeUsd7d = sum(volume24h of latest up to 7 snapshots)(supports partial history for newer pools).volumeUsd1d = dynamicData.volume24hwith fallback to latest snapshot day.volumeUsd1dandvolumeUsd7din adapter pool output.Why
Balancer V2/V3 pools were missing
volumeUsd7din Yields responses, causing blank 7d volume UI fields.These changes populate the existing optional fields already supported by
yield-serveringestion/query layers.Compatibility
Notes
THIRTY_DAYS(minimum available in Balancer API), with local slicing to 7 days.Summary by CodeRabbit