feat: make portfolio fetch one request and make LLM smart about connected accounts #128
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughConsolidates portfolio fetching into an aggregated, single POST-based flow that returns network-tagged results; chat system prompt becomes dynamic and wallet-aware; server portfolio tool and route now accept an optional networks array (or derive connected networks) and return per-network portfolio items. Changes
Sequence DiagramsequenceDiagram
participant User
participant ChatRoute as Chat Route
participant PortfolioTool as Portfolio Tool
participant PortfolioRoute as Portfolio Route
participant Service as Portfolio Service
participant API as /api/portfolio
User->>ChatRoute: sends portfolio request
ChatRoute->>ChatRoute: buildSystemPrompt(evm, solana) (wallets + date/time)
ChatRoute->>PortfolioTool: invoke portfolio tool
alt networks provided
PortfolioTool->>PortfolioTool: use explicit networks
else networks omitted
PortfolioTool->>PortfolioTool: call getConnectedNetworks()
end
PortfolioTool->>PortfolioRoute: request portfolio for networks
PortfolioRoute->>PortfolioRoute: validate networksToFetch (error if empty)
PortfolioRoute->>Service: fetchFullPortfolio(evmAddress?, solanaAddress?)
Service->>API: POST /api/portfolio { evmAddress?, solanaAddress? }
API-->>Service: aggregated PortfolioNetworkResult[] (per-network)
Service->>Service: aggregate allAssets, totalBalance, allocations, delta24h
Service-->>PortfolioRoute: PortfolioData (aggregated)
PortfolioRoute-->>PortfolioTool: network-tagged PortfolioDataFull[]
PortfolioTool-->>ChatRoute: respond with per-network portfolio items
ChatRoute-->>User: chat reply with portfolio info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/agentic-server/src/routes/portfolio.ts (2)
57-62: Helper correctly derives EVM/Solana networks but duplicates logic from tools layer
getConnectedNetworkscleanly filters connected wallets down to validEvmSolanaNetworkvalues usingchainIdToNetworkandEVM_SOLANA_NETWORKS, which is exactly what you want for constraining to portfolio-capable networks.The same logic now exists in
apps/agentic-server/src/tools/portfolio.ts; consider centralizing this (e.g., a shared helper inwalletContextSimpleor exporting it from one module) to avoid future drift if supported networks change.
20-27: Clarify intended behavior whennetworksis provided as an empty array
networksToFetchis computed asnetworks || getConnectedNetworks(walletContext). Because[]is truthy, an explicitnetworks: []will result innetworksToFetch.length === 0and a 400"No networks available..."response, instead of falling back to the connected wallets.If the API is never supposed to receive
networks: [], this is fine; otherwise, and if you’d like[]to behave like “omit networks and use connected ones,” you could guard explicitly on length:- const networksToFetch = networks || getConnectedNetworks(walletContext) + const networksToFetch = + networks && networks.length > 0 ? networks : getConnectedNetworks(walletContext)Also applies to: 68-75, 77-78
apps/agentic-server/src/routes/chat.ts (1)
171-181: Dynamic system prompt wiring is correct; consider reusing a singleDateinstanceThe new
buildSystemPromptcorrectly injects the connected-wallet summary, current date/time, and updated portfolio rule (“fetches all connected networks by default”) and is wired intostreamTextviasystem: buildSystemPrompt(evmAddress, solanaAddress).If you care about consistency and micro-efficiency, you could create one
const now = new Date()and reuse it for the formatted date and Unix timestamp instead of callingnew Date()three times.Also applies to: 242-244, 268-268
apps/agentic-chat/src/services/portfolioService.ts (1)
22-27: Consolidated fetch and aggregation logic are consistent with server shapeThe client now expects
PortfolioNetworkResult[](network + balances) and correctly flattens all balances intoPortfolioAsset[], recomputestotalBalance, per-assetallocation, anddelta24hfrom the combined view. Error and non-OK responses both return a clean, emptyPortfolioDataobject, which is a reasonable fallback.You might consider extracting the repeated “empty portfolio” object into a small helper (e.g.,
makeEmptyPortfolio()) to avoid duplication across the early-return, non-OK, and catch paths, but that’s purely a cleanliness tweak.Also applies to: 44-61, 63-98
apps/agentic-server/src/tools/portfolio.ts (1)
13-21: Schema and output types correctly model multi-network portfolios; helper duplicates route logicThe updated
portfolioSchemaandPortfolioOutputnow encode multipleEvmSolanaNetworkentries with per-network balances, which matches the new client-side expectations.
getConnectedNetworkshere mirrors the implementation inroutes/portfolio.ts(filtering connected wallets down to EVM/Solana networks viachainIdToNetwork+EVM_SOLANA_NETWORKS), which is correct but duplicated; consider moving this helper to a shared module to keep supported-network logic in one place.Also applies to: 41-53, 54-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/agentic-chat/src/services/portfolioService.ts(2 hunks)apps/agentic-server/src/routes/chat.ts(3 hunks)apps/agentic-server/src/routes/portfolio.ts(3 hunks)apps/agentic-server/src/tools/portfolio.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/agentic-chat/src/services/portfolioService.ts (4)
packages/types/src/network.ts (1)
EvmSolanaNetwork(61-61)apps/agentic-chat/src/types/portfolio.ts (1)
PortfolioAsset(3-17)apps/agentic-chat/src/lib/bignumber.ts (2)
bnOrZero(5-13)bn(3-3)apps/agentic-chat/src/lib/portfolio.ts (1)
calculate24hDelta(7-39)
apps/agentic-server/src/routes/chat.ts (1)
apps/agentic-server/src/context.ts (1)
supportedChainsContext(1-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: main
🔇 Additional comments (2)
apps/agentic-server/src/routes/chat.ts (1)
161-169: Connected wallets prompt helper looks solidNicely encapsulates wallet display logic and cleanly handles the “no wallets” case while abbreviating addresses for readability; slicing will behave safely even if the input address is shorter than expected.
apps/agentic-server/src/tools/portfolio.ts (1)
61-118: Single-network fetch and caching flow are coherent
getPortfolioDataSinglecleanly maps a logicalnetworkto itschainId, derives the account fromwalletContext, pulls balances viaexecuteGetAccount, enriches them with asset metadata fromexecuteGetAssetsBasic, and computescryptoAmount/usdAmount. The cache is keyed by(account, network), and cached entries are safely normalized to includenetworkwhen returned.Only thing to watch: if
getAddressForNetwork(walletContext, network)can ever returnundefined(e.g., networks passed without a corresponding connected wallet), this will bubble up as an error fromexecuteGetAccount. Make sure all call sites either (a) only pass networks that exist in the wallet context or (b) handle such errors and surface a clear “no wallet for this network” message.
premiumjibles
force-pushed
the
portfolio-fetch
branch
from
068fd7e to
509f925
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/agentic-server/src/tools/portfolio.ts (1)
131-135: Consider a structured error for better orchestration handling.As noted in previous review, when no networks are specified and no wallets are connected, this throws a generic
Error. The chat orchestration layer should catch this and return a user-friendly message. Consider using a custom error type or code to make this easier to identify upstream.+class NoWalletsConnectedError extends Error { + code = 'NO_CONNECTED_WALLETS' + constructor() { + super('No networks specified and no connected wallets found') + } +} if (networks.length === 0) { - throw new Error('No networks specified and no connected wallets found') + throw new NoWalletsConnectedError() }
🧹 Nitpick comments (2)
apps/agentic-chat/src/services/portfolioService.ts (1)
42-57: Consider logging the error response body for better debugging.When the response is not OK, you're logging
response.statusTextbut the server may return a JSON body with more details (e.g., validation errors). Consider parsing and logging the response body for richer diagnostics.if (!response.ok) { - console.error('[Portfolio] Failed to fetch:', response.statusText) + const errorBody = await response.text().catch(() => 'Unable to read body') + console.error('[Portfolio] Failed to fetch:', response.statusText, errorBody) return { assets: [],apps/agentic-server/src/tools/portfolio.ts (1)
120-125: Consider adding error handling for partial failures.
Promise.allwill reject immediately if any single network fails, losing successful results from other networks. Depending on requirements, you may want to usePromise.allSettledto return partial results and surface per-network errors gracefully.export async function getPortfolioData( input: { networks: EvmSolanaNetwork[] }, walletContext?: WalletContext ): Promise<PortfolioDataFull[]> { - return Promise.all(input.networks.map(network => getPortfolioDataSingle(network, walletContext))) + const results = await Promise.allSettled( + input.networks.map(network => getPortfolioDataSingle(network, walletContext)) + ) + return results + .filter((r): r is PromiseFulfilledResult<PortfolioDataFull> => r.status === 'fulfilled') + .map(r => r.value) }Alternatively, if fail-fast is intentional for consistency, add a comment clarifying that behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/agentic-chat/src/services/portfolioService.ts(1 hunks)apps/agentic-chat/src/types/portfolio.ts(0 hunks)apps/agentic-server/src/routes/chat.ts(3 hunks)apps/agentic-server/src/routes/portfolio.ts(2 hunks)apps/agentic-server/src/tools/portfolio.ts(6 hunks)
💤 Files with no reviewable changes (1)
- apps/agentic-chat/src/types/portfolio.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/agentic-server/src/routes/chat.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: main
🔇 Additional comments (10)
apps/agentic-chat/src/services/portfolioService.ts (4)
22-27: LGTM! Type aligns with server response structure.The
PortfolioNetworkResulttype correctly models the aggregated response from the server, including thenetworkfield for per-network context.
59-74: LGTM! Consolidation logic is correct.The flatMap approach correctly aggregates balances from all networks, and the asset mapping properly extracts fields from the nested structure. The fallback to
'0'for missingpriceChange24his appropriate.
76-81: Allocation calculation is correct.The BigNumber-based calculation properly handles division-by-zero by checking
totalBalance.gt(0)before computing percentages.
93-100: Consider logging the full error for debugging.The catch block logs
errordirectly, which is good. However, iferroris not anErrorinstance, the log may be less informative. The current approach is acceptable, but you could add more context.apps/agentic-server/src/routes/portfolio.ts (2)
19-27: Schema validation looks correct.The schema properly allows optional
networksarray with enum validation and maintains the refinement requiring at least one address. This aligns with the multi-network API design.
60-69: LGTM! Multi-network flow is well-structured.The logic correctly:
- Extracts optional
networksfrom the validated request- Falls back to
getConnectedNetworks(walletContext)when not provided- Guards against empty networks with a clear 400 response
This provides good flexibility for clients to either specify networks explicitly or rely on connected wallet derivation.
apps/agentic-server/src/tools/portfolio.ts (4)
13-18: LGTM! Schema correctly defines multi-network input.Using
z.enum(EVM_SOLANA_NETWORKS)ensures only valid network values are accepted, and the optional array with descriptive.describe()clearly communicates the API contract.
54-59: LGTM! Network derivation logic is correct.The function properly:
- Guards against missing wallet context
- Maps chain IDs to networks using the lookup table
- Filters to ensure only valid
EvmSolanaNetworkvalues are returnedThe type guard
(n): n is EvmSolanaNetworkcorrectly narrows the type.
139-150: LGTM! Output mapping is clean and correct.The mapping correctly transforms
PortfolioDataFull[]toPortfolioOutputby extracting only the necessary fields and including thenetworkfield in each entry for client-side disambiguation.
153-158: Tool description is clear and accurate.The updated description correctly indicates that omitting
networkswill fetch all connected wallets, aligning with the implementation behavior.
Summary by CodeRabbit
New Features
Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.