Conversation
📝 WalkthroughWalkthroughRefactoring extracts balance-fetching logic into a reusable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/handlers/private-api.ts (1)
1777-1946: Validation errors fromfetchChainBalance()are returned as 502 bybalance()(should be 400).
fetchChainBalance()throws for invalid chain/unsupported chain/invalid address, butbalance()maps non-Axios errors to 502, so client input errors become “Bad Gateway”.Proposed fix (minimal, message-based 400 mapping)
export const balance = async (req: express.Request, res: express.Response) => { const { chain, address } = req.params; if (!chain || !address) { res.status(400).send("Missing chain or address"); return; } const provider = parseBalanceProvider(req.query.provider); // Slightly extend server timeouts for BTC path const normalizedChain = chain.toLowerCase(); if (normalizedChain === "btc") { const extendedTimeout = BITCOIN_RPC_TIMEOUT_MS + 30_000; if (typeof req.setTimeout === "function") req.setTimeout(extendedTimeout); if (typeof res.setTimeout === "function") res.setTimeout(extendedTimeout); } try { const balanceResponse = await fetchChainBalance(chain, address, provider); // Set response headers res.setHeader("x-provider", balanceResponse.provider); if ( balanceResponse.provider === "blockstream" && balanceResponse.rateLimitRemaining !== undefined ) { res.setHeader( "x-blockstream-ratelimit-remaining", String(balanceResponse.rateLimitRemaining), ); } res.status(200).json(balanceResponse); } catch (err) { console.error("balance(): error while fetching chain balance", err); if (axios.isAxiosError(err) && err.response) { const { status, data } = err.response; if (data !== undefined) { if (data !== null && typeof data === "object") { res.status(status).json(data); } else { res.status(status).json({ error: String(data) }); } } else { res.sendStatus(status); } return; } + // Client input errors thrown by fetchChainBalance() + const msg = err instanceof Error ? err.message : "Unknown error"; + if ( + msg === "Invalid chain parameter" || + msg === "Unsupported chain" || + msg === "Invalid address format" || + msg === "Missing chain or address" + ) { + res.status(400).json({ error: msg }); + return; + } + // Final catch-all - const msg = err instanceof Error ? err.message : "Unknown error"; res.status(502).json({ error: msg }); } };
🤖 Fix all issues with AI agents
In @src/server/handlers/private-api.ts:
- Around line 1777-1781: The fetchChainBalance function signature references the
BalanceProvider type but it’s not imported; update the import list that brings
in helpers (where other types are imported) to also import BalanceProvider
(ensuring BalanceProvider is exported from the ../helper module), so the
function declaration export const fetchChainBalance(... provider:
BalanceProvider ...) compiles correctly. Ensure any renamed or moved type in
../helper is referenced correctly (BalanceProvider) and run the TS build to
verify no other missing type imports remain.
In @src/server/handlers/private-api.ts.backup:
- Around line 1-2896: The file src/server/handlers/private-api.ts.backup is an
accidental backup duplicate and must be removed from the PR; delete the file
from the repository (git rm --cached or git rm if not needed locally), commit
the deletion, and push the change so it’s not included in the branch, and search
the tree for any remaining “.backup” copies or references (e.g.,
private-api.ts.backup) to ensure no stray duplicates remain; also run the
test/build to confirm no code imports the .backup file before finalizing the
commit.
🧹 Nitpick comments (1)
src/server/handlers/wallet-api.ts (1)
1647-1674:processWithConcurrencyLimit()should guardconcurrencyLimitand could live in a shared util.
Current call-site uses3, but<= 0would break (Promise.raceon an empty set / always-true condition).Proposed fix
const processWithConcurrencyLimit = async <T, R>( items: T[], processor: (item: T) => Promise<R>, concurrencyLimit: number ): Promise<R[]> => { + if (!Number.isInteger(concurrencyLimit) || concurrencyLimit <= 0) { + throw new Error("concurrencyLimit must be a positive integer"); + } const results: R[] = []; const executing: Set<Promise<void>> = new Set();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/handlers/private-api.tssrc/server/handlers/private-api.ts.backupsrc/server/handlers/wallet-api.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/handlers/wallet-api.ts (2)
src/server/handlers/private-api.ts (2)
fetchChainBalance(1777-1890)balance(1892-1947)src/server/handlers/constants.ts (1)
ASSET_ICON_URLS(72-87)
src/server/handlers/private-api.ts (2)
src/server/helper.ts (5)
BalanceProvider(9-9)ChainBalanceResponse(11-19)fetchBlockstreamBalance(238-294)fetchChainzBalance(296-352)parseBalanceProvider(145-153)src/server/handlers/fallback.tsx (1)
req(5-8)
🔇 Additional comments (3)
src/server/handlers/private-api.ts (1)
1777-1890:fetchChainBalance()extraction + BTC fallback flow looks solid.
Nice separation of pure “fetch balance” logic from HTTP response handling; BTC fallback ordering is clear and bounded.src/server/handlers/wallet-api.ts (2)
6-7: DirectfetchChainBalance()usage is a good change (removes proxy overhead).
This should reduce double-timeouts / proxy 502s compared to bouncing through the HTTP balance endpoint.
1688-1738: Bounded concurrency + per-wallet fallback behavior looks correct.
Catching per-wallet failures and returning a “0 balance + error message” item keeps the portfolio responsive.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/handlers/private-api.ts (1)
1798-1805: Consider clarifying provider fallback behavior to callers.When a caller specifies
provider=chainzfor a non-BTC chain, the warning is only logged server-side. The caller receives a successful response from Chainstack without knowing their provider hint was ignored. This is acceptable since the response includesprovider: "chainstack"in the payload, but consider whether throwing an error would be more explicit for misconfigured clients.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/handlers/private-api.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/handlers/private-api.ts (2)
src/server/helper.ts (4)
BalanceProvider(9-9)ChainBalanceResponse(11-19)fetchBlockstreamBalance(238-294)fetchChainzBalance(296-352)src/server/handlers/fallback.tsx (1)
req(5-8)
🔇 Additional comments (9)
src/server/handlers/private-api.ts (9)
1774-1791: LGTM - Clean extraction of balance-fetching logic into reusable function.The function signature and initial validation are well-designed. Throwing errors instead of manipulating HTTP responses makes this function reusable from both HTTP endpoints and internal callers (like the wallet API).
1807-1825: Fallback helper design is appropriate.The null-returning pattern allows clean sequential fallback attempts. Logging errors server-side provides observability into which providers failed.
1827-1830: Address validation logic is correct.The optional validation pattern is appropriate since not all chains have client-side address validators defined.
1832-1839: Direct provider routing for explicit BTC requests is correct.Errors from these explicit provider calls will propagate appropriately to the caller's error handler.
1841-1870: Node discovery with BTC fallback handling is well-implemented.The fallback sequence (Blockstream → Chainz) is consistently applied both when node discovery fails and when no suitable node is found.
1872-1891: Balance fetch with BTC fallback logic is correct.The error logging before fallback attempts provides good observability for debugging provider issues.
1893-1909: Clean endpoint implementation with appropriate timeout handling.The defensive check for
setTimeoutavailability is good practice, and extending the timeout for BTC requests accounts for the potentially slowscantxoutsetRPC calls.
1911-1926: Response handling is correct.Headers are set appropriately based on the provider used, and the rate limit header for Blockstream provides useful client-side observability.
1927-1948: Error handling is comprehensive and appropriate.The logic correctly distinguishes between Axios errors (preserving upstream status/data) and other errors (returning 502 Bad Gateway). This provides good error transparency to clients.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.