feat(portfolio): add cross-exchange portfolio summary#164
Conversation
… balances and positions - Add getAccountBalance/getExchangePositions methods to ExchangeClient - Add cross_exchange_summary action: aggregates balances across all CEX exchanges (Binance, Bybit, OKX, Bitget) with asset totals and distribution - Add cross_exchange_positions action: aggregates open positions across exchanges with total unrealized PnL - Add /portfolio slash command - Update portfolio tool description with new actions - Add tests for both cross-exchange actions including error handling Closes #47
ChoKhoOu
left a comment
There was a problem hiding this comment.
COMPLETE
Requirement Verification: PR #164 vs Issue #47 (PRD-045)
All 6 acceptance criteria from the original issue are fully implemented:
| # | Acceptance Criterion | Status | Evidence |
|---|---|---|---|
| 1 | Aggregate balances across exchanges | PASS | cross_exchange_summary action in portfolio.tool.ts queries all 4 exchanges (binance, bybit, okx, bitget) via Promise.allSettled, returns exchangeBalances + aggregatedAssets. Test verifies multi-exchange aggregation. |
| 2 | Aggregate positions across exchanges | PASS | cross_exchange_positions action merges positions from all exchanges with exchange labels. Test verifies positions from binance + bybit are correctly combined. |
| 3 | Total PnL correctly calculated | PASS | totalUnrealizedPnl sums unrealizedPnl across all exchange positions. Test verifies: 500 + (-100) = 400. |
| 4 | Asset distribution percentages | PASS | distribution array in handleCrossExchangeSummary calculates per-exchange percentage based on USDT value. Test verifies binance at 63.16% (6000/9500). |
| 5 | AI conversation queryable | PASS | Tool description in portfolio.ts updated with cross-exchange use cases ("Viewing aggregated balances and positions across all connected exchanges", "Checking total portfolio value and asset distribution across exchanges"). New actions documented in the action table for agent discoverability. |
| 6 | /portfolio slash command |
PASS | Command registered in SLASH_COMMANDS registry, handler added in parseSlashCommand, action type added to SlashAction union. Tests confirm parsing returns { handled: true, action: 'portfolio' }. |
Technical Requirements (from PRD)
- Enhanced
src/tools/consolidated/portfolio.tool.ts— PASS - New actions
cross_exchange_summaryandcross_exchange_positions— PASS - Parallel gRPC queries via
Promise.allSettled— PASS - Graceful error handling per exchange — PASS (tested with mixed success/failure scenarios)
Test Coverage
cross_exchange_summary: aggregation test + error handling testcross_exchange_positions: aggregation test + error handling test/portfolioslash command: parsing test + registry count updated
All requirements from Issue #47 are fully satisfied.
ChoKhoOu
left a comment
There was a problem hiding this comment.
APPROVE
Code Review: feat(portfolio): add cross-exchange portfolio summary
Overall Assessment
Clean, well-structured implementation that follows existing codebase patterns. The use of Promise.allSettled for parallel exchange queries is the correct approach for graceful partial-failure handling. Test coverage is thorough with both happy-path and error scenarios.
Strengths
- Resilient parallel queries:
Promise.allSettledensures one exchange failure doesn't block others — critical for multi-exchange workflows. - Consistent patterns:
ctx.grpc?.exchange ?? getExchangeClient()follows the same DI-with-fallback pattern used throughout the tool layer. - Good test design: Tests inject mocks via
ctx.grpc.exchangerather than module-level state, which is cleaner and avoids inter-test pollution. - Schema + description updated together: New actions are properly registered in both the Zod schema and the AI-facing description, ensuring agent discoverability.
Minor Notes (non-blocking)
-
totalUsdtValueonly sums stablecoins (portfolio.tool.ts:131-133): The field name suggests total portfolio value, but it only aggregates USDT/USDC/USD assets. Non-stablecoin assets (BTC, ETH) are included inaggregatedAssetsbut not in the total or distribution percentages. This is a reasonable V1 approach (proper conversion would require price feeds), but consider renaming tototalStablecoinValueor adding a comment clarifying the limitation. -
Hardcoded
SUPPORTED_EXCHANGESarray (portfolio.tool.ts:6):['binance', 'bybit', 'okx', 'bitget']requires code changes to add exchanges. Consider extracting to a shared constant or reading from daemon config in a future iteration. -
Cross-domain import (
portfolio.tool.ts:4):getExchangeClientis imported from../trading/grpc-clients.js. This creates a portfolio→trading module dependency. Acceptable sinceExchangeClientis shared infrastructure, but worth noting for future refactoring. -
Distribution percentages can be misleading: Since
totalUsdtValueonly counts stablecoins, an exchange holding 10 BTC and 0 USDT would show 0% distribution despite significant value. TheexchangeBalancesarray provides the full picture, so agents can still reason about it correctly.
Verification Checklist
- Proto definitions for
GetAccountBalanceandGetExchangePositionsexist inproto/tino/exchange/v1/exchange.proto - Generated types (
GetAccountBalanceRequestSchema,GetExchangePositionsRequestSchema) are available insrc/grpc/gen/ -
ExchangeClientnew methods match proto service definition -
/portfolioslash command correctly registered inSLASH_COMMANDS,SlashActiontype, parser, andrunExtendedSlashAction - Test count updated (23 → 24 commands)
- Tool description updated for AI discoverability
- Error handling correctly propagates exchange names in error messages
-
symbol ?? ""default aligns with protobuf string default semantics
Summary
cross_exchange_summaryandcross_exchange_positionsactions to the portfolio tool/portfolioslash command for quick cross-exchange portfolio viewCloses #47
Test Plan
bun run typecheckpassesbun testpasses/portfolioslash command triggers cross_exchange_summary action🤖 Generated with Claude Code