Add tag breakdown to perp dexes - #7801
Open
FelixBruguera wants to merge 9 commits into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughSix DEX adapters (apollox, bluefin-pro, citrex-markets, dydx-v4, jupiter-perpetual, variational-omni, vest) are refactored to compute ChangesPer-asset volume and tag-breakdown aggregation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 8 | ❌ 7❌ Failed checks (7 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
|
The apollox adapter exports: |
|
The bluefin-pro adapter exports: |
|
The citrex-markets adapter exports: |
|
The dydx-v4 adapter exports: |
|
The jupiter-perpetual adapter exports: |
|
The variational-omni adapter exports: |
|
The vest adapter exports: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dexs/jupiter-perpetual/index.ts`:
- Around line 29-34: The code uses Promise.all to execute all market HTTP
requests concurrently without bounds, which violates the repository's coding
guidelines. Replace the Promise.all(markets.map(...)) pattern in the dailyVolume
block with PromisePool to limit concurrency. Keep the same async function logic
that calls httpGet for each market with mint and symbol parameters and
accumulates volume results via dailyVolume.addUSDValue, but use PromisePool
instead to prevent unbounded fan-out as the markets list grows.
In `@dexs/variational-omni/index.ts`:
- Around line 16-18: The code currently uses a nullish coalescing operator on
data.listings which silently defaults to an empty array when the listings
payload is missing, resulting in incorrect zero volume being written. Remove the
`?? []` fallback pattern in the loop that iterates over data.listings and
instead add validation logic before the loop that checks if data.listings exists
and is a valid array; throw an error with a descriptive message if validation
fails, ensuring that missing or invalid adapter responses fail loudly rather
than being swallowed silently.
In `@dexs/vest/index.ts`:
- Around line 16-19: The code currently coerces missing ticker.quoteVolume
values to zero using Number(ticker.quoteVolume || 0), which hides missing API
fields and underreports volume. Instead of using the fallback to zero, add
validation to check that ticker.quoteVolume exists and is a valid number, and
throw an error if it's absent or non-numeric. This ensures data integrity by
marking the day unavailable rather than silently charting with incorrect volume
data in the loop where dailyVolume.addUSDValue is called.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 02b6f1f6-5e62-46f0-8bbe-b765a2bdf6a8
📒 Files selected for processing (8)
adapters/utils/runAdapter.tsdexs/apollox/index.tsdexs/bluefin-pro/index.tsdexs/citrex-markets/index.tsdexs/dydx-v4/index.tsdexs/jupiter-perpetual/index.tsdexs/variational-omni/index.tsdexs/vest/index.ts
Comment on lines
+29
to
+34
| const dailyVolume = options.createBalances(); | ||
| await Promise.all(markets.map(async ({ mint, symbol }) => { | ||
| const res: any[] = (await httpGet(url(mint), { headers: header_user })).flat(); | ||
| const volume = res.reduce((acc: number, { result }: any) => acc + result.data.json.volume, 0); | ||
| dailyVolume.addUSDValue(volume, { id: symbol, isUSDValue: true }); | ||
| })); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Use PromisePool for the per-market HTTP fan-out.
Line 30 starts all non-EVM market requests with Promise.all; use the repo’s PromisePool pattern instead so this does not become unbounded if the market list grows. As per coding guidelines, “Use PromisePool for non-EVM calls” and “avoid Promise.all.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dexs/jupiter-perpetual/index.ts` around lines 29 - 34, The code uses
Promise.all to execute all market HTTP requests concurrently without bounds,
which violates the repository's coding guidelines. Replace the
Promise.all(markets.map(...)) pattern in the dailyVolume block with PromisePool
to limit concurrency. Keep the same async function logic that calls httpGet for
each market with mint and symbol parameters and accumulates volume results via
dailyVolume.addUSDValue, but use PromisePool instead to prevent unbounded
fan-out as the markets list grows.
Source: Coding guidelines
Comment on lines
+16
to
+19
| for (const ticker of data) { | ||
| if (blacklisted_tickers.includes(ticker.symbol)) continue; | ||
| const baseAsset = String(ticker.symbol).split("-")[0]; // "TSM-USD-PERP" -> "TSM", "BZ-PERP" -> "BZ" | ||
| dailyVolume.addUSDValue(Number(ticker.quoteVolume || 0), { id: baseAsset, isUSDValue: true }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not coerce missing ticker volume to zero.
Number(ticker.quoteVolume || 0) hides missing API fields and underreports volume instead of failing the run. Throw on absent/non-numeric quoteVolume so the day is marked unavailable rather than charted incorrectly.
Proposed fix
for (const ticker of data) {
if (blacklisted_tickers.includes(ticker.symbol)) continue;
const baseAsset = String(ticker.symbol).split("-")[0]; // "TSM-USD-PERP" -> "TSM", "BZ-PERP" -> "BZ"
- dailyVolume.addUSDValue(Number(ticker.quoteVolume || 0), { id: baseAsset, isUSDValue: true });
+ if (ticker.quoteVolume == null) throw new Error(`Vest ticker missing quoteVolume for ${ticker.symbol}`);
+ const quoteVolume = Number(ticker.quoteVolume);
+ if (!Number.isFinite(quoteVolume)) throw new Error(`Invalid Vest quoteVolume for ${ticker.symbol}`);
+ dailyVolume.addUSDValue(quoteVolume, { id: baseAsset, isUSDValue: true });
}Based on learnings, adapter fetches should not return sentinel zero values for missing data; as per coding guidelines, never swallow errors silently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const ticker of data) { | |
| if (blacklisted_tickers.includes(ticker.symbol)) continue; | |
| const baseAsset = String(ticker.symbol).split("-")[0]; // "TSM-USD-PERP" -> "TSM", "BZ-PERP" -> "BZ" | |
| dailyVolume.addUSDValue(Number(ticker.quoteVolume || 0), { id: baseAsset, isUSDValue: true }); | |
| for (const ticker of data) { | |
| if (blacklisted_tickers.includes(ticker.symbol)) continue; | |
| const baseAsset = String(ticker.symbol).split("-")[0]; // "TSM-USD-PERP" -> "TSM", "BZ-PERP" -> "BZ" | |
| if (ticker.quoteVolume == null) throw new Error(`Vest ticker missing quoteVolume for ${ticker.symbol}`); | |
| const quoteVolume = Number(ticker.quoteVolume); | |
| if (!Number.isFinite(quoteVolume)) throw new Error(`Invalid Vest quoteVolume for ${ticker.symbol}`); | |
| dailyVolume.addUSDValue(quoteVolume, { id: baseAsset, isUSDValue: true }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dexs/vest/index.ts` around lines 16 - 19, The code currently coerces missing
ticker.quoteVolume values to zero using Number(ticker.quoteVolume || 0), which
hides missing API fields and underreports volume. Instead of using the fallback
to zero, add validation to check that ticker.quoteVolume exists and is a valid
number, and throw an error if it's absent or non-numeric. This ensures data
integrity by marking the day unavailable rather than silently charting with
incorrect volume data in the loop where dailyVolume.addUSDValue is called.
Sources: Coding guidelines, Learnings
|
The apollox adapter exports: |
|
The bluefin-pro adapter exports: |
|
The citrex-markets adapter exports: |
|
The dydx-v4 adapter exports: |
|
The jupiter-perpetual adapter exports: |
|
The variational-omni adapter exports: |
|
The vest adapter exports: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dexs/variational-omni/index.ts`:
- Around line 19-21: In the for loop iterating through data.listings, the
conversion of listing.volume_24h using Number() does not validate the result
before adding to dailyVolume. After converting listing.volume_24h to a Number,
validate that the result is finite using Number.isFinite() to ensure it is not
NaN or Infinity. If the value is not finite, throw an error or skip the entry
loudly to fail fast rather than silently accepting malformed data that would
corrupt the dailyVolume output.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3684db5b-1f3d-4b3e-86f9-ec0da8f8777a
📒 Files selected for processing (1)
dexs/variational-omni/index.ts
Comment on lines
+19
to
+21
| for (const listing of data.listings) { | ||
| dailyVolume.addUSDValue(Number(listing.volume_24h), { id: baseAsset(String(listing.ticker)), isUSDValue: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate each listing’s volume_24h before adding to dailyVolume.
Line 20 converts listing.volume_24h with Number(...) but does not verify finiteness. A malformed row (null, missing, or non-numeric) can produce NaN and pollute persisted volume output.
Proposed fix
for (const listing of data.listings) {
- dailyVolume.addUSDValue(Number(listing.volume_24h), { id: baseAsset(String(listing.ticker)), isUSDValue: true });
+ if (listing.volume_24h == null) {
+ throw new Error(`Variational listing missing volume_24h for ${listing.ticker}`)
+ }
+ const volume24h = Number(listing.volume_24h)
+ if (!Number.isFinite(volume24h)) {
+ throw new Error(`Invalid Variational volume_24h for ${listing.ticker}`)
+ }
+ dailyVolume.addUSDValue(volume24h, { id: baseAsset(String(listing.ticker)), isUSDValue: true });
}Based on learnings, adapter payload gaps should fail loudly rather than emitting wrong chart data; as per coding guidelines, avoid silently accepting bad data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const listing of data.listings) { | |
| dailyVolume.addUSDValue(Number(listing.volume_24h), { id: baseAsset(String(listing.ticker)), isUSDValue: true }); | |
| } | |
| for (const listing of data.listings) { | |
| if (listing.volume_24h == null) { | |
| throw new Error(`Variational listing missing volume_24h for ${listing.ticker}`) | |
| } | |
| const volume24h = Number(listing.volume_24h) | |
| if (!Number.isFinite(volume24h)) { | |
| throw new Error(`Invalid Variational volume_24h for ${listing.ticker}`) | |
| } | |
| dailyVolume.addUSDValue(volume24h, { id: baseAsset(String(listing.ticker)), isUSDValue: true }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dexs/variational-omni/index.ts` around lines 19 - 21, In the for loop
iterating through data.listings, the conversion of listing.volume_24h using
Number() does not validate the result before adding to dailyVolume. After
converting listing.volume_24h to a Number, validate that the result is finite
using Number.isFinite() to ensure it is not NaN or Infinity. If the value is not
finite, throw an error or skip the entry loudly to fail fast rather than
silently accepting malformed data that would corrupt the dailyVolume output.
Sources: Coding guidelines, Learnings
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
2 participants
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.
https://github.com/DefiLlama/internal-docs/issues/40
Added breakdownByTag to the runner to return a breakdown of perp volume by asset tag (RWA, Layer 1, Meme...) and refactored multiple runAtCurrTime perp dex adapters to return balance objects with symbols