feat: enhance APY calculation with dynamic chain mapping#2353
Open
pawell24 wants to merge 3 commits intoDefiLlama:masterfrom
Open
feat: enhance APY calculation with dynamic chain mapping#2353pawell24 wants to merge 3 commits intoDefiLlama:masterfrom
pawell24 wants to merge 3 commits intoDefiLlama:masterfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe accountable adaptor adds a per-call chain ID→name resolver and refactors apy() to fetch chain metadata and accountable data in parallel, group vaults by resolved chain names, compute per-chain vault stats concurrently, and merge those stats for loan APY/TVL calculations. (50 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant apy as apy()
participant ChainAPI as Chain Metadata API
participant AccountAPI as Accountable API
participant StatsEngine as Statistics Engine
Client->>apy: call apy()
par Parallel Fetches
apy->>ChainAPI: request chain ID→name map
apy->>AccountAPI: request vaults & loans
and
ChainAPI-->>apy: chain ID→name map
AccountAPI-->>apy: vaults & loans (with chain_ids)
end
apy->>apy: group vaults by resolved chain name (skip unresolved)
par Per-Chain Stats
apy->>StatsEngine: compute stats for Chain A vaults
apy->>StatsEngine: compute stats for Chain B vaults
apy->>StatsEngine: compute stats for Chain N vaults
and
StatsEngine-->>apy: per-chain vault stats
end
apy->>apy: merge per-chain stats into single map
apy-->>Client: aggregated loans with APY and TVL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 accountable 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
🤖 Fix all issues with AI agents
In `@src/adaptors/accountable/index.js`:
- Around line 131-138: The loop currently assigns vaults to an 'unknown' chain
when chainIdToName lacks item.chain_id, which causes downstream SDK multiCall
failures; update the loop in the block that builds vaultsByChain (referencing
activeLoans, loanVaultMap, chainIdToName, vaultsByChain) to skip any item whose
chainIdToName[item.chain_id] is falsy (do not default to 'unknown'), so only
resolved chain names are lowercased and used as keys; ensure you still guard for
missing vault (if (!vault) continue) and only push unique vaults as before, so
getVaultStats(vaults, chainName) will never be called with an invalid chainName.
- Around line 140-151: The current Promise.all over
Object.entries(vaultsByChain) will reject if any getVaultStats throws, so change
the stats aggregation to use Promise.allSettled on the mapped promises from
Object.entries(vaultsByChain) (the block that produces statsEntries and
vaultStatsByChain); after allSettled, keep only results with status ===
"fulfilled" (extract value [chain, stats]) and log or surface any rejected
reasons, then build vaultStatsByChain via Object.fromEntries of the fulfilled
values and merge into vaultStats as before. Ensure you reference the existing
symbols getVaultStats, statsEntries/vaultStatsByChain, and vaultStats so the
replacement preserves the downstream merging logic.
- Around line 7-20: getChainIdToNameMap currently swallows errors and returns an
empty map which causes downstream grouping to 'unknown' and triggers
sdk.api.abi.multiCall with an invalid chain; update getChainIdToNameMap to log
the caught error (include the error object) and rethrow it (or return null)
instead of returning an empty object so callers can fail fast; also update
callers (e.g., getVaultStats and apy) to check the map result and avoid calling
sdk.api.abi.multiCall with a non-existent chain by early-returning or wrapping
the multiCall in a try/catch that logs the failure and skips that vault/chain.
Ensure you reference getChainIdToNameMap, getVaultStats, apy, and
sdk.api.abi.multiCall when making these changes so the error is both surfaced
and handled safely.
🧹 Nitpick comments (2)
src/adaptors/accountable/index.js (2)
155-155: Inconsistent chain name casing between line 135 and 155.Line 135 lowercases the chain name (
chainIdToName[item.chain_id] || 'unknown').toLowerCase()), but line 155 does not. While it works here because line 200 calls.toLowerCase()on the pool string and line 201 passes throughutils.formatChain, it would be clearer and less error-prone to extract a consistent chain-name resolution once per loan rather than computing it twice with different transforms.
44-51: Remove the unusedgetVaultAddressesFromApifunction.This function is no longer called anywhere in the codebase. Vault discovery is now handled through
fetchVaultsByLoanIdscombined with the per-chain grouping logic.
|
The accountable adapter exports pools: Test Suites: 1 passed, 1 total |
Contributor
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/accountable/index.js (1)
160-219:⚠️ Potential issue | 🟠 MajorLoans with unmapped
chain_idproduce broken pool entries.Lines 133–141 correctly skip vaults whose
chain_idisn't inchainIdToName, but the finalPromise.allon line 160 still iterates over allactiveLoans. WhenchainIdToName[item.chain_id]isundefined(line 162), the resulting pool entry has:
pool:"0x…-undefined"(line 207)chain:utils.formatChain(undefined)(line 208) — likely"undefined"or a crashFilter out unresolvable loans before mapping, consistent with the earlier grouping logic.
Proposed fix
return Promise.all( - activeLoans.map(async(item) => { - const chainName = chainIdToName[item.chain_id]; + activeLoans.filter((item) => { + const vault = loanVaultMap[item.id]; + const chainName = chainIdToName[item.chain_id]; + return vault && chainName; + }).map(async(item) => { + const chainName = chainIdToName[item.chain_id]; const vaultAddress = loanVaultMap[item.id];
🧹 Nitpick comments (1)
src/adaptors/accountable/index.js (1)
88-91: Inconsistentparamsformat forbalanceOfcall.Line 81 wraps the param in an array (
[supplies[i]]), but line 90 passesvaultas a bare value. While the SDK may coerce this, it's inconsistent and fragile.Suggested consistency fix
calls: vaults.map((vault, i) => ({ target: underlyings[i], - params: vault, + params: [vault], })),
|
The accountable adapter exports pools: Test Suites: 1 passed, 1 total |
0xkr3p
approved these changes
Feb 15, 2026
Contributor
|
hi @pawell24, thanks for the PR, these changes look fine. However, I've noticed something which may need looking at, can you check how I got total borrowed > total supplied on a test run: |
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