refactor(data): unify market data flow to Python daemon via gRPC#166
refactor(data): unify market data flow to Python daemon via gRPC#166
Conversation
ChoKhoOu
left a comment
There was a problem hiding this comment.
COMPLETE
Requirement Verification: PR #166 vs Issue #50 (PRD-048)
All acceptance criteria from Issue #50 are fully met.
Acceptance Criteria Checklist
- New CEX data sources implemented in Python side:
DataServiceServicerinpython/tino_daemon/services/data.pyadds 4 new gRPC methods (GetMarketQuote,GetMarketKlines,GetMarketOverview,ListSupportedExchanges) that delegate to Python-side exchange connectors viaget_connector(exchange). - TS tools call Python via gRPC for data:
market-data-router.tsroutes newcrypto_exchange_quote,crypto_exchange_klines,crypto_exchange_overview, andcrypto_supported_exchangesactions throughDataClientgRPC calls. Thedata-client.tsexposes typed methods matching the new proto RPCs. - Exchange adapters only implemented once (Python side): The TS side creates zero new exchange-specific adapters. All exchange logic lives in Python connectors accessed via
get_connector(). TS is purely a gRPC consumer. - Data format unified in Python side: Protobuf messages
MarketQuoteandMarketKlinedefine the canonical format. The Python_to_market_quote()helper converts exchange-specific data into the unified protobuf format.
Gradual Migration Pattern
- Old TS-side crypto providers (
binance-public,bybit,okx) are preserved but marked with@deprecatedJSDoc annotations - New
crypto_exchange_*actions coexist alongside existing actions (e.g.,crypto_prices,crypto_market_data) - No existing functionality is removed — only new paths added
Coverage
- Proto layer: 4 new RPCs with proper request/response messages (61 additions to
data.proto) - Python service: Full implementation with input validation, error handling (ValueError/NotImplementedError/generic), and proper gRPC status codes
- Python tests: 6 new test cases covering success/error paths for all 4 methods (192 lines)
- TS client: 4 new typed methods in
DataClient - TS router: 4 new case branches with
exchangevalidation andbigintJSON serialization support - TS tests: 4 new test cases verifying gRPC routing for each action (104 lines)
- Tool schema: Updated with new actions,
exchangeandintervalparameters - Descriptions: Documentation updated with new actions table and usage notes
ChoKhoOu
left a comment
There was a problem hiding this comment.
APPROVE
Code Review: PR #166 — Unify Market Data Flow to Python Daemon via gRPC
Overview
This PR implements Phase 1 of PRD-048 (Issue #50) by adding 4 new gRPC RPCs to the DataService (GetMarketQuote, GetMarketKlines, GetMarketOverview, ListSupportedExchanges), wiring them through the Python daemon's exchange connector infrastructure, and exposing them as new crypto_exchange_* actions in the TS market-data tool. The gradual migration strategy is well-executed — existing TS finance providers are preserved with deprecation markers, while new data flows route through the daemon.
Strengths
- Clean proto design: Messages are well-named, field types are appropriate (
doublefor prices,int64for timestamps on klines), and field numbers don't conflict with existing definitions. - Solid error handling in Python service: Three-tier error mapping (
ValueError->INVALID_ARGUMENT,NotImplementedError->UNIMPLEMENTED,Exception->INTERNAL) with a shared_set_errorhelper for consistency. GetMarketOverviewusesasyncio.gather: Parallel ticker fetching is the right call for batch queries.- BigInt serializer in
fmt(): Correctly handlesint64fields from protobuf that becomebigintin TypeScript. - Test coverage: Both Python and TS sides have good coverage including error paths (invalid exchange, negative limit). TS tests use
spyOnwith properfinallycleanup. - Deprecation markers:
@deprecatedJSDoc on binance-public, bybit, okx providers is the right approach for gradual migration.
Suggestions (non-blocking)
-
_to_market_quotetype annotation (python/tino_daemon/services/data.py):
Thetickerparameter is typed asobject, forcing the use ofgetattr(ticker, "symbol")etc. Since this is always called with aTickerdataclass, typing it asTickerwould improve readability and enable IDE support:@staticmethod def _to_market_quote(exchange: str, ticker: Ticker) -> data_pb2.MarketQuote: return data_pb2.MarketQuote( exchange=exchange, symbol=ticker.symbol, ... )
-
GetMarketOverviewpartial failure: If one symbol in theasyncio.gathercall fails, the entire request fails. For robustness, considerreturn_exceptions=Trueand filtering out exceptions, returning partial results. Not required for Phase 1, but worth noting for future iterations. -
Timestamp type inconsistency in proto:
MarketQuote.timestampisstringwhileMarketKline.open_time/close_timeareint64. This is pragmatic (matches upstream exchange formats), but a brief comment in the proto file explaining the design choice would help future contributors.
Architecture Alignment
The PR correctly follows the gradual migration strategy outlined in PRD-048:
- New CEX data sources route through Python daemon
- TS tool layer calls Python via gRPC
- Existing TS providers preserved but marked deprecated
- Exchange adapter implemented once (Python side only)
Verdict
Clean implementation with proper error handling, good test coverage, and correct architecture alignment. The suggestions above are non-blocking improvements. APPROVE.
Summary
proto/tino/data/v1/data.protowith new RPCs:GetMarketQuote,GetMarketKlines,GetMarketOverview,ListSupportedExchangesDataServiceServicerdelegating to exchange connectorsCloses #50
Test Plan
buf lintpasses on proto changesbun run typecheckpassesbun testpassescd python && uv run pytestpasses🤖 Generated with Claude Code