Skip to content

[Orderbook/Proto] rework protocol v1#26

Merged
wlvchandler merged 1 commit into
mainfrom
feat/orderbook-proto-v1
Apr 8, 2026
Merged

[Orderbook/Proto] rework protocol v1#26
wlvchandler merged 1 commit into
mainfrom
feat/orderbook-proto-v1

Conversation

@wlvchandler

@wlvchandler wlvchandler commented Apr 8, 2026

Copy link
Copy Markdown
Owner
  • Scope book, status, trades, and cancel by instrument
  • send amounts as fixed-point decimal
  • add batch place/cancel
  • streaming market-data RPCs

Note

High Risk
High risk because it changes the gRPC protocol (field types, enum values, and request shapes) and rewires core orderbook RPC behavior around instrument-scoped lanes, which can break compatibility across gateway/CLI/OB if any client/server stubs aren’t regenerated and updated consistently.

Overview
Reworks the orderbook gRPC protocol to be instrument-scoped and fixed-point: price/quantity fields move from strings to DecimalValue, enums gain explicit *_UNSPECIFIED values (shifting numeric values), and requests for cancel_order, get_order_book, get_order_status, and get_trade_history now require instrument_id.

Adds new RPCs and batching semantics: introduces cancel_orders batch RPC, streaming RPCs for order book and trades, and updates place_orders to return per-item results (OrderBatchItemResult) with structured ErrorDetail and BatchMode (PARTIAL_OK vs ALL_OR_NONE).

Updates implementations and clients: the Rust orderbook service routes work by instrument/lane (no cross-lane scans), adds cancel idempotency caching, and implements polling-based streaming responses; the Elixir gateway and Python CLI switch to the new decimal/instrument-aware request/response shapes (including updated parsing/formatting and a CLI --instrument flag), with the gateway explicitly marking streaming RPCs as unimplemented for now.

Reviewed by Cursor Bugbot for commit 699f812. Bugbot is set up for automated code reviews on this repo. Configure here.

… cancel by instrument; send amounts as fixed-point; add batch place/cancel and streaming market-data RPCs
@wlvchandler wlvchandler merged commit cdd6506 into main Apr 8, 2026
1 of 2 checks passed

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 4 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 699f812. Configure here.

return Some((Err(Status::not_found("No trades yet")), idx));
}
idx = (idx + 1) % resp.len();
Some((Ok(resp[idx].clone()), idx))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream trade history skips first trade each cycle

High Severity

In stream_trade_history, the trade history index is incremented before accessing the response vector. This causes the newest trade (resp[0]) to be skipped on the first emission and subsequently delivered out of order in the stream.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 699f812. Configure here.

results.push(result);
}
Ok(Response::new(OrderBatchResponse { orders: responses }))
Ok(Response::new(OrderBatchResponse { results }))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AllOrNone batch doesn't rollback placed orders

High Severity

In place_orders, BatchMode::AllOrNone processes orders sequentially. If an order fails, previously successful orders are not rolled back, leaving the matching engine in a partially updated state and violating the "all or none" atomicity guarantee.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 699f812. Configure here.

defp to_proto_response(response) do
%Orderbook.OrderResponse{
id: response.id,
price: to_string(response.price),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gateway batch response uses outdated proto field

High Severity

The place_orders handler constructs Orderbook.OrderBatchResponse with an orders field containing OrderResponse items. The updated proto definition now uses a results field with OrderBatchItemResult items. This creates an incompatible wire format and will break once the Elixir proto module is regenerated.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 699f812. Configure here.

units: value.mantissa() as i64,
scale: value.scale() as i32,
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decimal mantissa silently truncated from i128 to i64

Medium Severity

The decimal_to_proto function converts the i128 mantissa to i64 using a direct as cast. This silently truncates values exceeding the i64 range, which can corrupt financial data like prices and quantities in the wire representation.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 699f812. Configure here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant