Open
Conversation
Contributor
|
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 a Curvance adapter and an ABI module. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Adapter as Curvance Adapter
participant Registry as Central Registry
participant Manager as Market Manager
participant Chain as On-chain Contracts
participant PriceSvc as Price Service
Caller->>Adapter: apy(chainId)
Adapter->>Registry: getMarketManagers(chainId)
Registry-->>Adapter: managers[]
loop per manager
Adapter->>Manager: getMarkets()
Manager-->>Adapter: markets[]
loop per market
Adapter->>Chain: getDynamicMarketData(market) (ABI)
Chain-->>Adapter: dynamicData
Adapter->>Chain: getUnderlyingToken(market)
Chain-->>Adapter: tokenAddress
Adapter->>Chain: getTokenMetadata(tokenAddress)
Chain-->>Adapter: symbol, decimals
end
end
Adapter->>PriceSvc: batchQueryPrices(underlyingTokens)
PriceSvc-->>Adapter: prices{}
Adapter->>Adapter: compute TVL, APY, supply/borrow metrics
Adapter-->>Caller: pools[]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
|
The curvance adapter exports pools: Test Suites: 1 passed, 1 total |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/adaptors/curvance/index.js`:
- Around line 169-170: The statement building poolMeta is missing a trailing
semicolon; update the assignment to poolMeta (the line using
managerSymbols[manager].join("/")) to terminate the statement with a semicolon
so the two consecutive statements (const manager =
marketToManager[market.toLowerCase()]; and const poolMeta =
managerSymbols[manager].join("/");) are properly separated.
- Around line 78-84: The multiCall using sdk.api.abi.multiCall (the
underlyingRes variable) uses permitFailure: true, so each result element is
{success, output}; update the code that builds underlyings from
underlyingRes.output to only use outputs where success is true (e.g., map/filter
the underlyingRes.output array or reduce to extract output when success) and
handle/skip failed calls (or provide a sensible default) so undefined outputs
don't propagate; apply the same fix to the other multiCall usages that
destructure output directly (the subsequent calls that build
borrowRates/underlyings at the later multiCall sites).
- Around line 144-189: The map callback over markets currently returns null for
entries missing required data (inside the markets.map(...) block where it checks
if (!symbol || !underlying || !price) return null), but those nulls are left in
the returned array; update the code that returns the mapped array to filter out
null results (e.g., apply a .filter(...) after the markets.map call or replace
map with a combined flatMap) so the final returned value contains only the pool
objects (with fields like pool, chain, tvlUsd, apyBase, etc.) and no nulls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33f5da8c-eab7-4429-8ae0-412091d31e01
📒 Files selected for processing (2)
src/adaptors/curvance/abi.jssrc/adaptors/curvance/index.js
|
The curvance adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/adaptors/curvance/index.js (1)
27-29: Consider adding timeout protection for external price requests.The
axios.getcall here lacks a timeout. While this is consistent with most other adapters in the codebase, adding timeout protection would be a defensive improvement to prevent potential hung requests from stalling the adapter.Suggested improvement
for (let i = 0; i < addresses.length; i += chunkSize) { const chunk = addresses.slice(i, i + chunkSize); + const timeoutMs = 10_000; const { data } = await axios.get( - `https://coins.llama.fi/prices/current/${chunk.join(',')}` + `https://coins.llama.fi/prices/current/${chunk.join(',')}`, + { timeout: timeoutMs } ); for (const [key, val] of Object.entries(data.coins)) { prices[key.split(':')[1].toLowerCase()] = val.price; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/curvance/index.js` around lines 27 - 29, The axios.get call that fetches prices (the line using axios.get with chunk.join(',')) lacks a timeout; update that call to include a timeout option (e.g., { timeout: 5000 }) or wire in an existing shared DEFAULT_TIMEOUT constant and use it in the axios.get options so external price requests cannot hang indefinitely; ensure you add the timeout to the same axios.get invocation that destructures const { data } = await axios.get(...).
🤖 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/curvance/index.js`:
- Around line 84-111: underlyings currently includes null entries from
underlyingRes.output.map when asset calls failed, then those nulls are passed as
targets into sdk.api.abi.multiCall for symbol/decimals which can break the
multicall; filter out falsy/null addresses before building the multicall calls
(e.g., create a filteredUnderlyings = underlyings.filter(Boolean) or filter
based on underlyingRes.output success) and use filteredUnderlyings.map(...) for
the symbolRes and decimalsRes multicalls so only valid contract addresses are
passed as targets while keeping permitFailure: true.
---
Nitpick comments:
In `@src/adaptors/curvance/index.js`:
- Around line 27-29: The axios.get call that fetches prices (the line using
axios.get with chunk.join(',')) lacks a timeout; update that call to include a
timeout option (e.g., { timeout: 5000 }) or wire in an existing shared
DEFAULT_TIMEOUT constant and use it in the axios.get options so external price
requests cannot hang indefinitely; ensure you add the timeout to the same
axios.get invocation that destructures const { data } = await axios.get(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad2d2d11-6522-489e-a3bb-0c112f169bf8
📒 Files selected for processing (1)
src/adaptors/curvance/index.js
|
The curvance adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/adaptors/curvance/index.js (1)
63-72: Consider explicit success check for market list calls.Line 67 uses
|| []as a fallback, which prevents crashes, but an explicit success check would be more consistent with the pattern used elsewhere in this file (lines 84-86, 131-132).♻️ Optional improvement for consistency
for (let i = 0; i < managers.length; i++) { - const list = marketsRes.output[i].output || []; + const res = marketsRes.output[i]; + const list = res?.success ? res.output : []; for (const addr of list) { markets.push(addr); marketToManager[addr.toLowerCase()] = managers[i].toLowerCase(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/curvance/index.js` around lines 63 - 72, The code currently uses a silent fallback (marketsRes.output[i].output || []) which masks failed market-list calls; update the loop that builds markets and marketToManager to explicitly check marketsRes.output[i].success (or equivalent success flag) before reading output, skip or handle failed entries (e.g., log/continue) instead of using the || [] fallback, and only iterate the returned output when success is true; reference the variables marketToManager, markets, managers and marketsRes.output to locate and update the logic.
🤖 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/curvance/index.js`:
- Around line 18-35: getPrices lacks error handling around the axios.get call so
a failing coins.llama.fi request will throw and break the adapter; wrap the
per-chunk network call in a try/catch inside getPrices, log the error (or emit
via existing logger) and continue processing remaining chunks, and for any
tokens in the failed chunk either skip them or set their price to null/NaN in
the prices object so callers can handle missing prices; ensure you reference the
chunk variable and preserve chunkSize/addresses looping behavior when
implementing the try/catch and optional retry/backoff logic.
- Around line 44-54: In getPoolsForChain, the response from sdk.api.abi.call
(managersRes) is used without checking its success flag when permitFailure:
true; update the logic to verify managersRes exists and managersRes.success is
true before reading managersRes.output (e.g., treat failures by returning an
empty array or handling the error), and ensure any early-return or logging
occurs when managersRes.success is false so managers is never dereferenced when
the call failed.
---
Nitpick comments:
In `@src/adaptors/curvance/index.js`:
- Around line 63-72: The code currently uses a silent fallback
(marketsRes.output[i].output || []) which masks failed market-list calls; update
the loop that builds markets and marketToManager to explicitly check
marketsRes.output[i].success (or equivalent success flag) before reading output,
skip or handle failed entries (e.g., log/continue) instead of using the || []
fallback, and only iterate the returned output when success is true; reference
the variables marketToManager, markets, managers and marketsRes.output to locate
and update the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fccdc139-b36a-4b50-af0a-292e0e9f8d54
📒 Files selected for processing (1)
src/adaptors/curvance/index.js
Minor fixes Avoid using null targets Error handling for price fetching
|
The curvance adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/adaptors/curvance/index.js (1)
49-56:⚠️ Potential issue | 🟠 MajorGuard manager discovery on
successwhen usingpermitFailure.Line 55 dereferences
managersRes.outputdirectly. WithpermitFailure: true, failed calls should be gated onsuccessbefore readingoutput.Proposed fix
- const managers = managersRes.output; + const managers = managersRes?.success ? managersRes.output : null; if (!managers || !managers.length) { return []; }In `@defillama/sdk` v5.0.207, what is the return shape of sdk.api.abi.call when permitFailure:true and the call fails? Is `success` false with `output` unset?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/curvance/index.js` around lines 49 - 56, The code reads managersRes.output unguarded after calling sdk.api.abi.call with permitFailure:true; instead, check managersRes.success (from the sdk call) before accessing output and only set managers = managersRes.output when success is true, otherwise handle the failed call path (e.g., log or skip) to avoid dereferencing undefined; update the logic around the variables managersRes and managers in the function that calls sdk.api.abi.call (the centralRegistry/marketManagers block) to gate on managersRes.success and treat the failure case explicitly.
🤖 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/curvance/index.js`:
- Around line 28-30: The axios.get call that fetches
`https://coins.llama.fi/prices/current/${chunk.join(',')}` is missing a request
timeout; update the call to include a timeout option (e.g. { timeout: 5000 }) so
the request fails fast on hangs, or use an axios instance with a default timeout
and replace the inline axios.get call accordingly; ensure the timeout value is
reasonable for your environment and keep the request semantics the same.
---
Duplicate comments:
In `@src/adaptors/curvance/index.js`:
- Around line 49-56: The code reads managersRes.output unguarded after calling
sdk.api.abi.call with permitFailure:true; instead, check managersRes.success
(from the sdk call) before accessing output and only set managers =
managersRes.output when success is true, otherwise handle the failed call path
(e.g., log or skip) to avoid dereferencing undefined; update the logic around
the variables managersRes and managers in the function that calls
sdk.api.abi.call (the centralRegistry/marketManagers block) to gate on
managersRes.success and treat the failure case explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9387e47b-2369-4eca-8f38-e3de47ae0320
📒 Files selected for processing (2)
src/adaptors/curvance/abi.jssrc/adaptors/curvance/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/adaptors/curvance/abi.js
|
The curvance adapter exports pools: Test Suites: 1 passed, 1 total |
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.
Summary by CodeRabbit