Capture Alpaca errors in trade_order.notes + add POST /updateOrders endpoint#88
Open
Capture Alpaca errors in trade_order.notes + add POST /updateOrders endpoint#88
Conversation
…ndpoint - When PlaceOrder fails, persist the error message in the trade_order notes column for easier debugging - Add POST /updateOrders endpoint to reconcile pending trade orders without triggering a full rebalance
📝 WalkthroughWalkthroughA new HTTP endpoint Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/api.go`:
- Line 121: The POST /updateOrders route is currently unprotected; add an authz
gate by implementing ApiHandler.requireAuthenticatedUser that checks for the
"userAccountID" value on the gin.Context and returns a 401 JSON error (using
returnErrorJsonCode) if missing, otherwise calls c.Next(); then register the
route using this middleware (e.g., replace engine.POST("/updateOrders",
m.updateOrders) with engine.POST("/updateOrders", m.requireAuthenticatedUser,
m.updateOrders)) so only authenticated callers can invoke m.updateOrders.
In `@api/update_orders.resolver.go`:
- Line 8: The call to m.TradingService.UpdateAllPendingOrders incorrectly passes
the Gin context object c; UpdateAllPendingOrders expects a context.Context —
replace the argument with c.Request.Context() so invoke
m.TradingService.UpdateAllPendingOrders(c.Request.Context()) to pass the correct
context type.
In `@internal/service/trade.service.go`:
- Around line 112-120: The code currently calls TradeOrderRepository.Update(...)
to persist an errMsg but ignores its returned error and overwrites any existing
notes; change this so you (1) read existing notes for insertedOrder.TradeOrderID
(or use an atomic read-modify update), append the new errMsg instead of
replacing Notes, (2) call TradeOrderRepository.Update(...) with the updated
model.TradeOrder{Notes: &newNotes} and the same postgres.ColumnList, and (3)
check the Update error return — if it fails, log the update error (including
context: insertedOrder.TradeOrderID and errMsg) and return or wrap it so the
caller is aware of the DB write failure rather than silently ignoring it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86ddba6e-5fab-49b2-a69b-7fbbda33e8ec
📒 Files selected for processing (3)
api/api.goapi/update_orders.resolver.gointernal/service/trade.service.go
| engine.GET("/publishedStrategies", m.getPublishedStrategies) | ||
|
|
||
| engine.POST("/rebalance", m.rebalance) | ||
| engine.POST("/updateOrders", m.updateOrders) |
There was a problem hiding this comment.
Protect POST /updateOrders with an authz gate.
Line 121 registers a state-mutating endpoint, but current global auth middleware permits anonymous requests (see Line 253–255). This allows unauthenticated callers to trigger order reconciliation.
🔐 Suggested fix
- engine.POST("/updateOrders", m.updateOrders)
+ engine.POST("/updateOrders", m.requireAuthenticatedUser, m.updateOrders)func (m ApiHandler) requireAuthenticatedUser(c *gin.Context) {
if _, ok := c.Get("userAccountID"); !ok {
returnErrorJsonCode(fmt.Errorf("authentication required"), c, 401)
return
}
c.Next()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/api.go` at line 121, The POST /updateOrders route is currently
unprotected; add an authz gate by implementing
ApiHandler.requireAuthenticatedUser that checks for the "userAccountID" value on
the gin.Context and returns a 401 JSON error (using returnErrorJsonCode) if
missing, otherwise calls c.Next(); then register the route using this middleware
(e.g., replace engine.POST("/updateOrders", m.updateOrders) with
engine.POST("/updateOrders", m.requireAuthenticatedUser, m.updateOrders)) so
only authenticated callers can invoke m.updateOrders.
| ) | ||
|
|
||
| func (m ApiHandler) updateOrders(c *gin.Context) { | ||
| err := m.TradingService.UpdateAllPendingOrders(c) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TradeService signature =="
rg -n -C2 'type TradeService interface|UpdateAllPendingOrders\s*\(' internal/service/trade.service.go
echo
echo "== Handler call site =="
rg -n -C2 'func \(m ApiHandler\) updateOrders|UpdateAllPendingOrders\s*\(' api/update_orders.resolver.goRepository: sahilsk11/factorbacktest
Length of output: 863
Pass c.Request.Context() to UpdateAllPendingOrders, not the Gin context directly.
Line 8 passes c (*gin.Context), but TradeService.UpdateAllPendingOrders expects context.Context. Change to c.Request.Context().
Suggested fix
- err := m.TradingService.UpdateAllPendingOrders(c)
+ err := m.TradingService.UpdateAllPendingOrders(c.Request.Context())📝 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
| err := m.TradingService.UpdateAllPendingOrders(c) | |
| err := m.TradingService.UpdateAllPendingOrders(c.Request.Context()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/update_orders.resolver.go` at line 8, The call to
m.TradingService.UpdateAllPendingOrders incorrectly passes the Gin context
object c; UpdateAllPendingOrders expects a context.Context — replace the
argument with c.Request.Context() so invoke
m.TradingService.UpdateAllPendingOrders(c.Request.Context()) to pass the correct
context type.
Comment on lines
+112
to
+120
| // persist the error message in the trade order notes for debugging | ||
| errMsg := fmt.Sprintf("order for trade request %s %s %s failed: %s", alpacaSide, symbol, quantity.String(), err.Error()) | ||
| h.TradeOrderRepository.Update(nil, | ||
| insertedOrder.TradeOrderID, | ||
| model.TradeOrder{ | ||
| Notes: &errMsg, | ||
| }, postgres.ColumnList{ | ||
| table.TradeOrder.Notes, | ||
| }) |
There was a problem hiding this comment.
Do not silently ignore failures when persisting trade_order.notes.
Lines 114–120 ignore the Update(...) error. If this DB write fails, the endpoint returns only the Alpaca failure and you lose the note-capture guarantee this PR introduces. Also, Line 117 overwrites existing notes.
🛠️ Suggested fix
if err != nil {
// persist the error message in the trade order notes for debugging
- errMsg := fmt.Sprintf("order for trade request %s %s %s failed: %s", alpacaSide, symbol, quantity.String(), err.Error())
- h.TradeOrderRepository.Update(nil,
+ prefix := ""
+ if insertedOrder.Notes != nil && *insertedOrder.Notes != "" {
+ prefix = *insertedOrder.Notes + " | "
+ }
+ errMsg := fmt.Sprintf("%sorder for trade request %s %s %s failed: %s", prefix, alpacaSide, symbol, quantity.String(), err.Error())
+ _, noteErr := h.TradeOrderRepository.Update(nil,
insertedOrder.TradeOrderID,
model.TradeOrder{
Notes: &errMsg,
}, postgres.ColumnList{
table.TradeOrder.Notes,
})
- return nil, fmt.Errorf("failed to execute order for trade order %s: %w", insertedOrder.TradeOrderID, err)
+ if noteErr != nil {
+ return nil, fmt.Errorf("failed to execute order for trade order %s: %w (also failed to persist notes: %v)", insertedOrder.TradeOrderID, err, noteErr)
+ }
+ return nil, fmt.Errorf("failed to execute order for trade order %s: %w", insertedOrder.TradeOrderID, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/trade.service.go` around lines 112 - 120, The code currently
calls TradeOrderRepository.Update(...) to persist an errMsg but ignores its
returned error and overwrites any existing notes; change this so you (1) read
existing notes for insertedOrder.TradeOrderID (or use an atomic read-modify
update), append the new errMsg instead of replacing Notes, (2) call
TradeOrderRepository.Update(...) with the updated model.TradeOrder{Notes:
&newNotes} and the same postgres.ColumnList, and (3) check the Update error
return — if it fails, log the update error (including context:
insertedOrder.TradeOrderID and errMsg) and return or wrap it so the caller is
aware of the DB write failure rather than silently ignoring it.
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
1 participant
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.
Changes
1. Capture Alpaca error in trade_order.notes
When
PlaceOrderfails inplaceOrder(), the error message is now persisted in the trade_order'snotescolumn before returning the error. Previously, the error message was only propagated through the call stack and ended up in Lambda logs (which get truncated). Now it's queryable directly from the DB:2. Add POST /updateOrders endpoint
Exposes
TradingService.UpdateAllPendingOrders()as an HTTP endpoint. This lets you reconcile pending trade order fills from Alpaca without triggering a full rebalance cycle. Previously this was only available as a CLI command.Invoke via Lambda:
{ "httpMethod": "POST", "path": "/updateOrders", "pathParameters": {"proxy": "updateOrders"} }Summary by CodeRabbit
New Features
Bug Fixes