Add Kraken exchange adapter and refactor multi-exchange architecture#28
Merged
Conversation
- Added KrakenAdapter class to handle WebSocket connections and order book updates from Kraken. - Introduced kraken_utils.hpp for utility functions related to Kraken data processing. - Created SymbolNormalizer class to manage symbol mappings between exchange-specific formats and canonical formats. - Updated feed_handler.hpp to include SymbolNormalizer for symbol conversion. - Enhanced OrderBook class with applyDelta method for processing incremental updates. - Added tests for Kraken utilities and symbol normalization to ensure correctness. - Updated main.cpp to support selecting between Binance and Kraken exchanges based on configuration.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Added BinanceAdapter class to handle WebSocket connections and order book updates from Binance. - Integrated SymbolNormalizer for symbol mapping between Binance and canonical formats. - Updated main application logic to support multiple exchanges (Binance and Kraken) with dynamic configuration. - Enhanced MetricsServer to serve metrics for multiple exchanges. - Refactored SubscriberServer to handle order books keyed by exchange and symbol. - Improved error handling and validation for configuration inputs. - Added unit tests for Kraken utility functions.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors single-exchange architecture to support multiple exchanges: replaces feed_handler with exchange-specific adapters (Binance, Kraken), introduces per-exchange runtimes, updates subscription/metrics servers to use composite exchange.symbol keys, and adjusts related utilities and build stubs. (49 words) Changes
Sequence DiagramsequenceDiagram
participant Main as Main (main.cpp)
participant Bin as BinanceAdapter
participant Krk as KrakenAdapter
participant Norm as SymbolNormalizer
participant OB as OrderBook
participant Sub as SubscriberServer
participant Met as MetricsServer
Main->>Main: Parse exchanges config
Main->>Norm: Create/seed normalizer per exchange
Main->>Bin: Instantiate BinanceAdapter (per-exchange)
Main->>Krk: Instantiate KrakenAdapter (per-exchange)
loop Per-Exchange Startup
Bin->>OB: fetchAndApplySnapshot(binanceSym, canonical)
OB->>OB: update book state
Bin->>Sub: updateCallback("exchange.symbol", snapshot)
Krk->>OB: apply websocket updates
Krk->>Sub: updateCallback("exchange.symbol", update)
end
Main->>Met: Start MetricsServer with ExchangeMetricsView[]
Met->>Met: buildPrometheusOutput per view (emitHeaders controlled)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
bountx
changed the title
![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: 4
🧹 Nitpick comments (1)
src/metrics_server.hpp (1)
19-23: Consider documenting or enforcing non-null pointer invariant.The struct uses raw non-owning pointers which is appropriate for a view, but
metricsMapandbooksare dereferenced unconditionally inbuildPrometheusMetrics. Consider either:
- Adding a comment that these must never be null, or
- Adding debug assertions in the constructor/start method
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/metrics_server.hpp` around lines 19 - 23, ExchangeMetricsView holds non-owning raw pointers (metricsMap, books) but buildPrometheusMetrics dereferences them unconditionally; add an explicit non-null invariant by documenting that metricsMap and books must not be null in the ExchangeMetricsView definition and enforce it with debug-time checks where the view is constructed or before buildPrometheusMetrics runs (e.g., assert(metricsMap && books) or similar), referencing the ExchangeMetricsView type and the metricsMap/books members and ensuring buildPrometheusMetrics only executes when those checks pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kraken_adapter.cpp`:
- Around line 143-155: The guard in KrakenAdapter::start() drops subscribe
ack/error messages because it expects book channel payloads with
"channel"/"type"/"data"; update the receive logic to first check for top-level
subscribe acknowledgements (presence of "method" == "subscribe" and a
"result"/"success" or "error" field) before applying the existing
channel/type/data checks, and if a subscribe ack indicates failure (success ==
false or error present) mark the startup as failed and resolve/clear the
corresponding pendingSnapshots_ entry so the startup does not wait for a
snapshot timeout; ensure you reference and update the code paths that touch
pendingSnapshots_ and the startup state handling so a subscription error
triggers immediate failure rather than being ignored.
In `@src/main.cpp`:
- Around line 171-172: The partial-startup path can destroy subServer while some
adapters in runtimes are still live; ensure started adapters are stopped before
returning by adding an explicit rollback that iterates the runtimes vector and
calls each adapter's teardown/stop method (the same function used on normal
shutdown) when any ExchangeRuntime::start() fails, or alternatively move the
subServer declaration so it is created before runtimes (making subServer outlive
the adapters). Update the failure branch that currently returns -1 (after the
ExchangeRuntime::start() loop) to first stop/teardown all previously-started
ExchangeRuntime instances in runtimes, then proceed to return, and keep
references to subServer valid until that rollback completes.
- Around line 203-215: SubscriberServer is started before the exchange adapters
finish their initial sync so clients can subscribe while books are not yet
applied (snap.applied == false) and miss the initial snapshot; fix by deferring
SubscriberServer::start() (the subServer instance created with booksPtrs) until
all books have completed their initial sync (e.g., wait for each Book/BookPtr in
booksPtrs to report applied/resynced or for the adapters' start() loops to
signal completion), or alternatively implement a path in
SubscriberServer::handleSubscribe() that detects an un-applied book and pushes a
fresh snapshot when that book becomes applied/resynced; update the startup flow
around the subServer start call and/or add a watcher/callback to emit snapshots
on the transition to applied to ensure subscribers always receive the initial
snapshot.
In `@src/metrics_server.hpp`:
- Around line 108-111: The loop over views_ calls buildPrometheusOutput
repeatedly and causes duplicate `# TYPE`/`# HELP` headers; fix by emitting
headers only once—either (A) refactor buildPrometheusMetrics to call a new
buildPrometheusHeaders helper once before the for (const auto& v : views_) loop
and then call buildPrometheusOutput for each view with headers suppressed, or
(B) add a boolean parameter to buildPrometheusOutput (e.g., emit_headers = true)
and pass false for subsequent iterations (use &v == &views_.front() to decide),
updating all internal header emission sites in buildPrometheusOutput to check
that flag so headers are written only on the first call.
---
Nitpick comments:
In `@src/metrics_server.hpp`:
- Around line 19-23: ExchangeMetricsView holds non-owning raw pointers
(metricsMap, books) but buildPrometheusMetrics dereferences them
unconditionally; add an explicit non-null invariant by documenting that
metricsMap and books must not be null in the ExchangeMetricsView definition and
enforce it with debug-time checks where the view is constructed or before
buildPrometheusMetrics runs (e.g., assert(metricsMap && books) or similar),
referencing the ExchangeMetricsView type and the metricsMap/books members and
ensuring buildPrometheusMetrics only executes when those checks pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72c84324-f5ab-4b15-bf7d-4010817e3d42
📒 Files selected for processing (10)
CMakeLists.txtDockerfilesrc/binance_adapter.cppsrc/binance_adapter.hppsrc/kraken_adapter.cppsrc/kraken_utils.hppsrc/main.cppsrc/metrics_server.hppsrc/subscriber_server.hppsrc/symbol_normalizer.hpp
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
1 participant
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
New Features
Improvements
Bug Fixes