Skip to content

Add orderbook ingestion scaffold for Binance & OKX (ETH/USDT) with Docker, uv, and tests - #1

Open
beingzy wants to merge 2 commits into
mainfrom
codex/build-orderbook-data-reader-for-binance-and-okx
Open

Add orderbook ingestion scaffold for Binance & OKX (ETH/USDT) with Docker, uv, and tests#1
beingzy wants to merge 2 commits into
mainfrom
codex/build-orderbook-data-reader-for-binance-and-okx

Conversation

@beingzy

@beingzy beingzy commented Jan 31, 2026

Copy link
Copy Markdown

Motivation

  • Provide a minimal, uv-managed Python foundation to ingest real-time and historical ETH/USDT orderbook data from Binance and OKX and prepare for order placement.
  • Include a reproducible development and deployment setup using Docker Compose and PostgreSQL for storage.
  • Enable fast local validation via a simulated-data generator so parsing and storage logic can be exercised without live API access.

Description

  • Add project metadata and dependencies with pyproject.toml and a uv-friendly entrypoint script (orderbook_app.__main__:main).
  • Implement orderbook models (models.py) and parsers for Binance and OKX payloads (connectors/binance.py, connectors/okx.py) with a shared parse_levels helper.
  • Add async PostgreSQL helpers (storage/db.py) that create the orderbook_updates table and provide init_db/insert_update using asyncpg, and wire a simulated ingest flow in __main__.py that uses services/simulated.py.
  • Provide deployment artifacts: Dockerfile to build a container using uv and docker-compose.yml to start db (Postgres) and app, and document required REST/WebSocket endpoints and API permissions in README.md.

Testing

  • Added unit tests in tests/test_parsers.py that validate Binance/OKX snapshot/update parsing and the simulated update generator; these are runnable with pytest.
  • No automated tests were executed as part of this change (tests added but not run locally or in CI).

Codex Task

@beingzy

beingzy commented Jun 3, 2026

Copy link
Copy Markdown
Author

Code Review by 🤖 Claude Code · Sonnet 4.6

🚫 BLOCK

Summary

Scaffold for real-time ETH/USDT orderbook ingestion from Binance and OKX using asyncpg, Pydantic v2, websockets, and Docker. Includes parsers, simulated data generator, DB storage, and unit tests.


Critical Issues

OKX level unpacking will crash on real API data (models.py:30, connectors/okx.py)

parse_levels does for price, size in levels, which assumes 2-element arrays. The real OKX books channel returns 4-element arrays: ["price", "size", "liquidated_orders", "orders"]. This raises ValueError: too many values to unpack (expected 2) on any live OKX feed. The tests pass only because they use hand-crafted 2-element arrays, masking the bug entirely.

Fix: OrderBookLevel(price=float(row[0]), size=float(row[1])) for row in levels

asyncpg JSONB insertion likely fails at runtime (storage/db.py:35-40, __main__.py:18-22)

asyncpg requires JSONB column values to be passed as JSON-encoded strings by default, not raw Python lists/dicts. insert_update receives Python lists (e.g. [{"price": 3000.0, "size": 1.5}]) and passes them directly as $4/$5. Without a registered codec this raises a type error. Should be json.dumps(bids) / json.dumps(asks).


Warnings

float for price/size (models.py:9-10): Binary float representation is lossy for financial data. Decimal is conventional; at minimum this limitation should be documented.

New DB connection per insert (storage/db.py): init_db and insert_update each call asyncpg.connect(dsn). For a streaming orderbook this creates thousands of short-lived connections. Use a connection pool (asyncpg.create_pool).

Hardcoded credentials (Dockerfile:12, config.py:14): postgres:postgres is embedded as a default in application code and the image layer. The compose file can define defaults, but the app default should require the env var to be set explicitly (raise if absent), or at least be document-only.

Double JSON round-trip (__main__.py:18-21): json.loads(update.model_dump_json())["bids"] serializes to a JSON string then parses it back. update.model_dump()["bids"] achieves the same result without the round-trip.


Suggestions

  • OrderBookSnapshot and OrderBookUpdate are structurally identical; a single OrderBookData model with a kind: Literal["snapshot", "update"] field would reduce duplication.
  • connectors/okx.py: parse_snapshot and parse_update share identical body; extract a private _parse_okx_data(payload, symbol, cls) helper.
  • Missing connectors/__init__.py (fine for namespace packages, but explicit is clearer).
  • Consider pytest-asyncio with DB integration tests using pytest-docker or a testcontainer to catch asyncpg JSONB codec issues before they reach CI.

Reviewed with Claude Code via Crosscheck

Crosscheck-Reviewer: claude
Crosscheck-Model: claude-sonnet-4-6
Crosscheck-Step: fix
Crosscheck-Service: crosscheck
@beingzy

beingzy commented Jun 3, 2026

Copy link
Copy Markdown
Author

✅ Auto-fix applied

Pushed bbeb552 addressing the code review: 3 changes applied.


Applied by Claude Code via Crosscheck.

@beingzy

beingzy commented Jun 3, 2026

Copy link
Copy Markdown
Author

Recheck of original review

Recheck by 🤖 Claude Code · Sonnet 4.6

APPROVE

Now I have a complete picture. The fix commit bbeb552 addressed exactly 3 issues from the original review. Let me verify each fix is correctly applied in the current PR diff.

Issue 1 — __main__.py: Redundant JSON roundtrip

  • Original: json.loads(update.model_dump_json())["bids"] — serialized to JSON then immediately parsed back
  • Fixed: update.model_dump()["bids"]

Issue 2 — models.py: Fragile tuple unpacking in parse_levels

  • Original: for price, size in levels — would throw ValueError on OKX rows which have 4 fields (price, qty, liquidated_orders, num_orders)
  • Fixed: for row in levels with row[0], row[1]

Issue 3 — storage/db.py: Missing JSON serialization for asyncpg JSONB columns

  • Original: passed raw Python lists as bids/asksasyncpg requires a JSON string for jsonb parameters
  • Fixed: json.dumps(bids), json.dumps(asks) with the necessary import json added ✓

All three fixes are present and correct in the current PR diff.



Reviewed with Claude Code via Crosscheck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant