Conversation
Share the proto-loader option set across descriptor and proto loads so the bundled server matches existing runtime materialization behavior. Normalize enum names at the server boundary because enums are now deserialized as strings, and cover descriptor defaults, enums, and longs with a focused regression test.
Validate numeric action and subscription inputs against the enum values before accepting them. Use own-property checks for string names so inherited keys fall through to the existing unknown labels and subscription fallback behavior.
Switch gRPC schema loading from runtime protobuf files to precompiled descriptor
feat: capture CEX order execution telemetry
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts server.ts into modular handlers and helpers, adds proto descriptor/package loading, expands order-book, perp, telemetry, and streaming behavior, and updates docs, tests, CI, and packaging. ChangesProto, docs, config, and scaffolding
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ExecuteActionHandler
participant ActionRegistry
participant Broker
Client->>Server: ExecuteAction request
Server->>ExecuteActionHandler: createExecuteActionHandler(...)
ExecuteActionHandler->>ActionRegistry: dispatchExecuteAction(ctx)
ActionRegistry->>Broker: invoke action handler
Broker-->>ExecuteActionHandler: result or error
ExecuteActionHandler-->>Client: wrappedCallback(response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/order-telemetry.ts`:
- Line 83: The telemetry payload logged via log.info("CEX market action
execution telemetry", telemetry) currently includes raw upstream error text
(error.message stored earlier), so before emitting telemetry remove or redact
the error.message field on the telemetry object (or replace it with a generic
code/value like "redacted_error" or an errorCode) — locate where telemetry is
assembled and where error.message is assigned and ensure you strip or replace
that property prior to calling log.info so structured telemetry never contains
raw exchange error strings.
In `@src/server.ts`:
- Around line 882-897: The RPC path is blocking on emitOrderExecutionTelemetry;
change the three await usages to fire-and-forget so telemetry won't delay
responses: replace awaiting calls to emitOrderExecutionTelemetry(otelMetrics,
{...}, order) with non-blocking invocations such as void
emitOrderExecutionTelemetry(otelMetrics, {...}, order).catch(err =>
processLogger.warn("Telemetry emit failed", err)) (or equivalent error handling)
so errors are surfaced but do not block the RPC; apply the same change for each
occurrence where emitOrderExecutionTelemetry is currently awaited.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec8dab9-e263-44ab-99df-4ce7a782082b
📒 Files selected for processing (16)
.gitignoreREADME.mdexamples/kraken-orderbook-demo.tspackage.jsonsrc/helpers/constants.tssrc/helpers/order-telemetry.tssrc/proto-loader-options.tssrc/proto-package-definition.tssrc/proto/node.descriptor.tssrc/server.tstest/helpers-constants.test.tstest/internal-transfer-rpc.test.tstest/order-telemetry-fixtures.tstest/order-telemetry.test.tstest/proto-descriptor.test.tstest/server-proto-loader-options.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-14T06:47:01.283Z
Learnt from: csmithington
Repo: usherlabs/cex-broker PR: 38
File: src/client.dev.ts:0-0
Timestamp: 2026-04-14T06:47:01.283Z
Learning: In this codebase, gRPC action constants such as `FetchTicker`, `FetchFees`, and `FetchAccountId` should be sourced from `src/helpers/constants.ts` and imported from there (e.g., used by both `src/client.dev.ts` and `src/server.ts`). Do not import or reference generated proto TypeScript artifacts (for example `./proto/cex_broker/Action`), since those generated files are git-ignored and won’t be available/committed consistently.
Applied to files:
src/proto-loader-options.tssrc/proto-package-definition.tssrc/proto/node.descriptor.tssrc/helpers/constants.tssrc/helpers/order-telemetry.tssrc/server.ts
🪛 LanguageTool
README.md
[grammar] ~546-~546: Use a hyphen to join words.
Context: ...change timestamp when present and broker observed timestamp Use metrics for aggr...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (13)
package.json (1)
3-3: LGTM!src/proto-loader-options.ts (1)
1-9: LGTM!examples/kraken-orderbook-demo.ts (1)
9-9: LGTM!Also applies to: 16-19
src/proto/node.descriptor.ts (1)
1-127: LGTM!test/proto-descriptor.test.ts (1)
1-10: LGTM!src/proto-package-definition.ts (1)
1-8: LGTM!.gitignore (1)
39-39: LGTM!test/server-proto-loader-options.test.ts (1)
1-54: LGTM!src/helpers/constants.ts (1)
44-110: LGTM!test/helpers-constants.test.ts (1)
4-45: LGTM!test/internal-transfer-rpc.test.ts (1)
6-13: LGTM!test/order-telemetry-fixtures.ts (1)
1-135: LGTM!test/order-telemetry.test.ts (1)
1-370: LGTM!
…ew' (#3) from feat/create-a-new-pr-for-cex-broker-enable-github-ci-on-develop-a into develop Reviewed-on: https://git.aos.usher.so/ao/cex-broker/pulls/3
…) from gitea/order-book-depth-sourcing-refinements into ao Reviewed-on: https://git.aos.usher.so/ao/cex-broker/pulls/4
Local/improved backtest harness
…book-depth-sourcing-refinements
Route Binance BALANCE and ORDERS Subscribe requests through a signature-based Spot WebSocket API user-data adapter instead of CCXT watchBalance/watchOrders. Normalize Subscribe exchange lookup and install stream lifecycle handling before long-running subscription work starts. Add focused Subscribe tests for Binance balance and order frames, uppercase exchange lookup, secondary account selection, and avoiding the retired listenKey-backed watch paths.
Document Subscribe enum values to match the proto/constants contract, including BALANCE and ORDERS account streams. Describe Binance account streams as using the WebSocket API user-data subscription path without Spot listenKey REST lifecycle. Refresh the checked-in proto descriptor generated by the build workflow.
Allow callable treasury methods when capability metadata is missing while still rejecting methods explicitly marked unsupported. Add regression coverage for missing capability metadata and explicit false capability handling.
Default missing, empty, and unknown deposit statuses to pending while preserving explicit terminal status mappings. Report observed deposit address mismatches with the deposit_address_mismatch prefix and remove unused handler bindings. Add focused regression coverage for deposit status normalization and address mismatch RPC errors, with related lint-only cleanup.
End subscribe streams after writing per-subscription error responses and outer internal-error responses. Report the resolved subscription type from the outer subscribe error path. Add focused subscribe handler coverage for stream closure and outer error type reporting.
Archive the authoring component on order events
Release cex-broker 0.2.33
Archive rows are delivered at-most-once over a bounded queue, so a lost row and an event that never happened are currently indistinguishable. Add `seq`, a monotonic per-(controller_id, run_id) counter shared across every strategy_data table, so gap detection becomes a query over holes in the sequence. The column ships on both the CREATE TABLE definitions and as idempotent ALTER ... ADD COLUMN IF NOT EXISTS statements, so existing deployments pick it up when the forwarder applies the schema at startup. Document the delivery contract in the schema header, including the ordering constraint: inserts use JSONEachRow, so a producer emitting a column ClickHouse does not have fails the whole per-table batch. The forwarder must be deployed before the producer.
Add per-run seq column to strategy_data archive tables
Release cex-broker 0.2.34
fix(orders): preserve venue error codes for passive submissions
Capture venue deposits in the execution archive
chore(release): bump cex-broker to 0.2.35
fix(deposits): preserve venue status transitions for locked Binance deposits
…eartbeat Two issues from review, both real. Rejected rows are precisely the rows whose table is unknown, so their names came straight from the request payload and became metric labels verbatim. Every distinct label value is a permanent series in the metrics SDK, so any client could grow our memory without bound — a metrics pipeline that traffic can exhaust is worse than no metrics. Labels are now bounded to the supported set plus fixed malformed/unsupported buckets, enforced at the metric boundary rather than at the single call site so a future caller cannot reintroduce it. Raw names still reach the response and the log, where they are bounded per request and are what an operator actually needs. The flush heartbeat also advanced on batches that inserted nothing, because zero failures is trivially true for an empty batch. An empty POST would have kept a staleness alert green while no data reached ClickHouse — the exact condition the heartbeat exists to detect, since its only job is separating quiet from dead. Tests: a client inventing 50 table names yields one series rather than fifty, and an empty batch leaves the heartbeat untouched. Full suite 423 pass, 0 fail.
feat(archive): add forwarder OpenTelemetry metrics
release: cex-broker 0.2.36
A submission row carried only the caller's withdrawal id and the venue observations that terminate it carried only the venue id, so the two halves of one movement had no shared key and the lifecycle could not be joined. Amounts differ by the withdrawal fee, so they are not a fallback. Binance echoes the caller's withdrawOrderId on withdrawal-history records; carry it onto observe_withdrawal rows as client_withdrawal_id. The column and its migration already exist.
…elation-id fix(archive): correlate withdrawal observations with their submission
…archive fix(archive): index Binance internal transfer venue ids
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
4 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 by CodeRabbit
.gitignorerule, and server line-budget script