feat(order-router): scaffold ccxt-based smart order router service - #16
Draft
pcriadoperez wants to merge 12 commits into
Draft
feat(order-router): scaffold ccxt-based smart order router service#16pcriadoperez wants to merge 12 commits into
pcriadoperez wants to merge 12 commits into
Conversation
…king price routing Standalone Node/TS service (order-router/) built on ccxt: per-exchange WS connectors cache full L2 order books in-process, and /price/best walks the book to the requested order size (not just top-of-book) with fee-adjusted ranking across exchanges. Includes a WS latency/depth benchmark script and its results/caveats (Binance/Bybit/OKX unreachable from this sandbox, clock-skew caveat on latency numbers) documented in README.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
…ecture decisions - OrderBookCache now emits update:<symbol> events; /stream/best subscribes instead of polling on a fixed interval, coalescing bursts via setImmediate. Polling cost scales with poll_rate x client_count regardless of whether anything changed, which doesn't hold up at scale. - README documents why single-process Node (not a Redis-split collector/Rust-API architecture) is the right call for now, and the connection-count-vs-message-throughput scaling analysis for WS ingestion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Owner
Author
|
The Generated by Claude Code |
…universe Dynamically discover every ccxt.pro exchange with watchOrderBook support (76 of 80) and their full market lists, then subscribe only to symbols tradable on >= ORDER_ROUTER_MIN_EXCHANGES_PER_SYMBOL exchanges (default 2) — a symbol listed on exactly one exchange has nothing to route between, and empirically that's 78% of all markets across just 5 test exchanges (9,412 unique symbols, only 2,102 on 2+ exchanges). Live-tested against the full routable symbol set on Kraken/Coinbase/KuCoin/ Bitget/Gate and fixed three real failures surfaced in the process: - Reconnect storms from N independent per-symbol watch loops retrying in lockstep after a shared-connection failure -> prefer batched watchOrderBookForSymbols where supported (24/76 exchanges), add jittered backoff. - Per-message subscription caps (Bitget "max:1000", KuCoin overlong topic string) -> chunk into ORDER_ROUTER_MAX_SYMBOLS_PER_SUBSCRIPTION-sized groups with staggered startup. - Session-wide caps that chunking alone can't fix (Coinbase "too many L2 streams") -> per-exchange total-symbol cap via ORDER_ROUTER_MAX_SYMBOLS_PER_EXCHANGE. Also adds process-based sharding (ORDER_ROUTER_SHARD_COUNT) for the single-thread CPU ceiling at full discovery scale: exchanges are load-balanced across child_process workers, each running its own connectors and cache, relaying book/health/fee writes to the parent over Node's IPC pipe rather than Redis (same-host, no external DB, off the API's synchronous read path). Fee lookups moved to a shared FeeRegistry so both single-process and sharded modes work through the same interface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Owner
Author
|
Updated to scale to all exchanges and their routable symbol universe, per discussion:
Full writeup of what broke and why is in Generated by Claude Code |
…ic MCP server Unit tests (node:test via tsx, 49 tests, all offline, no mocked fetch): routing/bestPrice (VWAP + fee ranking), cache/orderBookCache, cache/feeRegistry, discovery/symbolUniverse (extracted pure computeRoutableSymbols), sharding/ orchestrator (load balancing), connectors/exchangeConnector (extracted pure chunkSymbols/normalizeLevels), api/server (Fastify inject() against the routes), and the new MCP layer (real Client/McpServer over InMemoryTransport, plus a real local HTTP fixture for the REST proxy layer instead of mocking fetch). MCP server (src/mcp/): a separate process exposing the router's public REST endpoints as MCP tools (get_health, get_exchanges_status, list_symbols, get_order_book, get_best_price) over the Streamable HTTP transport. Thin proxy over the existing REST API rather than a second implementation of the routing logic, decoupled onto its own port/process so MCP client traffic never touches the router's hot path. Verified live end-to-end (real StreamableHTTPClientTransport against a running router with live exchange data), not just unit-tested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Owner
Author
|
Added unit tests and a public MCP server: Tests ( MCP server (
Generated by Claude Code |
Owner
Author
|
Another Generated by Claude Code |
Owner
Author
|
The Generated by Claude Code |
Owner
Author
|
Another Given the pattern across all four CI failures on this PR so far (Java x2, JS live-tests, Python live-tests — all pre-existing and unrelated to this diff), I'll keep monitoring but won't comment on further instances of the same unrelated flakiness unless something actually touches Generated by Claude Code |
…e benchmarks, CI Auth (src/api/auth.ts): shared API key via X-API-Key or Authorization: Bearer, on every route except /health (kept open for orchestrator liveness probes). Compared through SHA-256 digests + timingSafeEqual so neither key length nor common prefix leaks via timing and a length mismatch can't throw. The hook runs at onRequest, before routing, so unknown paths 401 rather than 404 (no route enumeration oracle) and missing vs wrong keys are indistinguishable. Unset ORDER_ROUTER_API_KEY falls back to a well-known dev literal and warns loudly. Explicitly a stopgap: no rotation, per-client keys, revocation or scopes. Rate limiting (@fastify/rate-limit): 600/60s, registered before the auth hook so invalid-key floods burn budget rather than reaching the comparison unbounded. Bucketed per API key so one client can't exhaust another's; /health exempt so a throttled probe can't read as an outage. buildServer now takes explicit options rather than only reading module config, so tests can drive the real middleware chain at a low limit. MCP server authenticates its own callers with the same key and forwards it upstream — without that it was an unauthenticated bypass around router auth. Benchmarks: benchmark/load-test.mjs (autocannon over the live service, reporting per-scenario percentiles and non-2xx counts) and benchmark/multi-exchange-connect.mjs (simultaneous live WS connections). Measured 28 of 34 exchanges connected concurrently for 75s, 0 errors on 27, 230MB RSS; and 4.3k-13.8k req/s with p99 9-18ms across endpoints. All 6 connection failures are external (missing optional protobuf dep, credential-gated WS, or sandbox geo-restrictions). CI (.github/workflows/order-router.yml): path-scoped to order-router/, runs build, the offline test suite, and boot smoke tests asserting over real HTTP that /health is open, protected routes 401 without a key and 200 with it, a wrong key is still rejected, and the MCP endpoint rejects unauthenticated JSON-RPC. Smoke assertions verified locally before commit. Tests: 70 total (up from 49) — auth unit tests plus integration tests through the real hook chain for auth and rate limiting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
…ce was unthrottled Two real vulnerabilities in the auth/rate-limit code shipped in c475f54, found by an adversarial review of that commit and confirmed against a running server. 1. Hook ordering was inverted from what the code and README claimed. @fastify/rate-limit attaches its check as a PER-ROUTE hook, and route-level onRequest hooks run after all instance-level ones — so the instance-level auth hook preceded the limiter regardless of registration order. Every 401 short-circuited before the limiter counted it, leaving API key brute-force completely unthrottled while authenticated traffic still appeared correctly limited. Measured before the fix: 30 wrong-key requests against a limit of 10 returned 401 thirty times, never 429. Fix: auth moves to preValidation, which runs after the entire onRequest chain. A custom notFoundHandler re-checks the key so unknown paths still return 401 rather than 404 for unauthenticated callers, preserving the no-route- enumeration property that moving off onRequest would otherwise have lost. 2. Bucketing the limiter on the caller-supplied API key header let an attacker rotate the header per request to mint a fresh bucket every time and brute-force without limit (also an unbounded-memory vector, one counter per attacker-chosen value). Fix: bucket by key only once the key is valid; wrong or absent keys bucket by IP. Valid clients keep per-key fairness. Adds three regression tests covering exactly what the original tests missed: repeated wrong keys, rotating keys, and no key at all must each eventually 429. Testing only the authenticated path cannot catch either bug. Corrects the README, which had asserted the broken ordering as if it were working. Verified live: rotating wrong keys now 401x5 then 429; /health still never throttled; unknown paths still 401 unauthenticated. 73 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Second round of findings from the adversarial security review of the auth work.
The /stream/best WS handler took Number(request.query.amount) with no validation
while the REST handler at the same logic returned 400 unless amount > 0. So
`amount=abc` produced NaN, which defeats the `remaining <= 0` termination check
in walkBook: it traversed every level of every cached book and then streamed
{amount: null, filledAmount: null, averagePrice: null} — repeating that on every
book update, which for a streaming endpoint is strictly worse than the one-shot
REST equivalent. `amount=-5` and `amount=0` were accepted too. Now rejected with
a 1008 policy-violation close.
Also verified (not assumed) that the preValidation move in 6c79d93 fixed a
separate confirmed finding: when auth 401'd at onRequest, reply.sent halted the
hook chain before @fastify/websocket's hook ran, so request.ws stayed null and
its cleanup hook no-oped, leaking the raw upgrade socket on every rejected
WS handshake. Measured after the fix: 0 sockets held after 25 rejected upgrades.
Adds a regression test using a real listening server and ws client — inject()
cannot reach the upgrade handler where the bug lived.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
…ce door Third round from the adversarial review. Fixing failed-auth throttling in 6c79d93 covered only the Fastify router on :8080. The MCP server is a separate process on :8081 running a bare node:http listener with its own inline auth and no throttling at all, published on 0.0.0.0 by docker-compose. Measured before this change: 200 POST /mcp requests with rotating x-api-key values returned 401 x200, never a 429 — the exact vulnerability class the previous commit called a shipped vulnerability, still fully open on the other port. Adds src/api/rateLimiter.ts, a small fixed-window limiter used by the MCP server (it cannot use @fastify/rate-limit). Mirrors the router's semantics: bucket by API key only when the key is valid, otherwise by client address, so header rotation cannot mint fresh buckets. Prunes expired buckets, since bucket keys are partly client-controlled and would otherwise grow without bound under a distributed flood. Verified live: rotating wrong keys now 401x5 then 429; /health still never throttled. 79 tests pass, including limiter unit tests covering window rollover, bucket independence, remaining/reset reporting, and pruning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
…t leak Closes out the adversarial security review (20 agents, 16 raw findings, 5 confirmed). The socket-leak finding turned out to be far more severe than the first pass suggested: the verifier measured 1200 unauthenticated upgrades leaking 1200 FDs in ~708ms (~1700 FDs/sec), held across a 400s observation, and leaking path-independently — matched WS route, matched non-WS route, and unmatched paths alike. A 1024-FD container dies in under a second, taking /health with it, so an orchestrator restart just resets the clock. Re-verified the preValidation fix from 6c79d93 under those same conditions rather than trusting the earlier 2-second single-path check: 0 sockets leaked across all three path classes and across a 300-upgrade burst. Adds a regression test driving real TCP upgrade requests — app.inject() never touches the socket path where this bug lives, which is why the existing suite could not see it. A/B verified the test is not vacuous: reverting auth to onRequest fails it (along with two rate-limit tests); restoring the fix passes all 80. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Closes security finding #4b, the last open item from the adversarial review. Each /stream/best connection holds an EventEmitter listener on the cache that is removed only on socket close — which the client controls — plus a recomputation per book update for its symbol. Rate limiting does not bound this: it caps how fast connections open, not how many stay open. Two mechanisms now do: - Per-key concurrency cap (ORDER_ROUTER_WS_MAX_CONNECTIONS_PER_KEY, default 50), refused with a 1013 close. Counted per key so one client cannot starve another. - Ping/pong heartbeat (ORDER_ROUTER_WS_IDLE_TIMEOUT_MS, default 120s) that terminates sockets which stop responding without sending a close frame — half-open TCP, suspended clients — which would otherwise hold a listener and a cap slot indefinitely. Uses terminate() rather than close() because an unresponsive peer will never complete a closing handshake. Slot release is idempotent and deletes rather than zeroes its map entry: 'close' can fire after terminate(), and double-release would corrupt the count and eventually lock a legitimate client out of its own budget, turning a DoS defence into a self-inflicted DoS. The key is client-supplied, so retaining empty entries would let key rotation grow the map without bound. Defaults are chosen to be generous rather than blocking; both are configurable. Also adds protobufjs, which mexc requires to decode its WS frames. Verified live: mexc now returns a real book (104 bids / 108 asks), taking the reachable exchange count from 28 to 29. 84 tests pass (up from 80), including regression coverage for the cap, slot release and reuse, per-key independence, and the heartbeat reaper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
Next prod-readiness blocker: the service was operable only by manually poking /exchanges/status, so degradation would be discovered by users rather than by us. GET /metrics serves Prometheus text format, authenticated like every other non-/health route — it exposes the venue list, traffic volume and internal health, which is exactly the reconnaissance an attacker wants. Values are derived from cache state at scrape time via collect() callbacks rather than incremented alongside it. The cache already owns the authoritative counters; mirroring them into separate Prometheus counters would create two sources of truth that can drift, and a missed increment would be invisible. Consequence: cumulative series are Gauges whose values are monotonic rather than Counters — rate() and increase() still work and a restart resets to 0 exactly as a Counter would. The metric that matters most is exchange_last_update_age_seconds. An exchange can hold an open socket while its subscription is silently dead: connected stays 1, nothing errors, and the router keeps ranking on data that no longer reflects the market. Connection state alone cannot catch that. An exchange that has never updated reports process uptime rather than 0, so a connector that never produced a message is loud rather than indistinguishable from one that just updated. Histogram labels use the route template, never the raw URL — labelling by concrete symbol would mint one series per symbol across a ~10k routable universe. Also fixes the test script, which silently excluded tests: the glob was unquoted, so bash expanded src/**/*.test.ts as src/*/*.test.ts and dropped every file directly under src/. The new metrics tests did not run at all until this was found. Quoting hands globbing to Node's test runner. Test count went 84 -> 92, 8 of which were being skipped. Verified live against kraken + coinbase: unauthenticated /metrics returns 401, authenticated returns real values, and histogram labels show the route template rather than concrete symbols. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
…rse proxy Prompted by the question of whether nginx on a VPS covers the TLS gap. It does — and that closes TLS for a proxied deployment — but putting a proxy in front breaks two things in the current code. 1. request.ip becomes the proxy's address. The rate limiter buckets failed-auth attempts by IP, so behind nginx ALL unauthenticated traffic collapses into a single shared bucket: per-client fairness disappears and one abuser throttles every other client's legitimate retries. Fixed with an opt-in ORDER_ROUTER_TRUST_PROXY flag wired to Fastify's trustProxy. Deliberately opt-in rather than automatic, because the opposite error is worse: trusting X-Forwarded-For with no proxy in front lets any client rotate the header, mint a fresh bucket per request, and bypass rate limiting entirely — re-opening the brute-force hole the limiter exists to close. Both directions are regression-tested. 2. The WS heartbeat (120s) was longer than nginx's default proxy_read_timeout (60s), so nginx would reap an idle stream before the heartbeat ever fired. Default lowered to 30s. This hides on liquid pairs — a busy book keeps the connection full of real data — and only shows up on quiet symbols, where the stream silently drops about once a minute. Also documents the deployment: bind HOST=127.0.0.1 so the service cannot be reached directly on 8080 bypassing TLS, a full nginx config including the WS upgrade headers /stream/best requires, and restricting /metrics to a scraper network. Notes what nginx does NOT solve — the API key is still a single shared secret with no rotation or revocation. 94 tests (up from 92). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ZBbcVGn34SCwcfCkWL218
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
2 participants
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
Scaffolds
order-router/, a standalone Node/TypeScript service (depends onccxtas a library — no changes tots/src/**or the build/transpile pipeline) that aggregates live L2 order books across exchanges viaccxt.proWebSocket connections and serves best-execution price lookups that account for order size (book-walking, not just top-of-book) and taker fees.ExchangeConnector(isolated failure domain, exponential-backoff reconnect) caches full L2 books in an in-processOrderBookCache— reads never touch the network, no Redis/network hop in the hot path./price/best/:symbol?side=&amount=walks the cached book to the requested size and ranks exchanges by fee-adjusted effective price, not raw top-of-book./stream/best/:symbolpushes on book-change (event-driven, coalesced viasetImmediate) rather than polling./health,/exchanges/status,/symbols,/orderbook/:exchange/:symbol,/price/best/:symbol,/stream/best/:symbol(WS).benchmark/ws-latency.mjs— WS latency/depth benchmark script, with results and important caveats documented inorder-router/README.md(Binance/Bybit/OKX unreachable from this dev sandbox due to geo-blocking; latency figures affected by unsynced container clock — needs re-running from real deployment infra).README.mdalso documents the architecture decisions made along the way and why: single-process Node vs. a split TS-collector/Redis/Rust-API design (rejected — Redis round-trip cost dominates any language-layer speed gain), and the WS connection-count-vs-message-throughput scaling analysis.Test plan
npx tsc -p order-router/tsconfig.json— clean build/health,/symbols,/exchanges/status,/orderbook/:exchange/:symbol,/price/best/:symbol(both buy and sell, various sizes) all return correct, sane data/stream/best/:symbolpushes on book change via a WS test clientNotes
This is diff-scoped to
order-router/only — nots/src/**, generated files, or other CCXT library code touched. Order execution routing (placing real orders) is intentionally out of scope for this milestone; see README "Known gaps" for what's next.Generated by Claude Code