feat: modify endpoint #13
Conversation
WalkthroughThis PR introduces order modification capability across the trading system by extending protobuf definitions with ModifyOrderRequest/ModifyOrderResponse messages, implementing RPC handlers in the order service and matching engine, adding actor-based ModifyOrder messaging, introducing validation schemas, and wiring the new modifyOrder endpoint in the API gateway. Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIGateway as API Gateway
participant OrderService
participant MatchingEngine
participant OrderActor
Client->>APIGateway: PATCH /:id (modifyOrder)
APIGateway->>APIGateway: Validate request (ModifyOrderValidator)
APIGateway->>OrderService: ModifyOrderRequest (orderId, symbol, newPrice, newQuantity)
OrderService->>OrderService: Generate clientModifyId
OrderService->>MatchingEngine: ModifyOrder RPC (symbol, orderId, userId, clientModifyId, newPrice, newQuantity)
MatchingEngine->>OrderActor: Send ModifyOrderMsg
OrderActor->>OrderActor: Validate order ownership & symbol
OrderActor->>OrderActor: Determine replace vs. reduce logic
alt Replace Flow
OrderActor->>OrderActor: Cancel old order
OrderActor->>OrderActor: Create new order with updated fields
else Reduce Flow
OrderActor->>OrderActor: Update quantity, emit ORDER_UPDATED event
end
OrderActor-->>MatchingEngine: ModifyOrderInternalResponse
MatchingEngine-->>OrderService: ModifyOrderResponse (orderId, oldOrderId, newOrderId, status)
OrderService-->>APIGateway: ModifyOrderResponse
APIGateway-->>Client: 200 OK (response data)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
apps/api-gateway/src/controllers/order.controller.ts (1)
90-90: Inconsistent logger argument format.Other handlers use
{ response }object format (e.g., line 43:this.logger.info("Order placed", { response })), but this uses positional argument. This may affect log formatting consistency.Suggested fix
- this.logger.info("Order Modified", response); + this.logger.info("Order Modified", { response });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/api-gateway/src/controllers/order.controller.tsapps/api-gateway/src/routers/order.route.tsapps/api-gateway/src/types/index.tsapps/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 (6)
packages/validator/src/Order.ts (2)
apps/matching-engine/internal/actor_registy.go (1)
ModifyOrder(62-95)apps/api-gateway/src/middlewares/zod.validator.middleware.ts (2)
T(6-31)req(7-30)
apps/matching-engine/internal/actor_registy.go (2)
packages/validator/src/Order.ts (1)
ModifyOrder(61-61)apps/matching-engine/internal/engine.go (2)
ModifyOrderInternalResponse(607-613)ModifyOrderMsg(783-792)
apps/matching-engine/internal/server.go (3)
apps/matching-engine/internal/actor_registy.go (1)
ModifyOrder(62-95)packages/validator/src/Order.ts (1)
ModifyOrder(61-61)apps/api-gateway/src/types/index.ts (1)
ModifyOrderRequest(58-61)
apps/api-gateway/src/types/index.ts (2)
apps/matching-engine/internal/actor_registy.go (1)
ModifyOrder(62-95)packages/validator/src/Order.ts (1)
ModifyOrder(61-61)
apps/api-gateway/src/routers/order.route.ts (2)
packages/validator/src/Order.ts (1)
ModifyOrderValidator(39-60)apps/api-gateway/src/types/index.ts (1)
ModifyOrderRequest(58-61)
apps/api-gateway/src/controllers/order.controller.ts (1)
apps/api-gateway/src/types/index.ts (1)
ModifyOrderRequest(58-61)
🔇 Additional comments (11)
packages/proto-defs/proto/engine/order_matching.proto (1)
58-78: LGTM! Well-designed modify order messages.The proto definitions are well-structured with:
- Proper idempotency handling via
client_modify_id- Flexible modification (either price or quantity or both)
- Clear response structure distinguishing old vs new order IDs (supporting cancel-replace pattern)
Also applies to: 83-83
apps/order-service/src/grpc.server.ts (1)
29-29: LGTM! Correctly wired modifyOrder RPC.The new RPC is properly bound to the controller method following the same pattern as existing RPCs.
packages/validator/src/Order.ts (1)
51-59: Good validation: Ensures at least one field is modified.The refinement correctly enforces that either
newPriceornewQuantity(or both) must be provided, preventing no-op modification requests.apps/matching-engine/internal/server.go (1)
95-101: LGTM!The response mapping correctly translates
ModifyOrderInternalResponsefields topb.ModifyOrderResponse.apps/matching-engine/internal/actor_registy.go (1)
62-95: LGTM!The implementation follows the established actor pattern used by
PlaceOrderandCancelOrder. The buffered channels and select-based synchronization are consistent with the existing code.apps/api-gateway/src/types/index.ts (1)
57-61: LGTM!The
ModifyOrderRequestinterface follows the same pattern asCancelOrderRequest, properly typing bothparamsandbodyfrom the validator schema.apps/api-gateway/src/controllers/order.controller.ts (1)
72-99: LGTM!The
modifyOrderhandler follows the established pattern and correctly maps request parameters to the gRPC request type.apps/order-service/src/controllers/order.controller.ts (1)
137-191: Implementation follows established patterns.The
modifyOrderhandler correctly generates aclientModifyId, constructs the request, and handles both success and error cases consistently withplaceOrderandcancelOrder.apps/matching-engine/internal/engine.go (3)
84-84: LGTM!Using
RemainingQuantityinstead ofQuantitywhen removing an order is correct—only the unfilled portion should be subtracted from the price level's total volume.
607-666: LGTM!The
ModifyOrderInternallogic correctly handles validation (ownership, symbol, status), distinguishes between price changes, quantity increases (requiring replace), and quantity reductions (in-place update).
783-792: LGTM!The
ModifyOrderMsgstruct correctly captures all required fields for the modification request, including optionalNewPriceandNewQuantitypointers.
| func (me *MatchingEngine) reduceOrder( | ||
| order *Order, | ||
| newQuantity *int64, | ||
| ) ([]EngineEvent, error) { | ||
|
|
||
| if newQuantity == nil { | ||
| return nil, fmt.Errorf("quantity required") | ||
| } | ||
|
|
||
| executed := order.Quantity - order.RemainingQuantity | ||
| newRemaining := *newQuantity - executed | ||
|
|
||
| if newRemaining < 0 { | ||
| return nil, fmt.Errorf("invalid reduce") | ||
| } | ||
|
|
||
| order.Quantity = *newQuantity | ||
| order.RemainingQuantity = newRemaining | ||
|
|
||
| // snapshot update (NO semantic meaning here) | ||
| return []EngineEvent{ | ||
| { | ||
| Type: uint8(EngineEventType_ORDER_UPDATED), | ||
| Data: struct { | ||
| OrderID string | ||
| Price int64 | ||
| Quantity int64 | ||
| RemainingQuantity int64 | ||
| }{ | ||
| OrderID: order.ClientOrderID, | ||
| Price: order.Price, | ||
| Quantity: order.Quantity, | ||
| RemainingQuantity: order.RemainingQuantity, | ||
| }, | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
reduceOrder doesn't update PriceLevel.TotalVolume, causing data inconsistency.
When reducing an order's quantity, order.Quantity and order.RemainingQuantity are updated, but the associated PriceLevel.TotalVolume is not adjusted. This causes the order book's aggregated volume at that price level to become stale.
Suggested fix
func (me *MatchingEngine) reduceOrder(
order *Order,
newQuantity *int64,
) ([]EngineEvent, error) {
if newQuantity == nil {
return nil, fmt.Errorf("quantity required")
}
executed := order.Quantity - order.RemainingQuantity
newRemaining := *newQuantity - executed
if newRemaining < 0 {
return nil, fmt.Errorf("invalid reduce")
}
+ // Update PriceLevel volume
+ if order.PriceLevel != nil {
+ volumeDelta := order.RemainingQuantity - newRemaining
+ order.PriceLevel.TotalVolume -= uint64(volumeDelta)
+ }
+
order.Quantity = *newQuantity
order.RemainingQuantity = newRemaining📝 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.
| func (me *MatchingEngine) reduceOrder( | |
| order *Order, | |
| newQuantity *int64, | |
| ) ([]EngineEvent, error) { | |
| if newQuantity == nil { | |
| return nil, fmt.Errorf("quantity required") | |
| } | |
| executed := order.Quantity - order.RemainingQuantity | |
| newRemaining := *newQuantity - executed | |
| if newRemaining < 0 { | |
| return nil, fmt.Errorf("invalid reduce") | |
| } | |
| order.Quantity = *newQuantity | |
| order.RemainingQuantity = newRemaining | |
| // snapshot update (NO semantic meaning here) | |
| return []EngineEvent{ | |
| { | |
| Type: uint8(EngineEventType_ORDER_UPDATED), | |
| Data: struct { | |
| OrderID string | |
| Price int64 | |
| Quantity int64 | |
| RemainingQuantity int64 | |
| }{ | |
| OrderID: order.ClientOrderID, | |
| Price: order.Price, | |
| Quantity: order.Quantity, | |
| RemainingQuantity: order.RemainingQuantity, | |
| }, | |
| }, | |
| }, nil | |
| } | |
| func (me *MatchingEngine) reduceOrder( | |
| order *Order, | |
| newQuantity *int64, | |
| ) ([]EngineEvent, error) { | |
| if newQuantity == nil { | |
| return nil, fmt.Errorf("quantity required") | |
| } | |
| executed := order.Quantity - order.RemainingQuantity | |
| newRemaining := *newQuantity - executed | |
| if newRemaining < 0 { | |
| return nil, fmt.Errorf("invalid reduce") | |
| } | |
| // Update PriceLevel volume | |
| if order.PriceLevel != nil { | |
| volumeDelta := order.RemainingQuantity - newRemaining | |
| order.PriceLevel.TotalVolume -= uint64(volumeDelta) | |
| } | |
| order.Quantity = *newQuantity | |
| order.RemainingQuantity = newRemaining | |
| // snapshot update (NO semantic meaning here) | |
| return []EngineEvent{ | |
| { | |
| Type: uint8(EngineEventType_ORDER_UPDATED), | |
| Data: struct { | |
| OrderID string | |
| Price int64 | |
| Quantity int64 | |
| RemainingQuantity int64 | |
| }{ | |
| OrderID: order.ClientOrderID, | |
| Price: order.Price, | |
| Quantity: order.Quantity, | |
| RemainingQuantity: order.RemainingQuantity, | |
| }, | |
| }, | |
| }, nil | |
| } |
| if err != nil { | ||
| slog.Error("Failed to cancel order") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Copy-paste error in log message.
The error log says "Failed to cancel order" but this is the ModifyOrder handler. This also matches the same issue in CancelOrder at line 75, which lacks details.
Suggested fix
if err != nil {
- slog.Error("Failed to cancel order")
+ slog.Error("Failed to modify order",
+ "Error", err,
+ "orderId", req.OrderId,
+ )
return nil, err
}📝 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.
| if err != nil { | |
| slog.Error("Failed to cancel order") | |
| return nil, err | |
| } | |
| if err != nil { | |
| slog.Error("Failed to modify order", | |
| "Error", err, | |
| "orderId", req.OrderId, | |
| ) | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
In apps/matching-engine/internal/server.go around lines 90-93 (and also fix the
similar incorrect message at line 75), the slog.Error call uses the wrong
message ("Failed to cancel order" in the ModifyOrder handler) and omits error
details; update the log messages to reflect the correct handler ("Failed to
modify order" for ModifyOrder and ensure CancelOrder uses "Failed to cancel
order") and include the error information in the log call (pass the error or
include it as a field) so the logs contain both the correct context and the
actual error details.
| this.logger.info("Order Cancelled successfully", { | ||
| ...response, | ||
| engineStatus: response.status, | ||
| }); |
There was a problem hiding this comment.
Copy-paste error in log message.
The success log says "Order Cancelled successfully" but this is the modifyOrder handler.
Suggested fix
- this.logger.info("Order Cancelled successfully", {
+ this.logger.info("Order Modified successfully", {
...response,
engineStatus: response.status,
});📝 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.
| this.logger.info("Order Cancelled successfully", { | |
| ...response, | |
| engineStatus: response.status, | |
| }); | |
| this.logger.info("Order Modified successfully", { | |
| ...response, | |
| engineStatus: response.status, | |
| }); |
🤖 Prompt for AI Agents
In apps/order-service/src/controllers/order.controller.ts around lines 166 to
169, the log message incorrectly says "Order Cancelled successfully" in the
modifyOrder handler; update the log message to accurately reflect this handler
(e.g., "Order modified successfully" or "Order updated successfully") while
keeping the same structured payload ({ ...response, engineStatus:
response.status }) so logging remains consistent.
| message ModifyOrderRequest { | ||
| string order_id = 1; | ||
| string user_id = 2; | ||
| string symbol = 3; | ||
|
|
||
| optional int64 new_quantity = 4; | ||
| optional int64 new_price = 5; | ||
| } |
There was a problem hiding this comment.
Missing critical idempotency field: client_modify_id
The engine-level proto (packages/proto-defs/proto/engine/order_matching.proto line 67) includes client_modify_id marked as "VERY IMPORTANT" for idempotency, but this API-level proto is missing it. Without this field, clients cannot provide idempotency keys for modify requests, creating risk of duplicate modifications on retries—critical in trading systems.
🔎 Proposed fix to add client_modify_id
message ModifyOrderRequest {
string order_id = 1;
string user_id = 2;
string symbol = 3;
optional int64 new_quantity = 4;
optional int64 new_price = 5;
+
+ // Idempotency key (VERY IMPORTANT)
+ string client_modify_id = 6;
}📝 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.
| message ModifyOrderRequest { | |
| string order_id = 1; | |
| string user_id = 2; | |
| string symbol = 3; | |
| optional int64 new_quantity = 4; | |
| optional int64 new_price = 5; | |
| } | |
| message ModifyOrderRequest { | |
| string order_id = 1; | |
| string user_id = 2; | |
| string symbol = 3; | |
| optional int64 new_quantity = 4; | |
| optional int64 new_price = 5; | |
| // Idempotency key (VERY IMPORTANT) | |
| string client_modify_id = 6; | |
| } |
🤖 Prompt for AI Agents
In packages/proto-defs/proto/api/order_service.proto around lines 58 to 65, the
ModifyOrderRequest is missing the critical idempotency field client_modify_id
required by the engine-level proto; add a new string field named
client_modify_id (use the next available tag, e.g., 6) to the message, mark it
optional to preserve backward compatibility, add a short comment noting "VERY
IMPORTANT — idempotency key" and ensure consumers, server-side validators, and
any mapping to engine/order_matching.proto propagate and validate this field so
clients can supply idempotency keys for modify requests.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.