Feat/matching engine #12
Conversation
WalkthroughThis pull request introduces an order cancellation feature across the trading platform. It adds a CancelOrder gRPC service to the matching engine and order service, implements actor-based cancellation logic with order book management, exposes a cancel order HTTP endpoint via the API gateway, updates protobuf message schemas, and adds validation for cancel requests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIGateway as API Gateway
participant Validator
participant OrderService
participant MatchingEngine as Matching Engine
participant ActorRegistry as Actor Registry
Client->>APIGateway: POST /order/:id (cancel request)
APIGateway->>Validator: Validate CancelOrderRequest
alt Validation fails
Validator-->>APIGateway: Error
APIGateway-->>Client: 400 Bad Request
else Validation passes
Validator-->>APIGateway: Valid
APIGateway->>OrderService: gRPC CancelOrder(id, userId, symbol)
OrderService->>MatchingEngine: gRPC CancelOrder(id, userId, symbol)
MatchingEngine->>ActorRegistry: Look up symbol actor
alt Symbol not found
ActorRegistry-->>MatchingEngine: Error
MatchingEngine-->>OrderService: Error response
else Symbol found
ActorRegistry->>ActorRegistry: Send CancelOrderMsg to actor inbox
ActorRegistry->>ActorRegistry: SymbolActor processes message
rect rgb(200, 220, 255)
note over ActorRegistry: CanCancelOrderInternal:<br/>validate ownership,<br/>remove from order book,<br/>emit event
end
ActorRegistry-->>MatchingEngine: CancelOrderInternalResponse
MatchingEngine-->>OrderService: CancelOrderResponse (id, status, cancelled_quantity)
end
OrderService-->>APIGateway: gRPC response
APIGateway-->>Client: 200 OK {status, message, data}
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Buf (1.61.0)packages/proto-defs/proto/engine/order_matching.protoFailure: no .proto files were targeted. This can occur if no .proto files are found in your input, --path points to files that do not exist, or --exclude-path excludes all files. packages/proto-defs/proto/api/order_service.protoFailure: no .proto files were targeted. This can occur if no .proto files are found in your input, --path points to files that do not exist, or --exclude-path excludes all files. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
🧹 Nitpick comments (17)
packages/validator/src/Order.ts (1)
29-36: Consider adding validation for the symbol field.The
symbolfield currently only validates that it's a string. If there's a known set of valid symbols (like the ones initialized in the matching engine: BTCUSD, ETHUSD, SOLUSD), consider usingz.enum()to restrict the valid values and catch invalid symbols early.🔎 Example with enum validation
export const CancelOrderValidator = z.object({ params: z.object({ id: z.uuid(), }), body: z.object({ - symbol: z.string(), + symbol: z.enum(["BTCUSD", "ETHUSD", "SOLUSD"]), }), });Alternatively, if the symbol list should be dynamic, consider a pattern validation like:
symbol: z.string().regex(/^[A-Z]{6,10}$/)apps/api-gateway/src/routers/order.route.ts (1)
36-41: Consider using DELETE method for order cancellation.Using POST for a cancellation operation is not RESTful. The DELETE HTTP method would be more semantically appropriate for canceling/removing an order. This would also eliminate the need to pass
symbolin the request body, as the order ID should be sufficient to identify and cancel the order.🔎 Suggested refactor using DELETE
-router.post( +router.delete( "/:id", zodValidatorMiddleware(CancelOrderValidator), (req: CancelOrderRequest, res: Response, next: NextFunction) => orderController.cancelOrder(req, res, next), );This would also allow you to simplify the validator to only require the
idparam if you can derive the symbol from the order lookup.apps/order-service/src/controllers/order.controller.ts (1)
87-91: Simplify redundant object construction.The
requestBodyobject is redundantly mapping properties fromcall.request. You can directly use the destructured request or passcall.requestif it matches theCancelOrderRequesttype.🔎 Simplified approach
async cancelOrder( call: grpc.ServerUnaryCall<CancelOrderRequest, CancelOrderResponse>, callback: grpc.sendUnaryData<CancelOrderResponse>, ): Promise<void> { - const body = call.request; - const requestBody = { - id: body.id, - userId: body.userId, - symbol: body.symbol, - } as CancelOrderRequest; + const { id, userId, symbol } = call.request; try { const response = await new Promise<CancelOrderResponse>((resolve, reject) => { this.matchingEngineClient.cancelOrder( - requestBody, + { id, userId, symbol }, (err: grpc.ServiceError | null, res: CancelOrderResponse) => {apps/matching-engine/cmd/matching-engine/main.go (1)
35-39: Move hardcoded symbols to configuration.The symbol list is hardcoded in
main.go. Consider moving these to environment variables or a configuration file to allow for easier updates without code changes and to support different environments (dev/staging/prod) with different symbol sets.🔎 Suggested configuration-driven approach
Example using environment variable:
// Read from environment or config file symbolsStr := os.Getenv("TRADING_SYMBOLS") // e.g., "BTCUSD,ETHUSD,SOLUSD" if symbolsStr == "" { symbolsStr = "BTCUSD,ETHUSD,SOLUSD" // fallback } symbols := strings.Split(symbolsStr, ",") internal.StartActors(symbols)apps/api-gateway/src/controllers/order.controller.ts (1)
50-54: Use ES6 property shorthand.The object construction uses redundant property assignments. Use ES6 shorthand for cleaner code.
🔎 Simplified approach
const requestBody = { - id: id, - userId: userId, + id, + userId, symbol: req.body.symbol, };apps/matching-engine/internal/server.go (1)
20-20: Consider using the context parameter.Both
PlaceOrderandCancelOrderreceive acontext.Contextparameter but don't use it. If these operations can be long-running or involve downstream calls, consider propagating the context to respect cancellation signals and timeouts. This is especially important for gRPC services to handle client disconnections gracefully.This would require updating the internal
PlaceOrderandCancelOrderfunctions inactor_registy.goto accept and respect context, which could be a broader refactor.Also applies to: 70-70
apps/matching-engine/internal/actor_registy.go (1)
1-1: Typo in filename: "registy" should be "registry".The filename
actor_registy.gocontains a typo. Consider renaming toactor_registry.gofor clarity.packages/proto-defs/proto/engine/order_matching.proto (2)
26-26: Inconsistent field naming:executedValueshould beexecuted_value.Protobuf convention recommends
snake_casefor field names. This field uses camelCase while surrounding fields use snake_case.🔎 Proposed fix
- int64 executedValue = 4; + int64 executed_value = 4;
51-55: Inconsistent status type:CancelOrderResponse.statusisstringwhilePlaceOrderResponse.statususescommon.order.OrderStatusenum.Using a string for status in
CancelOrderResponse(line 53) whilePlaceOrderResponseuses the enum type (line 39) creates inconsistency. Consider using the same enum for type safety and consistency across the API.🔎 Proposed fix
message CancelOrderResponse { string id = 1; - string status = 2; + common.order.OrderStatus status = 2; optional string status_message = 3; int64 cancelled_quantity = 4; }packages/proto-defs/proto/api/order_service.proto (2)
32-32: Same naming inconsistency:executedValueshould beexecuted_value.Apply the same fix as in
order_matching.protofor consistency across proto files.
51-55: Same status type inconsistency as inorder_matching.proto.
CancelOrderResponse.statusshould usecommon.order.OrderStatusenum instead of string for consistency withCreateOrderResponse.apps/matching-engine/internal/engine.go (6)
68-69: Consistency issue:Pushaddsorder.Quantitybut the order book tracks resting orders.When an order is pushed to the book, it should contribute its
RemainingQuantity(what's actually resting), not its originalQuantity. For new orders these may be equal, but this could cause issues if orders are ever re-added.🔎 Proposed fix
- pl.TotalVolume += uint64(order.Quantity) + pl.TotalVolume += uint64(order.RemainingQuantity)
377-378: Potential division by zero ifFilledQuantityis zero.Although the loop guard ensures matching occurred, adding a defensive check prevents issues if the logic changes.
🔎 Proposed fix
- incoming.AveragePrice = incoming.ExecutedValue / int64(incoming.FilledQuantity) - restingOrder.AveragePrice = restingOrder.ExecutedValue / int64(restingOrder.FilledQuantity) + if incoming.FilledQuantity > 0 { + incoming.AveragePrice = incoming.ExecutedValue / int64(incoming.FilledQuantity) + } + if restingOrder.FilledQuantity > 0 { + restingOrder.AveragePrice = restingOrder.ExecutedValue / int64(restingOrder.FilledQuantity) + }
45-54: Type inconsistency:TotalVolumeisuint64but quantities areint64.The
Order.Quantityand related fields areint64, butPriceLevel.TotalVolumeisuint64. This requires explicit casts throughout and could cause issues with negative values (though quantities shouldn't be negative). Consider aligning types.
666-668: Avoid panic in production code for unknown message types.Panicking on an unknown message type will crash the actor goroutine. Consider logging and continuing instead.
🔎 Proposed fix
+ default: + // Log unknown message type and continue processing + fmt.Printf("warning: unknown actor message type: %T\n", msg) - default: - panic("unknown actor message")
673-675: EmptyPublishKafkaEventsstub - events are silently dropped.This is a placeholder that discards all engine events. Ensure this is tracked for implementation.
Would you like me to open an issue to track the Kafka event publishing implementation?
124-124: Redundant type cast:priceis alreadyint64.The cast
int64(price)is unnecessary sincepriceis already of typeint64.🔎 Proposed fix
- level = &PriceLevel{Price: int64(price)} + level = &PriceLevel{Price: price}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api-gateway/src/controllers/order.controller.tsapps/api-gateway/src/routers/order.route.tsapps/api-gateway/src/types/index.tsapps/matching-engine/cmd/matching-engine/main.goapps/matching-engine/internal/actor_registy.goapps/matching-engine/internal/engine.goapps/matching-engine/internal/server.goapps/order-service/src/controllers/order.controller.tsapps/order-service/src/grpc.server.tspackages/proto-defs/proto/api/order_service.protopackages/proto-defs/proto/engine/order_matching.protopackages/validator/src/Order.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/order-service/src/controllers/order.controller.ts (1)
apps/api-gateway/src/types/index.ts (1)
CancelOrderRequest(53-56)
apps/api-gateway/src/types/index.ts (1)
packages/validator/src/Order.ts (1)
CancelOrder(37-37)
packages/validator/src/Order.ts (1)
apps/matching-engine/internal/actor_registy.go (1)
CancelOrder(37-60)
apps/api-gateway/src/routers/order.route.ts (2)
packages/validator/src/Order.ts (1)
CancelOrderValidator(29-36)apps/api-gateway/src/types/index.ts (1)
CancelOrderRequest(53-56)
apps/api-gateway/src/controllers/order.controller.ts (1)
apps/api-gateway/src/types/index.ts (1)
CancelOrderRequest(53-56)
apps/matching-engine/internal/server.go (4)
apps/matching-engine/internal/actor_registy.go (2)
PlaceOrder(15-35)CancelOrder(37-60)packages/validator/src/Order.ts (2)
PlaceOrder(27-27)CancelOrder(37-37)apps/matching-engine/internal/engine.go (1)
Order(11-38)apps/api-gateway/src/types/index.ts (1)
CancelOrderRequest(53-56)
apps/matching-engine/cmd/matching-engine/main.go (2)
apps/matching-engine/internal/server.go (1)
Server(16-18)apps/matching-engine/internal/actor_registy.go (1)
StartActors(7-13)
🔇 Additional comments (5)
apps/order-service/src/grpc.server.ts (1)
28-28: LGTM!The cancelOrder binding follows the same pattern as the existing createOrder binding and correctly integrates the new RPC endpoint.
apps/order-service/src/controllers/order.controller.ts (1)
82-132: LGTM! Consistent error handling pattern.The cancelOrder implementation correctly mirrors the structure and error handling approach of the existing placeOrder method, maintaining consistency across the codebase.
apps/api-gateway/src/types/index.ts (1)
53-56: LGTM!The
CancelOrderRequestinterface correctly extends ExpressRequestand properly references the validator types, following the established pattern used byCreateOrderRequest.apps/matching-engine/cmd/matching-engine/main.go (1)
20-21: LGTM! Good addition of structured logging.The structured JSON logger using
slogis a good improvement for production observability.apps/matching-engine/internal/actor_registy.go (1)
5-5: Global map access may need synchronization if actors can be added dynamically.The
actorsmap is written duringStartActorsand read inPlaceOrder/CancelOrder. IfStartActorsis guaranteed to complete before any order operations and no dynamic symbol addition is planned, this is safe. Otherwise, consider usingsync.RWMutexto protect concurrent access.
| cancelOrder = (req: CancelOrderRequest, res: Response, next: NextFunction) => { | ||
| const id = req.params.id; | ||
| const userId = "d8036c81-a1d7-45de-b4d8-e3847bfadd3b"; // TODO: replace with authenticated userId --> req.userId | ||
|
|
||
| const requestBody = { | ||
| id: id, | ||
| userId: userId, | ||
| symbol: req.body.symbol, | ||
| }; | ||
|
|
||
| this.grpcEngine.cancelOrder( | ||
| requestBody, | ||
| (err: grpc.ServiceError | null, response: CancelOrderResponse) => { | ||
| if (err) return next(err); | ||
|
|
||
| this.logger.info("Order Cancelled", { response }); | ||
| res | ||
| .status(200) | ||
| .json({ status: "success", message: "Order Cancelled Successfully", data: response }); | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Add authorization check to prevent unauthorized order cancellation.
The current implementation allows any authenticated user to cancel any order by ID, as there's no verification that the order belongs to the requesting user. While userId is included in the request to the matching engine, a malicious user could potentially cancel others' orders by guessing or enumerating order IDs.
Consider adding authorization logic to verify that req.userId (once implemented) matches the owner of the order being canceled, either in this controller or in the matching engine layer.
Example check:
// After fetching order details or in matching engine
if (order.userId !== req.userId) {
throw new Error('Unauthorized: Cannot cancel orders belonging to other users');
}🤖 Prompt for AI Agents
In apps/api-gateway/src/controllers/order.controller.ts around lines 46 to 67,
remove the hardcoded userId and add an authorization check: obtain the
authenticated userId from the request (req.userId once implemented), fetch the
order owner (either by retrieving order details from the matching engine or a
dedicated order service) and verify order.userId === req.userId before calling
cancelOrder; if they don't match, short-circuit with a 403 Forbidden (or pass an
authorization error to next), otherwise proceed to call the matching engine with
the authenticated userId.
| select { | ||
| case res := <-replyCh: | ||
| return res, nil | ||
| case err := <-errCh: | ||
| return nil, fmt.Errorf("failed to process the order. Error: %v", err) | ||
| } |
There was a problem hiding this comment.
Add timeout to prevent indefinite blocking if the actor hangs or crashes.
The select statement waits indefinitely for a response. If the actor goroutine panics or becomes unresponsive, this call will block forever, potentially exhausting resources.
🔎 Proposed fix using context with timeout
+import (
+ "context"
+ "fmt"
+ "time"
+)
+
-func PlaceOrder(order *Order) (*AddOrderInternalResponse, error) {
+func PlaceOrder(ctx context.Context, order *Order) (*AddOrderInternalResponse, error) {
actor, ok := actors[order.Symbol]
if !ok {
return nil, fmt.Errorf("unknown symbol %s", order.Symbol)
}
replyCh := make(chan *AddOrderInternalResponse, 1)
errCh := make(chan error, 1)
actor.inbox <- PlaceOrderMsg{
Order: order,
Reply: replyCh,
Err: errCh,
}
select {
case res := <-replyCh:
return res, nil
case err := <-errCh:
return nil, fmt.Errorf("failed to process the order. Error: %v", err)
+ case <-ctx.Done():
+ return nil, ctx.Err()
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/matching-engine/internal/actor_registy.go around lines 29 to 34, the
select that waits on replyCh and errCh can block forever if the actor hangs or
panics; add a timeout case so callers don't wait indefinitely. Wrap the receive
in a context with a reasonable timeout (or add a time.After channel) and include
a case in the select that returns a timeout error when the timer fires; ensure
any created context is properly canceled where needed and return a clear timeout
error message to the caller.
| select { | ||
| case res := <-replyCh: | ||
| return res, nil | ||
| case err := <-errCh: | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Same timeout concern applies to CancelOrder.
Apply consistent timeout handling as suggested for PlaceOrder.
🤖 Prompt for AI Agents
In apps/matching-engine/internal/actor_registy.go around lines 54 to 59, the
CancelOrder reply/err select lacks a timeout like PlaceOrder does; update
CancelOrder to apply the same timeout behavior by using the configured timeout
(either via context with deadline or a time.After channel) in the select,
returning a clear timeout error when the timeout elapses and otherwise returning
the received response or error, ensuring symmetry with PlaceOrder's handling.
| pl.TotalVolume -= uint64(order.Quantity) | ||
| pl.OrderCount-- |
There was a problem hiding this comment.
Volume tracking bug: uses order.Quantity instead of order.RemainingQuantity.
When removing a partially filled order, order.Quantity reflects the original quantity, not what remains. This will incorrectly decrement TotalVolume by more than the order actually contributed to the book.
🔎 Proposed fix
- pl.TotalVolume -= uint64(order.Quantity)
+ pl.TotalVolume -= uint64(order.RemainingQuantity)Also verify if line 68 should use RemainingQuantity instead of Quantity when pushing orders:
#!/bin/bash
# Check how orders are pushed - if they're pushed with full Quantity initially,
# then Remove should also use RemainingQuantity for correctness
rg -n "\.Push\(" apps/matching-engine/🤖 Prompt for AI Agents
In apps/matching-engine/internal/engine.go around lines 85-86, TotalVolume is
decremented by order.Quantity which is the original size; change this to
subtract order.RemainingQuantity so removal only removes the outstanding volume
contributed by that order, and decrement OrderCount as before. Also check the
code at line ~68 where orders are initially pushed and ensure orders are
recorded using RemainingQuantity (or RemainingQuantity is initialized to
Quantity) so the two sides are consistent; adjust the push/initialization to set
RemainingQuantity equal to Quantity if needed.
| // Self-trade prevention | ||
| if incoming.UserID == restingOrder.UserID { | ||
| break // skip resting order | ||
| } |
There was a problem hiding this comment.
Self-trade prevention breaks the matching loop entirely.
When a self-trade is detected, the loop breaks, stopping all matching even if other non-self orders exist at the same or worse price levels. Consider using continue with logic to skip to the next resting order instead.
🔎 Proposed approach
The current implementation stops matching entirely when encountering self-trade. A more robust approach would skip the current resting order and continue matching against other orders at the same or subsequent price levels. This requires iterating through orders at the price level rather than breaking.
// Self-trade prevention
if incoming.UserID == restingOrder.UserID {
- break // skip resting order
+ // Skip this resting order, try next one at same level or next level
+ // This requires refactoring the loop to iterate through orders
+ bestPriceLevel.Remove(restingOrder)
+ // Re-add to end of queue or handle according to STP policy
+ continue
}Note: The exact behavior depends on the desired self-trade prevention policy (cancel newest, cancel oldest, cancel both, etc.).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/matching-engine/internal/engine.go around lines 356 to 359, the current
self-trade check uses "break" which exits the entire matching loop; change this
to skip only the current resting order and continue matching other resting
orders at the same or worse price levels. Replace the break with logic that
advances to the next resting order (e.g., continue the inner loop or move the
resting-order pointer) so the matcher doesn't stop processing the price level;
ensure any per-policy self-trade handling (skip/cancel newest/oldest/both) is
applied before continuing and that loop indices/iterators remain consistent to
avoid skipping or re-processing orders.
| } | ||
|
|
||
| func (me *MatchingEngine) ExecuteTrade(aggressor *Order, restingOrder *Order, matchQuantity int64, matchPrice int64) Trade { | ||
| me.TradeSequence = +1 |
There was a problem hiding this comment.
Critical bug: me.TradeSequence = +1 assigns 1 instead of incrementing.
The unary + operator just returns the value, so this always assigns 1 to TradeSequence. Every trade will have the same sequence number, breaking trade ID uniqueness.
🔎 Proposed fix
- me.TradeSequence = +1
+ me.TradeSequence++📝 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.
| me.TradeSequence = +1 | |
| me.TradeSequence++ |
🤖 Prompt for AI Agents
In apps/matching-engine/internal/engine.go around line 443, the statement
"me.TradeSequence = +1" wrongly assigns the literal 1 instead of incrementing;
replace it with an increment operation (e.g. "me.TradeSequence += 1" or
"me.TradeSequence++") so the sequence advances per trade, and if this field can
be accessed concurrently ensure the increment is protected (mutex or atomic) as
appropriate.
| return nil, err | ||
| } | ||
|
|
||
| fmt.Println(res.Order, res.Trades) |
There was a problem hiding this comment.
Remove debug print statement.
The fmt.Println should be removed or replaced with proper structured logging using slog.
🔎 Suggested fix
- fmt.Println(res.Order, res.Trades)
+ slog.Info("Order processed",
+ "orderId", res.Order.ClientOrderID,
+ "status", res.Order.Status,
+ "tradesCount", len(res.Trades),
+ )Or simply remove it if the information is already logged elsewhere.
📝 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.
| fmt.Println(res.Order, res.Trades) | |
| slog.Info("Order processed", | |
| "orderId", res.Order.ClientOrderID, | |
| "status", res.Order.Status, | |
| "tradesCount", len(res.Trades), | |
| ) |
🤖 Prompt for AI Agents
In apps/matching-engine/internal/server.go around line 47, remove the debug
fmt.Println(res.Order, res.Trades) and replace it with a structured slog call
(or remove entirely if already logged elsewhere); specifically, use slog to log
at the appropriate level and include fields like "order" and "trades" (e.g.,
slog.Debug/Info with key/value pairs) so the output is structured and consistent
with the service's logging, or simply delete the line if redundant.
| if err != nil { | ||
| slog.Error("Failed to cancel order") | ||
| return nil, err |
There was a problem hiding this comment.
Enhance error logging with request context.
The error log in CancelOrder lacks important details compared to the PlaceOrder error logging. Include request information like orderId, userId, and the actual error for better debugging.
🔎 Suggested improvement
if err != nil {
- slog.Error("Failed to cancel order")
+ slog.Error("Failed to cancel order",
+ "Error", err,
+ "orderId", req.Id,
+ "userId", req.UserId,
+ "symbol", req.Symbol,
+ )
return nil, err
}🤖 Prompt for AI Agents
In apps/matching-engine/internal/server.go around lines 74 to 76, the
CancelOrder error log is missing request context and the actual error; update
the logging call to include the orderId and userId from the request plus the
error value (use structured key/value fields consistent with PlaceOrder logging)
so the log reads a descriptive message with fields like "orderId", "userId", and
"err" to aid debugging before returning the error.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.