Add OroSwap yield adapter#2745
Open
permapod-rsk wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughA new ChangesOroswap Fee-Based Yield Adaptor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/adaptors/oroswap/index.js`:
- Around line 56-58: The fallback value for total on line 56 in the OROSwap
adaptor is set to pools.length when data.total is unavailable, but pools.length
only represents the current page size, not the total count of all pages. This
causes the pagination loop to exit prematurely after the first full page since
offset will exceed the fallback total value. Replace the fallback from
pools.length to Infinity (or another sentinel value that won't prematurely
terminate pagination) to ensure that pagination continues based on the
page.length < PAGE_SIZE condition rather than relying on an inaccurate total
count.
- Around line 47-51: The fetchPools() function has two error handling gaps that
cause instability. First, the axios.get() call within the while loop at lines
47-51 lacks error handling; wrap this call and the entire loop in a try-catch
block to gracefully handle 403 and network errors instead of letting them
propagate up to the apy() function. Second, the fallback pagination logic at
line 56 where total is set to pools.length causes premature loop termination
when the first API response contains exactly PAGE_SIZE items (e.g., 150 items),
making the while condition false on the next iteration; fix this by either
continuing pagination when the current batch equals PAGE_SIZE, or implementing a
fallback that doesn't assume pagination has completed based on the current pool
count. Ensure both the error handling and pagination logic properly account for
incomplete data responses.
🪄 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: ec1790d3-60b8-48f5-a995-48f4aceb0c20
📒 Files selected for processing (1)
src/adaptors/oroswap/index.js
|
The oroswap adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/adaptors/oroswap/index.js (4)
67-67: 💤 Low valueInconsistent object key quoting may cause issues.
The IBC hash
CC5268F89C752A4BDCBDAA574AF0A381786FCC839104E077DA9A9145176BF8EDis unquoted while all other IBC hashes are quoted strings. JavaScript allows this because it starts with letters, but it's inconsistent and could lead to confusion or bugs if someone copies this pattern for a hash starting with digits.🔧 Suggested fix for consistency
- CC5268F89C752A4BDCBDAA574AF0A381786FCC839104E077DA9A9145176BF8ED: { + 'CC5268F89C752A4BDCBDAA574AF0A381786FCC839104E077DA9A9145176BF8ED': {🤖 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 `@src/adaptors/oroswap/index.js` at line 67, The object key with the IBC hash CC5268F89C752A4BDCBDAA574AF0A381786FCC839104E077DA9A9145176BF8ED is unquoted while all other IBC hash keys in the object are quoted strings. Add quotes around this hash key to make it consistent with the rest of the object properties and follow JavaScript object literal conventions.
420-431: 💤 Low valueSilent error swallowing may hide persistent RPC issues.
The catch block swallows errors without any logging. While this keeps the adapter functional when individual pools fail, it makes it difficult to detect persistent RPC problems or misconfigured pools.
💡 Optional: Add minimal error logging
} catch (e) { - // Keep the adapter usable if one RPC tx_search call times out. + // Keep the adapter usable if one RPC tx_search call times out. + console.warn(`[oroswap] Fee stats failed for ${pool.pair.contract_addr}: ${e.message}`); }🤖 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 `@src/adaptors/oroswap/index.js` around lines 420 - 431, The catch block in the mapLimit function that processes pools is silently swallowing errors without any logging, making it difficult to detect persistent RPC issues or misconfigured pools. Add minimal error logging within the empty catch block to log the error details along with the pool identifier (pool.pair.contract_addr) so that RPC timeouts and other failures can be monitored and debugged while still maintaining adapter functionality when individual pools fail.
479-479: 💤 Low valueVariable
denomsshadows outer scope declaration.The
const denomson this line shadows thedenomsvariable declared on line 461. While the outer variable is not used after the price fetching, this shadowing can cause confusion during maintenance.🔧 Consider renaming for clarity
- const denoms = assets.map((asset) => asset.denom); + const poolDenoms = assets.map((asset) => asset.denom); const stats = feeStats.get(pair.contract_addr); const apyBase = stats?.day?.feesUsd && tvlUsd > 0 ? (stats.day.feesUsd * 365 * 100) / tvlUsd : 0; const mapped = { pool: `${pair.contract_addr}-${CHAIN}`.toLowerCase(), chain: CHAIN, project: PROJECT, - symbol: utils.formatSymbol(getPoolSymbol(denoms)), + symbol: utils.formatSymbol(getPoolSymbol(poolDenoms)), tvlUsd, apyBase, - underlyingTokens: denoms, + underlyingTokens: poolDenoms,🤖 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 `@src/adaptors/oroswap/index.js` at line 479, The const denoms variable declared on line 479 that maps asset objects to their denom properties shadows the outer denoms variable declared on line 461, which can cause confusion during code maintenance. Rename the inner denoms variable on line 479 (the one created from the assets.map call) to a more descriptive name that clarifies its purpose, such as assetDenoms or denominationList, to eliminate the shadowing and improve code clarity.
109-122: ⚡ Quick winCustom
withRetryretries all errors, unlikeutils.withRetry.The repository's
utils.withRetry(shown in relevant snippets) only retries transient failures (HTTP 429 and 5xx), failing fast on permanent 4xx errors. This local implementation retries all errors unconditionally, which could:
- Waste time retrying permanent failures (400, 404, etc.)
- Mask configuration issues that should fail fast
Consider using
utils.withRetryfor consistency, or add status-code filtering if the broader retry behavior is intentional.♻️ Option: Use the shared utility
-const withRetry = async (fn, attempts = 3) => { - let lastError; - - for (let i = 0; i < attempts; i++) { - try { - return await fn(); - } catch (error) { - lastError = error; - if (i < attempts - 1) await sleep(500 * (i + 1)); - } - } - - throw lastError; -}; +const withRetry = (fn) => utils.withRetry(fn, { retries: 3, delayMs: 500 });🤖 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 `@src/adaptors/oroswap/index.js` around lines 109 - 122, Replace the custom withRetry function with the shared utils.withRetry utility to ensure consistent retry behavior across the codebase. The local implementation unconditionally retries all errors including permanent failures like 4xx status codes, whereas utils.withRetry correctly filters to only retry transient failures such as HTTP 429 and 5xx errors. Update all calls to the local withRetry to use utils.withRetry instead, and remove the custom implementation from this file.
🤖 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.
Nitpick comments:
In `@src/adaptors/oroswap/index.js`:
- Line 67: The object key with the IBC hash
CC5268F89C752A4BDCBDAA574AF0A381786FCC839104E077DA9A9145176BF8ED is unquoted
while all other IBC hash keys in the object are quoted strings. Add quotes
around this hash key to make it consistent with the rest of the object
properties and follow JavaScript object literal conventions.
- Around line 420-431: The catch block in the mapLimit function that processes
pools is silently swallowing errors without any logging, making it difficult to
detect persistent RPC issues or misconfigured pools. Add minimal error logging
within the empty catch block to log the error details along with the pool
identifier (pool.pair.contract_addr) so that RPC timeouts and other failures can
be monitored and debugged while still maintaining adapter functionality when
individual pools fail.
- Line 479: The const denoms variable declared on line 479 that maps asset
objects to their denom properties shadows the outer denoms variable declared on
line 461, which can cause confusion during code maintenance. Rename the inner
denoms variable on line 479 (the one created from the assets.map call) to a more
descriptive name that clarifies its purpose, such as assetDenoms or
denominationList, to eliminate the shadowing and improve code clarity.
- Around line 109-122: Replace the custom withRetry function with the shared
utils.withRetry utility to ensure consistent retry behavior across the codebase.
The local implementation unconditionally retries all errors including permanent
failures like 4xx status codes, whereas utils.withRetry correctly filters to
only retry transient failures such as HTTP 429 and 5xx errors. Update all calls
to the local withRetry to use utils.withRetry instead, and remove the custom
implementation from this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 980d1010-fae7-4c7a-8a8c-a5df5d0748b0
📒 Files selected for processing (1)
src/adaptors/oroswap/index.js
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
1 participant
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