Extract active protocols dynamically rather than statically for Nolus Protocol#2321
Open
metodi96 wants to merge 6 commits intoDefiLlama:masterfrom
Open
Extract active protocols dynamically rather than statically for Nolus Protocol#2321metodi96 wants to merge 6 commits intoDefiLlama:masterfrom
metodi96 wants to merge 6 commits intoDefiLlama:masterfrom
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:
📝 WalkthroughWalkthroughThe Nolus Protocol adaptor now sources active protocols and currency decimals from the ETL API instead of hard-coded definitions; Changes
Sequence DiagramsequenceDiagram
participant getApy as getApy()
participant ETL as ETL API
participant Pool as Pool Data Source
participant Calc as Calc Logic
getApy->>ETL: fetchActiveProtocols()
ETL-->>getApy: active protocol descriptors (lpp, oracle, symbol, protocolName)
getApy->>ETL: fetchCurrencyDecimals()
ETL-->>getApy: ticker → decimals map
loop for each protocol
getApy->>Pool: request pool data (using lpp/oracle)
Pool-->>getApy: pool balances, rates
getApy->>Calc: compute price & TVL (use decimals map & getDecimalAdjustment)
Calc-->>getApy: adjusted price & APY
end
getApy->>getApy: aggregate results and return APY list
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 |
|
The nolus-protocol 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: 1
🤖 Fix all issues with AI agents
In `@src/adaptors/nolus-protocol/index.js`:
- Around line 34-40: The protocols.map projection directly accesses nested
properties p.contracts.lpp and p.contracts.oracle which will throw if contracts
is null/undefined; update the mapping in the protocols.map callback (the code
creating objects with lpp/oracle/symbol/protocolName/meta) to defensively read
contracts (e.g., const c = p.contracts || {}; or use optional chaining) and
assign lpp and oracle from that safe reference so missing contracts produce
undefined (or a sensible default string) instead of throwing.
🧹 Nitpick comments (2)
src/adaptors/nolus-protocol/index.js (2)
71-75: Consider parallelizing independent API calls.
fetchActiveProtocols()andfetchCurrencyDecimals()are independent and could be fetched concurrently to reduce latency.⚡ Proposed optimization
- // Fetch active protocols dynamically from ETL - const contracts = await fetchActiveProtocols(); - - // Fetch currency decimals for dynamic adjustment - const currencyDecimals = await fetchCurrencyDecimals(); + // Fetch active protocols and currency decimals concurrently + const [contracts, currencyDecimals] = await Promise.all([ + fetchActiveProtocols(), + fetchCurrencyDecimals(), + ]);
94-97: Prefer strict equality operator.Use
===instead of==for type-safe comparison.♻️ Proposed fix
let currencyData = _.find( oracleCurrenciesData.data, - (n) => n.ticker == lppTickerData.data, + (n) => n.ticker === lppTickerData.data, );
|
The nolus-protocol adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adaptors/nolus-protocol/index.js`:
- Around line 62-70: The code currently falls back to stable decimals using the
logical OR operator which treats 0 as falsy; update the decimal fallback to use
nullish coalescing (??) so only null/undefined trigger the default. In practice,
modify getDecimalAdjustment usage and any other places (e.g., the block around
lines referenced 132-134) to compute lpnDecimals = (currency.decimal_digits ??
STABLE_QUOTE_DECIMALS) and then call getDecimalAdjustment(lpnDecimals), and
replace any other patterns like x || STABLE_QUOTE_DECIMALS with x ??
STABLE_QUOTE_DECIMALS to preserve a possible zero value.
|
The nolus-protocol adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adaptors/nolus-protocol/index.js`:
- Around line 62-70: getDecimalAdjustment currently returns 1 whenever
lpnDecimals < STABLE_QUOTE_DECIMALS which skips the required downscaling and
inflates price/TVL; update the function (and the duplicate logic at the other
occurrence around lines 131-134) to always return Math.pow(10, decimalDiff)
(where decimalDiff = lpnDecimals - STABLE_QUOTE_DECIMALS) so negative
differences correctly produce fractional adjustment factors, and ensure callers
use that factor for price/TVL scaling.
Collaborator
|
@metodi96 pls change the symbols to USDC, instead of USDC_NOBLE -> can add Noble to poolMeta. (same for other pools) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/adaptors/nolus-protocol/index.js (2)
165-165: Misleading comment.The comment references "USDT address on ethereum" but this adapter is for the Nolus chain, and
bank_symbol/dex_symbolare likely not addresses. Consider updating or removing the comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/nolus-protocol/index.js` at line 165, The inline comment for underlyingTokens is misleading—update or remove the phrase "eg here USDT address on ethereum" and clarify that underlyingTokens holds symbol identifiers (currencyData.bank_symbol, currencyData.dex_symbol) for the Nolus adapter rather than Ethereum addresses; ensure the comment correctly reflects the Nolus chain and the actual data type (symbols vs. addresses) next to the underlyingTokens: [currencyData.bank_symbol, currencyData.dex_symbol] line.
98-110: Consider parallelizing contract queries for better performance.The loop makes 4 sequential
queryContractcalls per protocol. With 8+ protocols, this adds latency. Since each iteration's queries are independent of other iterations, you could batch them.♻️ Optional: Parallelize with Promise.all
- for (let i = 0; i < contracts.length; i++) { - const c = contracts[i]; - let lppTickerData = await queryContract(c.lpp, { lpn: [] }); - let oracleCurrenciesData = await queryContract(c.oracle, { - currencies: {}, - }); - let oraclePriceData = await queryContract(c.oracle, { - stable_price: { currency: lppTickerData.data }, - }); + const results = await Promise.all( + contracts.map(async (c) => { + const lppTickerData = await queryContract(c.lpp, { lpn: [] }); + const [oracleCurrenciesData, oraclePriceData, lppBalanceData] = await Promise.all([ + queryContract(c.oracle, { currencies: {} }), + queryContract(c.oracle, { stable_price: { currency: lppTickerData.data } }), + queryContract(c.lpp, { lpp_balance: [] }), + ]); + return { c, lppTickerData, oracleCurrenciesData, oraclePriceData, lppBalanceData }; + }) + ); + + for (const { c, lppTickerData, oracleCurrenciesData, oraclePriceData, lppBalanceData } of results) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/nolus-protocol/index.js` around lines 98 - 110, The four sequential queryContract calls inside the for loop (using variables lppTickerData, oracleCurrenciesData, oraclePriceData, currencyData) should be parallelized: for each contract c call the independent queries concurrently with Promise.all (or map to an async worker and await Promise.all over contracts) and then destructure the results to compute currencyData (using _.find) — this keeps the same logic but removes per-contract sequential waits and batches work across contracts to reduce latency.
🤖 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/nolus-protocol/index.js`:
- Line 165: The inline comment for underlyingTokens is misleading—update or
remove the phrase "eg here USDT address on ethereum" and clarify that
underlyingTokens holds symbol identifiers (currencyData.bank_symbol,
currencyData.dex_symbol) for the Nolus adapter rather than Ethereum addresses;
ensure the comment correctly reflects the Nolus chain and the actual data type
(symbols vs. addresses) next to the underlyingTokens: [currencyData.bank_symbol,
currencyData.dex_symbol] line.
- Around line 98-110: The four sequential queryContract calls inside the for
loop (using variables lppTickerData, oracleCurrenciesData, oraclePriceData,
currencyData) should be parallelized: for each contract c call the independent
queries concurrently with Promise.all (or map to an async worker and await
Promise.all over contracts) and then destructure the results to compute
currencyData (using _.find) — this keeps the same logic but removes per-contract
sequential waits and batches work across contracts to reduce latency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/adaptors/nolus-protocol/index.js (1)
104-127: Consider parallelizing per-pool RPC calls to reduce adapter latency.Line 106 through Line 127 executes multiple network calls serially per protocol. A small
Promise.allrefactor would improve runtime and reduce timeout exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adaptors/nolus-protocol/index.js` around lines 104 - 127, The loop over contracts calls queryContract serially (variables/functions: queryContract, lppTickerData, oracleCurrenciesData, oraclePriceData, lppBalanceData) which increases latency; refactor to run independent RPCs in parallel with Promise.all: first kick off queryContract(c.lpp, { lpn: [] }) and queryContract(c.oracle, { currencies: {} }) in parallel and await both, then once lppTickerData is available start the dependent oracle price query (queryContract(c.oracle, { stable_price: { currency: lppTickerData.data } })) together with the lpp balance query (queryContract(c.lpp, { lpp_balance: [] })) using Promise.all, then proceed with the existing currencyData checks and continue; ensure you preserve variable names and null checks when unpacking Promise.all results.
🤖 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/nolus-protocol/index.js`:
- Around line 49-59: The pool mapping uses a display-normalized symbol for both
presentation and math; add a canonical LPN ticker field (e.g., lpnSymbol or
canonicalSymbol) set to p.lpn_symbol when building the object in the map
(alongside the existing display-only symbol), and then update any downstream
decimal lookups (the places that index currencyDecimals currently with c.symbol)
to use that new canonical field (e.g., c.lpnSymbol) for all decimal/math
calculations so presentation aliases don't change decimal factors.
---
Nitpick comments:
In `@src/adaptors/nolus-protocol/index.js`:
- Around line 104-127: The loop over contracts calls queryContract serially
(variables/functions: queryContract, lppTickerData, oracleCurrenciesData,
oraclePriceData, lppBalanceData) which increases latency; refactor to run
independent RPCs in parallel with Promise.all: first kick off
queryContract(c.lpp, { lpn: [] }) and queryContract(c.oracle, { currencies: {}
}) in parallel and await both, then once lppTickerData is available start the
dependent oracle price query (queryContract(c.oracle, { stable_price: {
currency: lppTickerData.data } })) together with the lpp balance query
(queryContract(c.lpp, { lpp_balance: [] })) using Promise.all, then proceed with
the existing currencyData checks and continue; ensure you preserve variable
names and null checks when unpacking Promise.all results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e2bfa72-d563-4bec-aa74-402642b8174c
📒 Files selected for processing (1)
src/adaptors/nolus-protocol/index.js
Comment on lines
+49
to
+59
| .filter((p) => p.contracts?.lpp && p.contracts?.oracle) | ||
| .map((p) => { | ||
| const override = symbolMeta[p.lpn_symbol]; | ||
| return { | ||
| lpp: p.contracts.lpp, | ||
| oracle: p.contracts.oracle, | ||
| symbol: override ? override.symbol : p.lpn_symbol, | ||
| protocolName: p.name, | ||
| meta: override ? override.meta : '', | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
Use canonical LPN ticker for decimal math; keep symbol as display-only.
Line 149 currently indexes currencyDecimals with c.symbol, but c.symbol is normalized for presentation (e.g., ALL_BTC -> BTC, ALL_SOL -> SOL). That can apply the wrong decimal factor and skew price/TVL for aliased pools.
🔧 Proposed fix
return protocols
.filter((p) => p.contracts?.lpp && p.contracts?.oracle)
.map((p) => {
const override = symbolMeta[p.lpn_symbol];
return {
lpp: p.contracts.lpp,
oracle: p.contracts.oracle,
+ lpnTicker: p.lpn_symbol,
symbol: override ? override.symbol : p.lpn_symbol,
protocolName: p.name,
meta: override ? override.meta : '',
};
});
@@
- const lpnDecimals = currencyDecimals[c.symbol] ?? STABLE_QUOTE_DECIMALS;
+ const lpnTicker = lppTickerData?.data || c.lpnTicker;
+ const lpnDecimals =
+ currencyDecimals[lpnTicker] ??
+ currencyData?.decimal_digits ??
+ STABLE_QUOTE_DECIMALS;
const decimalsAdjustment = getDecimalAdjustment(lpnDecimals);Also applies to: 149-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adaptors/nolus-protocol/index.js` around lines 49 - 59, The pool mapping
uses a display-normalized symbol for both presentation and math; add a canonical
LPN ticker field (e.g., lpnSymbol or canonicalSymbol) set to p.lpn_symbol when
building the object in the map (alongside the existing display-only symbol), and
then update any downstream decimal lookups (the places that index
currencyDecimals currently with c.symbol) to use that new canonical field (e.g.,
c.lpnSymbol) for all decimal/math calculations so presentation aliases don't
change decimal factors.
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
3 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