fix(orders): preserve venue error codes for passive submissions - #91
Conversation
WalkthroughPassive-order error handling now preserves authentication and insufficient-funds broker errors, assigns stable error codes, maps those codes to gRPC statuses, and adds test coverage for the updated behavior. ChangesPassive-order error propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 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/handlers/execute-action/orders.ts`:
- Around line 212-214: Update the stable message composition in the
passive-order error handler around classifyPassiveOrderError and
rejectWithGrpcError to use a sanitized error detail without its class-name
prefix. Keep stableErrorCode as the sole error class identifier while preserving
the remaining sanitized detail.
🪄 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: 694b3fbe-5301-4352-9984-b54dbe3bfd31
📒 Files selected for processing (5)
src/handlers/execute-action/orders.tssrc/helpers/grpc/status.tssrc/helpers/passive-order.tstest/grpc-status.test.tstest/passive-order.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/orders.tssrc/helpers/grpc/status.tssrc/helpers/passive-order.ts
🔇 Additional comments (4)
src/helpers/passive-order.ts (1)
13-16: LGTM!Also applies to: 48-61
src/helpers/grpc/status.ts (1)
6-11: LGTM!test/grpc-status.test.ts (1)
12-17: LGTM!test/passive-order.test.ts (1)
206-242: LGTM!Also applies to: 259-261
| const stableErrorCode = classifyPassiveOrderError(error); | ||
| return rejectWithGrpcError(ctx, error, { | ||
| message: `${passiveErrorCode}: ${sanitizeErrorDetail(error)}`, | ||
| message: `${stableErrorCode}: ${sanitizeErrorDetail(error)}`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid duplicating the CCXT class in the stable message.
sanitizeErrorDetail(error) already includes errorClassName(error), so this produces messages such as InsufficientFunds: InsufficientFunds: ... and AuthenticationError: AuthenticationError: .... Use a sanitized detail that omits the class-name prefix when composing the stable code message.
🤖 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/handlers/execute-action/orders.ts` around lines 212 - 214, Update the
stable message composition in the passive-order error handler around
classifyPassiveOrderError and rejectWithGrpcError to use a sanitized error
detail without its class-name prefix. Keep stableErrorCode as the sole error
class identifier while preserving the remaining sanitized detail.
1 participant
Problem
passive_order_rejectedis the code that tells a client "the venue refused to rest this order for a post-only reason" — which is the client's cue that the price level may be re-placed.classifyPassiveOrderErrorwas an allowlist for two post-only faults (would-cross, post-only-unsupported) with a catch-all that assignedpassive_order_rejectedto everything else that failed during a passive submission.ccxtraisesInsufficientFundsfrom the submission call itself, so a plain balance shortfall was reported as a post-only rejection. The client then re-placed the rung, which failed identically, on repeat. In production this ran at 800–1500 failed venue submissions per hour across two markets for as long as the account was short of quote currency — no incorrect exposure (it fails closed), but wasted venue order-rate budget and heavy log/archive noise.Change
Balance and credential faults on the passive create path now report their own stable error codes instead of a
passive_*code:ccxt.InsufficientFunds→InsufficientFunds:(FAILED_PRECONDITION)ccxt.AuthenticationError→AuthenticationError:(UNAUTHENTICATED);PermissionDeniedsubclasses it, so one check covers bothBoth are already established codes in the shared error vocabulary, and the existing convention is that the stable code leads the message — which matters, because clients recover the error kind by reading the leading token. Routing these through the generic
Order Creation failed: …fallback would not work: that message leads withOrder, which classifies as unknown, and returnsINTERNALfor what is a precondition failure.Detection is by typed
ccxtclass, not message text. The text heuristics remain only for would-cross and post-only-unsupported, where some venues report without a distinct class. The new checks precede those, which is safe: the class hierarchies are disjoint (verified against theccxtprototype chain).The classification decision stays in one place — the handler emits whatever code the classifier returns.
Not changed
The
submissionstate gate (not_attempted/in_flight/placed), the would-cross and unsupported heuristics, telemetry/archive emission, and the generic fallback are all untouched. No retry, backoff, or balance pre-check is added: the broker reports what the venue said, and the client decides what to do about it.Verification
New coverage: a balance fault yields an
InsufficientFunds:-leading message with nopassive_prefix; authentication and permission faults yieldAuthenticationError:; existing would-cross / unsupported / generic-rejection classifications and the pre-submission and post-submissionsubmission-state behavior are unchanged.Release ordering
The consuming client needs a companion change to treat a venue balance fault as terminal rather than retryable. Without it, the client's stock retry path makes a bounded burst of doomed submissions instead of an unbounded loop — an improvement, but the two should ship together for the intended single-attempt behavior.
Companion client-side change: usherlabs/fiet-maker#965
Summary by CodeRabbit