Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
Broker surface core module src/helpers/broker-surface.ts, test/broker-surface.test.ts |
Defines BrokerSurface, defaults, env resolution/validation, action/subscription/CCXT classification, access checks, and a denied-error builder, with unit tests. |
CLI flags and broker startup resolution src/cli.ts, src/commands/start-broker.ts, src/index.ts, .env.sample |
CLI adds --no-read/--no-write flags; startBrokerCommand resolves and validates a merged surface; CEXBroker stores, validates, and logs the surface; .env.sample documents new env vars. |
Server wiring src/server.ts |
getServer accepts a brokerSurface parameter and passes it into the execute-action and subscribe handler factories. |
ExecuteAction handler and context guards src/handlers/execute-action/handler.ts, src/handlers/execute-action/context.ts |
ExecuteActionDeps/ExecuteActionContext gain brokerSurface; the handler classifies and denies disallowed actions with metrics; new rejectUnlessReadSurface/rejectUnlessWriteSurface guard helpers are exported. |
Per-action handler guards src/handlers/execute-action/order-book-call.ts, src/handlers/execute-action/orders.ts, src/handlers/execute-action/treasury-call.ts, src/handlers/execute-action/withdraw.ts |
Order-book, order create/cancel/details, treasury-call, and withdraw handlers each perform an early read/write surface check before existing logic. |
Subscribe handler surface gating src/handlers/subscribe/handler.ts, test/subscribe-handler.test.ts |
SubscribeDeps gains optional brokerSurface; subscriptions are classified and denied with a metric and error frame when disallowed. |
Integration and unit tests test/execute-action-surface-rpc.test.ts |
New gRPC integration suite exercises read/write surface denial and success paths for direct actions and CCXT calls. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Possibly related PRs
- usherlabs/cex-broker#44: Both modify the order-book handling path in
src/handlers/execute-action/order-book-call.ts. - usherlabs/cex-broker#46: Both modify
src/handlers/subscribe/handler.ts’s subscribe flow. - usherlabs/cex-broker#48: Both touch the subscribe handler’s error and stream-response path.
Suggested reviewers: victorshevtsov, rsoury, csmithington
Poem
A rabbit hops through read and write,
and checks each gate by day and night.
If paws are barred, it says “not now,”
with FAILED_PRECONDITION’s solemn vow.
🐇
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly describes the main change: adding read/write broker surfaces to support the TEE split. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
FIET-902
Comment @coderabbitai help to get the list of available commands.
![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
🧹 Nitpick comments (2)
test/subscribe-handler.test.ts (1)
487-511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a symmetric read-surface denial test.
Only write-surface denial is covered here (ORDERS subscription with
writeEnabled: false). A companion case forreadEnabled: falseagainst a read subscription type (e.g.ORDERBOOK) would round out coverage ofclassifySubscription/isBrokerAccessAllowedin this handler test file.🤖 Prompt for 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. In `@test/subscribe-handler.test.ts` around lines 487 - 511, Add a symmetric read-surface denial test in subscribe-handler.test.ts to mirror the existing write-surface case. Use createSubscribeHandler with brokerSurface set to readEnabled: false and writeEnabled: true, then exercise a read subscription such as ORDERBOOK via createSubscribeCall and verify the handler rejects it with the expected access-disabled error and ends the call. This should cover the read path in classifySubscription and isBrokerAccessAllowed alongside the current ORDERS/write-disabled test.test/execute-action-surface-rpc.test.ts (1)
108-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for server/client setup.
Every test repeats the same
getServer(...)call with identicalundefined, undefinedplaceholders,bindServer, and client construction. A smallsetupServer(pool, brokerSurface)helper returning{ client }would remove this repetition and make future signature changes (like the one flagged above) a one-line update.🤖 Prompt for 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. In `@test/execute-action-surface-rpc.test.ts` around lines 108 - 348, The tests repeat the same server/client bootstrap logic in each case, making them noisy and brittle. Extract a shared helper around getServer, bindServer, and grpcObj.cex_broker.cex_service construction in execute-action-surface-rpc.test.ts, ideally a setupServer(pool, brokerSurface) utility that returns the bound client (and port if needed). Update each test to use that helper so the repeated undefined placeholders and client setup live in one place and future getServer signature changes only need a single update.
🤖 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/cli.ts`:
- Around line 20-27: The negated Commander flags are being read from the wrong
option names, so the restrictions never take effect. Update the CLI handling in
cli.ts to use options.read and options.write for the --no-read and --no-write
flags, and adjust any related conditionals or defaults that currently reference
options.noRead or options.noWrite. Keep the fix centered on the option parsing
logic and the symbols that consume these flags.
In `@src/helpers/broker-surface.ts`:
- Around line 190-198: The denial message in buildBrokerSurfaceDeniedError is
misleading for access === "unknown" because it always maps to a write-disabled
message. Update the logic in buildBrokerSurfaceDeniedError to distinguish
unknown CCXT methods from true write-only denials and emit a message that
reflects the actual broker requirement in isBrokerAccessAllowed, using the
existing access value and the read/write enabled context rather than defaulting
unknown to write.
---
Nitpick comments:
In `@test/execute-action-surface-rpc.test.ts`:
- Around line 108-348: The tests repeat the same server/client bootstrap logic
in each case, making them noisy and brittle. Extract a shared helper around
getServer, bindServer, and grpcObj.cex_broker.cex_service construction in
execute-action-surface-rpc.test.ts, ideally a setupServer(pool, brokerSurface)
utility that returns the bound client (and port if needed). Update each test to
use that helper so the repeated undefined placeholders and client setup live in
one place and future getServer signature changes only need a single update.
In `@test/subscribe-handler.test.ts`:
- Around line 487-511: Add a symmetric read-surface denial test in
subscribe-handler.test.ts to mirror the existing write-surface case. Use
createSubscribeHandler with brokerSurface set to readEnabled: false and
writeEnabled: true, then exercise a read subscription such as ORDERBOOK via
createSubscribeCall and verify the handler rejects it with the expected
access-disabled error and ends the call. This should cover the read path in
classifySubscription and isBrokerAccessAllowed alongside the current
ORDERS/write-disabled test.
🪄 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: b3d09285-e214-4584-9df9-cc18d262a6e5
📒 Files selected for processing (16)
.env.samplesrc/cli.tssrc/commands/start-broker.tssrc/handlers/execute-action/context.tssrc/handlers/execute-action/handler.tssrc/handlers/execute-action/order-book-call.tssrc/handlers/execute-action/orders.tssrc/handlers/execute-action/treasury-call.tssrc/handlers/execute-action/withdraw.tssrc/handlers/subscribe/handler.tssrc/helpers/broker-surface.tssrc/index.tssrc/server.tstest/broker-surface.test.tstest/execute-action-surface-rpc.test.tstest/subscribe-handler.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/handlers/execute-action/order-book-call.tssrc/handlers/execute-action/withdraw.tssrc/helpers/broker-surface.tssrc/commands/start-broker.tssrc/handlers/execute-action/context.tssrc/handlers/execute-action/treasury-call.tssrc/cli.tssrc/handlers/subscribe/handler.tssrc/index.tssrc/handlers/execute-action/orders.tssrc/handlers/execute-action/handler.tssrc/server.ts
🔇 Additional comments (18)
test/execute-action-surface-rpc.test.ts (2)
1-107: LGTM!
109-118: 🗄️ Data Integrity & IntegrationNo issue with
getServerargument order
brokerSurfaceis already the 8th positional argument insrc/server.ts, after the two optionalotelMetricsandbrokerArchiverslots, so these test calls match the signature.> Likely an incorrect or invalid review comment.test/subscribe-handler.test.ts (1)
1-1: LGTM!src/server.ts (1)
31-31: LGTM!Also applies to: 42-49
src/handlers/execute-action/context.ts (1)
45-64: LGTM!src/handlers/execute-action/handler.ts (1)
118-132: LGTM!Also applies to: 177-177
src/handlers/execute-action/order-book-call.ts (1)
44-47: LGTM!src/handlers/execute-action/orders.ts (1)
26-29: LGTM!Also applies to: 177-179, 282-284
src/handlers/execute-action/treasury-call.ts (1)
23-29: LGTM!src/handlers/execute-action/withdraw.ts (1)
21-24: LGTM!src/handlers/subscribe/handler.ts (1)
58-58: LGTM!Also applies to: 320-320, 391-407
src/helpers/broker-surface.ts (2)
1-166: LGTM!
190-198: 🎯 Functional CorrectnessNo type change needed here.
src/helpers/grpc/callbacks.tsalready returnsgrpc.ServiceErrorobjects with justcodeandmessage, and the handlers pass that same shape toctx.wrappedCallback, so this helper is consistent with the existing pattern.> Likely an incorrect or invalid review comment.test/broker-surface.test.ts (1)
1-111: LGTM!.env.sample (1)
11-17: LGTM!src/cli.ts (1)
5-5: LGTM!Also applies to: 58-64
src/commands/start-broker.ts (1)
1-33: LGTM!src/index.ts (1)
15-19: LGTM!Also applies to: 57-57, 206-211, 299-302, 317-317
Comment on lines
+20
to
+27
| .option( | ||
| "--no-read", | ||
| "Disable read operations (fetch actions and market streams)", | ||
| ) | ||
| .option( | ||
| "--no-write", | ||
| "Disable write operations (orders, withdrawals, and user streams)", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the Commander option property naming convention for negatable options
rg -n "no-read|no-write|noRead|noWrite" src/cli.tsRepository: usherlabs/cex-broker
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the CLI with line numbers.
sed -n '1,90p' src/cli.ts | cat -nRepository: usherlabs/cex-broker
Length of output: 2654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the Commander version and any local typing that may affect option names.
rg -n '"commander"|from "commander"|from '\''commander'\''' package.json src -g '!node_modules'Repository: usherlabs/cex-broker
Length of output: 250
Use options.read / options.write for these negated flags.
Commander maps --no-read and --no-write to the positive properties, so options.noRead and options.noWrite stay undefined and the surface restrictions never apply.
🐛 Proposed fix
- if (options.noRead) {
+ if (options.read === false) {
brokerSurface.readEnabled = false;
}
- if (options.noWrite) {
+ if (options.write === false) {
brokerSurface.writeEnabled = false;
}🤖 Prompt for 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.
In `@src/cli.ts` around lines 20 - 27, The negated Commander flags are being read
from the wrong option names, so the restrictions never take effect. Update the
CLI handling in cli.ts to use options.read and options.write for the --no-read
and --no-write flags, and adjust any related conditionals or defaults that
currently reference options.noRead or options.noWrite. Keep the fix centered on
the option parsing logic and the symbols that consume these flags.
Comment on lines
+190
to
+198
| export function buildBrokerSurfaceDeniedError( | ||
| access: BrokerAccessClass | "unknown", | ||
| ): grpc.ServiceError { | ||
| const surface = access === "write" || access === "unknown" ? "write" : "read"; | ||
| return { | ||
| code: grpc.status.FAILED_PRECONDITION, | ||
| message: `${surface === "write" ? "Write" : "Read"} operations are disabled on this broker deployment`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Misleading denial message for "unknown" CCXT methods on a write-only broker.
When access === "unknown", the function always reports "Write operations are disabled", even though an unknown method is actually denied because it requires both read and write enabled (isBrokerAccessAllowed, Line 179). On a write-only broker (readEnabled: false, writeEnabled: true), this message is factually wrong and could mislead operators troubleshooting denials.
💬 Suggested clarification
export function buildBrokerSurfaceDeniedError(
access: BrokerAccessClass | "unknown",
): grpc.ServiceError {
- const surface = access === "write" || access === "unknown" ? "write" : "read";
+ if (access === "unknown") {
+ return {
+ code: grpc.status.FAILED_PRECONDITION,
+ message:
+ "This operation requires both read and write access, which is not fully enabled on this broker deployment",
+ };
+ }
+ const surface = access;
return {
code: grpc.status.FAILED_PRECONDITION,
message: `${surface === "write" ? "Write" : "Read"} operations are disabled on this broker deployment`,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function buildBrokerSurfaceDeniedError( | |
| access: BrokerAccessClass | "unknown", | |
| ): grpc.ServiceError { | |
| const surface = access === "write" || access === "unknown" ? "write" : "read"; | |
| return { | |
| code: grpc.status.FAILED_PRECONDITION, | |
| message: `${surface === "write" ? "Write" : "Read"} operations are disabled on this broker deployment`, | |
| }; | |
| } | |
| export function buildBrokerSurfaceDeniedError( | |
| access: BrokerAccessClass | "unknown", | |
| ): grpc.ServiceError { | |
| if (access === "unknown") { | |
| return { | |
| code: grpc.status.FAILED_PRECONDITION, | |
| message: | |
| "This operation requires both read and write access, which is not fully enabled on this broker deployment", | |
| }; | |
| } | |
| const surface = access; | |
| return { | |
| code: grpc.status.FAILED_PRECONDITION, | |
| message: `${surface === "write" ? "Write" : "Read"} operations are disabled on this broker deployment`, | |
| }; | |
| } |
🤖 Prompt for 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.
In `@src/helpers/broker-surface.ts` around lines 190 - 198, The denial message in
buildBrokerSurfaceDeniedError is misleading for access === "unknown" because it
always maps to a write-disabled message. Update the logic in
buildBrokerSurfaceDeniedError to distinguish unknown CCXT methods from true
write-only denials and emit a message that reflects the actual broker
requirement in isBrokerAccessAllowed, using the existing access value and the
read/write enabled context rather than defaulting unknown to write.
Member
|
Closed as auth gate per read/write model is excessive. |
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.
…902)
Summary by CodeRabbit
FAILED_PRECONDITIONerrors.