fix: paper trade data pipeline — persist fills, positions, and wire dashboard

[ajitpratap0]

Summary

• Root cause fixed: handlePaperTrade never called InsertTrade or CreatePosition after filling an order — the entire data pipeline downstream of the order was missing
• New REST endpoints: GET /api/v1/trades (fill records), GET /api/v1/risk/metrics|circuit-breakers|exposure, GET /api/v1/performance/pairs
• Dashboard wired to real data: useTrades, useRiskMetrics, useCircuitBreakers, useRiskExposure, usePairPerformance all call the real API; risk page replaces hardcoded mock constants with live data

Changes

Backend (Go)

• cmd/api/handlers_trading.go — handlePaperTrade now resolves/creates a PAPER session, calls GetMarketPrice + inline slippage, inserts trade fill via InsertTrade, creates/updates position via CreatePosition/UpdatePositionAveraging
• internal/db/trades.go — add ListAllTrades(ctx, limit, offset) helper
• internal/api/trades.go — TradesHandler serving GET /api/v1/trades
• internal/api/risk.go — RiskHandler serving VaR metrics, circuit breakers, exposure
• internal/api/performance.go — PerformanceHandler serving per-symbol realized PnL
• cmd/api/routes.go — register all new handlers

Frontend (Next.js)

• lib/api.ts — add getTrades, getRiskMetrics, getCircuitBreakers, getRiskExposure, getPairPerformance to ApiClient
• hooks/useTradeData.ts — useTrades hits real API, normalizes to Trade[], respects USE_MOCK_DATA; equity curve no longer hardcoded
• hooks/usePerformance.ts — fix raw fetch calls missing /api/v1 prefix; switch to apiClient; add useRiskExposure
• app/risk/page.tsx — remove all hardcoded mock constants; wire to real hooks

Test Plan

• go test -tags=integration -run TestPaperTrade_PersistsAllRows ./cmd/api/... — passes (order FILLED, trade fill in DB, open position created)
• npm test in web/dashboard — 14/14 pass including 4 new useTrades hook tests
• go build ./... — clean
• npm run type-check — clean

🤖 Generated with Claude Code

[claude]

PR Review: Paper Trade Data Pipeline (#89)

Good work fixing a significant gap — the missing InsertTrade/CreatePosition call was a real data loss bug. The new endpoints are clean and the test coverage is solid. Below are issues to address before merging, ordered by severity.

---

🔴 Critical

1. No DB transaction wrapping the fill pipeline (handlers_trading.go:506-570)

The sequence InsertOrder → UpdateOrderStatus → InsertTrade → CreatePosition → AggregateSessionStats is not atomic. If InsertTrade or CreatePosition fails after the order is already marked FILLED, the DB ends up in a partially-committed state — an order shows FILLED but no trade fill row or position exists. This is exactly what the PR is trying to fix, but silently re-introduces it for the error path.

// Wrap the entire fill pipeline in a transaction
tx, err := s.db.BeginTx(ctx)
if err != nil { ... }
defer tx.Rollback(ctx)
// ... all fill operations on tx ...
tx.Commit(ctx)

2. TOCTOU race condition on paper session creation (handlers_trading.go:403-425)

ListActiveSessions + loop + CreateSession is not atomic. Two concurrent paper trade requests racing here will both find no session and both call CreateSession, yielding duplicate PAPER sessions. Use INSERT ... ON CONFLICT DO NOTHING RETURNING id or a DB-level unique constraint on (mode=PAPER) + application-level retry.

3. N+1 query pattern in risk.go and performance.go

collectClosedReturns and aggregatePairPerformance both iterate all sessions and issue a GetPositionsBySession query per session. With N active sessions this fires N+1 queries. Replace with a single SQL join:

SELECT p.symbol, p.realized_pnl
FROM positions p
JOIN trading_sessions s ON s.id = p.session_id
WHERE s.status = 'active' AND p.exit_time IS NOT NULL AND p.realized_pnl IS NOT NULL

---

🟡 Medium

4. Position side doesn't handle closing an existing long (handlers_trading.go:536-567)

A SELL on BTCUSDT when the user already holds a long creates a new short position instead of reducing/closing the long. The UpdatePositionAveraging path only averages into the same-direction position. Need to check existingPos.Side vs orderSide and handle the close/reduce case:

if existingPos != nil && existingPos.Side != posSide {
    // closing or reducing — call ClosePosition or ReducePosition instead
}

5. Exposure uses entry price, not mark price (risk.go:60)

totalExposure += p.Quantity * p.EntryPrice

This misrepresents current exposure for positions with significant unrealised PnL. Either document this as "notional at entry" or use UnrealizedPnL to derive a mark price. At minimum the API response should clarify this is entry-price notional.

6. Circuit breaker thresholds are hardcoded (risk.go:122-126)

buildBreaker("Max Daily Loss", lossAmount, 5000)   // $5000 hardcoded
buildBreaker("Max Drawdown %", maxDrawdown*100, 10) // 10% hardcoded
buildBreaker("Total Trade Count", float64(totalTrades), 100) // 100 hardcoded

These should come from the risk config (configs/config.yaml) or at minimum be named constants. The "Total Trade Count" breaker in particular doesn't map to any standard risk control concept — Max Order Rate would be more meaningful.

7. Pagination response missing total count (trades.go:53-57)

c.JSON(http.StatusOK, gin.H{
    "trades": trades,
    "count":  len(trades),  // This is page size, not total
    ...
})

The frontend can't determine whether to show a "Load more" control without knowing the total. Add SELECT COUNT(*) FROM trades or return a has_more boolean.

---

🟢 Low / Nits

8. ptrStr/ptrF64 helpers in handlers_trading.go

These are generic enough to belong in an internal/util or shared helpers file. Their current placement in the handler file makes them hard to discover.

9. equity_curve will always be empty (useTradeData.ts:279)

equity: (raw as { equity_curve?: EquityPoint[] }).equity_curve ?? []

The dashboard PnL endpoint doesn't return equity_curve, so the equity chart will always be blank. Either wire up a real endpoint here or keep using getMockEquityPoints() until one exists — silently returning [] is worse than showing mock data.

10. Exposure side hardcoded to 'long' (risk/page.tsx:82)

side: 'long' as const,

The API doesn't include position side in the exposure endpoint, so this is always long. Either add side to the GET /risk/exposure response or remove the side badge from the UI to avoid misleading output.

11. CalculateVaR interface type casting (risk.go:74-87)

returnsIface := make([]interface{}, len(returns))

The []interface{} conversion is a workaround for a weakly-typed internal API. If risk.Service.CalculateVaR is under your control, consider changing its signature to accept []float64 directly.

---

✅ Positives

• The integration test (TestPaperTrade_PersistsAllRows) is exactly right — it covers order, fill, and position in one shot.
• Removing mock fallbacks from hooks in favour of throwing errors is the right call; silent failures were masking API issues.
• Input validation for limit orders (isLimit && req.Price <= 0) is a clean improvement.
• trades == nil guard before returning empty slice prevents a null in the JSON response.
• Route registration is clean and consistent with the existing pattern in routes.go.

[claude]

PR Review — fix: paper trade data pipeline

This is a solid fix for a critical data pipeline gap. The root cause is correctly identified and the approach is sensible. Below are issues ranging from bugs to minor nits.

---

🐛 Bugs

1. VaR units mismatch: dollar values rendered as percentages

collectClosedReturns feeds RealizedPnL dollar amounts (e.g. -234.56) into CalculateVaR. The frontend then calls formatPercentage(var95), which would display -23456% or similar garbage. The old mock used fractional values like 0.023 (2.3%). Either normalize returns to percentages server-side (divide by position notional), or change the frontend to formatCurrency(var95).

// internal/api/risk.go — returns are dollar PnL, not pct returns
returns = append(returns, *p.RealizedPnL)  // e.g. -234.56

// app/risk/page.tsx — formats as pct
value={var95 != null ? formatPercentage(var95) : '—'}

2. All exposure positions hardcoded as long

In app/risk/page.tsx:

side: 'long' as const,  // ← always long, ignores actual position side

The API doesn't return a side field from GetExposure, so short positions will render with a green bar instead of red. Either drop the side display entirely (it was removed from the breakdown bars but the bar color bg-profit is still always green) or add side to the exposure API response.

3. Session symbol constraint mismatch

When creating a new paper session, Symbol is set to req.Symbol. But the next call for a different symbol (e.g. ETHUSDT after a BTCUSDT trade) reuses the same session. If trading_sessions.symbol has a NOT NULL or unique constraint per-session expectation, multi-symbol paper trading may cause DB-level issues or confusing session state. The session should either be symbol-agnostic (empty symbol) or multi-symbol trading needs a different session-lookup strategy.

---

⚠️ Logic Issues

4. TOCTOU retry masks all create failures

if err := s.db.CreateSession(ctx, newSession); err != nil {
    log.Warn().Err(err).Msg("...")
    sessions2, err2 := s.db.ListActiveSessions(ctx)
    ...
    if sessionID == nil {
        log.Error().Err(err).Msg("Failed to create or find paper session")

The retry correctly handles concurrent inserts but also silently swallows transient DB errors (connection timeouts, constraint violations for unrelated reasons). The original error from CreateSession is dropped. Consider checking the error type — only retry on unique-constraint violations (pgerrcode.UniqueViolation), surface others immediately.

5. Hardcoded commission rate (0.1%)

commission := execQuoteQty * 0.001

TASKS.md/January 2026 review flags hardcoded values as an immediate blocker. This should come from config (e.g. s.config.Paper.CommissionRate).

6. GetAllClosedPositions and collectClosedReturns have no LIMIT

Both queries fetch all closed positions in one shot. In production with thousands of trades this will be slow. Add pagination or at minimum a reasonable cap (e.g. LIMIT 1000 ORDER BY exit_time DESC) for VaR calculations, which only need recent history anyway.

7. riskScore defaults to 100 when circuit breakers unavailable

if (!circuitBreakers.length) return 100  // "Low Risk"

When the API is down or returns empty, the UI shows "Low Risk" with score 100. This is the opposite of a safe default — a degraded state should show "Unknown" or the refresh button should signal the error.

8. useDashboardPnl equity curve silently goes empty

equity: (raw as { equity_curve?: EquityPoint[] }).equity_curve ?? [],

The API doesn't return equity_curve (there's no such field in any backend handler). This replaces the previous mock equity curve with an always-empty array, breaking the equity chart. Revert to mock equity until the backend endpoint actually provides this.

---

🔒 Security

9. New endpoints lack authentication

/api/v1/trades, /api/v1/risk/*, /api/v1/performance/pairs are registered with only readMiddleware (rate limiting), no AuthMiddleware. Other sensitive routes like decision analytics use AuthMiddleware. Exposure/trade history is sensitive data. Apply AuthMiddleware consistently or document why these are intentionally public.

---

🔧 Code Quality

10. ptrStr/ptrF64 likely duplicated

These small helpers are defined at the top of handlers_trading.go. Check whether internal/db or another package already has pointer helpers to avoid duplication across the codebase.

11. ListAllTrades missing error context wrapping

if err := rows.Scan(...); err != nil {
    return nil, err  // no context added
}

Inconsistent with the project convention of fmt.Errorf("failed to ...: %w", err). Matches the pattern in GetAllClosedPositions (which does wrap).

12. CalculateVaR error silently ignored

res95, err := h.riskService.CalculateVaR(...)
if err == nil {
    ...  // error path has no log
}

If VaR fails, the caller gets null with no indication of why. At least log.Debug().Err(err).Msg("VaR calculation failed") would help in production debugging.

13. CountAllTrades sequential full-table count

SELECT COUNT(*) FROM trades runs on every GET /api/v1/trades call. For large tables this is expensive and slow. Consider caching it or using an approximate count (pg_class.reltuples) if exactness isn't required for pagination.

---

✅ What's Good

• The TOCTOU comment and acknowledgment of the position close/reduce limitation are excellent — transparent about what's not done yet.
• Integration test TestPaperTrade_PersistsAllRows properly tests all three DB rows (order, trade fill, position).
• useTrades correctly checks USE_MOCK_DATA flag.
• Removing the hardcoded correlation matrix and mock constants from the risk page is the right call.
• buildBreaker helper is clean and reusable.
• The 14 frontend tests are well-structured with proper mock isolation.

---

Summary

Severity	Count
Bug	3 (VaR units, session symbol, equity curve empty)
Logic Issue	5
Security	1
Code Quality	4

The VaR units mismatch (#1) and missing auth (#9) should be fixed before merge. The rest can be addressed in follow-up issues.

🤖 Generated with Claude Code

[claude]

PR Review: fix: paper trade data pipeline — persist fills, positions, and wire dashboard

This is a solid, well-scoped fix that addresses a real data pipeline gap. The root cause identification is accurate, the TODO comments are appropriately honest about known limitations, and the frontend wiring is clean. Below are issues ranging from correctness bugs to minor nits.

---

🔴 Bugs / Correctness Issues

1. No database transaction wrapping the paper trade pipeline
handlePaperTrade now executes 4–5 sequential writes (InsertOrder → UpdateOrderStatus → InsertTrade → CreatePosition/UpdatePositionAveraging → AggregateSessionStats) with no transaction. If any step fails after InsertOrder, you get an orphaned filled order with no trade fill, or a trade fill with no position. This is exactly the kind of partial state that causes silent bugs in downstream analytics.

// handlers_trading.go ~line 440
if err := s.db.InsertOrder(ctx, order); err != nil { ... }
// ~100 lines later, if InsertTrade fails:
log.Warn().Err(err).Msg("Failed to insert paper trade fill row") // silently continues

Recommendation: wrap the fill block in a db.BeginTx / tx.Commit so all-or-nothing semantics are enforced.

2. Position averaging into opposite direction instead of closing
The opposite-side case falls through to the else (averaging) branch after logging a warning:

// handlers_trading.go ~line 543
if existingPos != nil && existingPos.Side != posSide {
    log.Warn()... // warns, then falls through
}
// ...
if existingPos == nil {
    // create new position
} else {
    // THIS branch runs for opposite-side trades too
    totalQty := existingPos.Quantity + req.Quantity   // adds to a long when selling!
    weightedAvg := (existingPos.Quantity*existingPos.EntryPrice + req.Quantity*execPrice) / totalQty
    s.db.UpdatePositionAveraging(...)
}

The TODO is acknowledged, but silently corrupting position data is worse than returning a 400. Consider returning http.StatusUnprocessableEntity for this case until the close logic is implemented.

3. Exposure calculation uses entry price, not current price
GetMetrics and GetExposure both compute p.Quantity * p.EntryPrice for open positions. For a position opened at $40k now trading at $50k, exposure is understated by 20%. Since current price isn't stored on the position, this requires either a live price lookup or a note in the response that it's cost-basis exposure, not mark-to-market.

// internal/api/risk.go ~line 56
for _, p := range openPositions {
    totalExposure += p.Quantity * p.EntryPrice // cost basis, not market value
}

---

🟡 Design / Performance Issues

4. Go-side aggregation instead of SQL for performance/risk endpoints
aggregatePairPerformance fetches all closed positions into memory and groups by symbol in Go. GetExposure does the same for open positions. For any non-trivial dataset these are full table scans. Replace with SQL GROUP BY:

-- performance/pairs
SELECT symbol, SUM(realized_pnl) AS realized_pnl, COUNT(*) AS trade_count
FROM positions WHERE exit_time IS NOT NULL AND realized_pnl IS NOT NULL
GROUP BY symbol ORDER BY realized_pnl DESC

-- risk/exposure
SELECT symbol, SUM(quantity * entry_price) AS exposure
FROM positions WHERE exit_time IS NULL
GROUP BY symbol

5. Hardcoded circuit breaker thresholds
buildBreaker uses magic numbers ($5000 loss, 10% drawdown, 100 trades) that don't come from configs/config.yaml or the risk config. For a system with configurable risk parameters this is surprising:

// internal/api/risk.go ~line 117
breakers := []gin.H{
    buildBreaker("Max Daily Loss", lossAmount, 5000),   // hardcoded
    buildBreaker("Max Drawdown %", maxDrawdown*100, 10), // hardcoded
    buildBreaker("Total Trade Count", float64(totalTrades), 100), // hardcoded
}

At minimum these should be constants with a comment pointing to the risk config. Ideally read from internal/risk service config.

6. New MockExchange instance per request

// handlers_trading.go ~line 441
mockEx := exchange.NewMockExchange(s.db)
refPrice = mockEx.GetMarketPrice(req.Symbol)

APIServer should hold a reference to the exchange (already likely injected elsewhere). Creating a new instance per request is wasteful and may miss any state the exchange holds (e.g. pre-seeded prices in tests).

---

🟡 Frontend Issues

7. Silent API failure in risk page looks like "no risk"
When all three API calls fail, circuitBreakers is [] and riskScore computes to 100 ("Low Risk") due to this guard:

// app/risk/page.tsx ~line 69
const riskScore = useMemo(() => {
    if (!circuitBreakers.length) return 100  // failure looks like "safe"
    ...
}, [circuitBreakers])

The useRiskMetrics / useCircuitBreakers / useRiskExposure hooks expose isError and isLoading — the page should surface an error/loading state rather than rendering a misleadingly green dashboard.

8. Exposure side always hardcoded to 'long'

// app/risk/page.tsx ~line 82
return raw.exposure.map(e => ({
    ...
    side: 'long' as const,   // always long, even for short positions
}))

The backend GET /risk/exposure doesn't return position side. Either add it to the endpoint, or remove the side field from the mapped object (the position sizing breakdown no longer renders it anyway after this PR).

9. Mock data ignores pagination in useTrades

// hooks/useTradeData.ts
if (USE_MOCK_DATA) {
    return { success: true, data: getMockTrades(), ... }  // ignores limit/offset
}

Fine for dev, but worth a comment so future readers don't wonder why pagination params have no effect in mock mode.

---

🟢 Minor / Nits

10. total: -1 sentinel in ListTrades response

// internal/api/trades.go ~line 57
total, err := h.db.CountAllTrades(ctx)
if err != nil {
    total = -1  // surprising sentinel for callers
}

Prefer omitting the field on error or using 0 with a logged warning. -1 will surprise any client doing if total > 0.

11. Paper session Symbol field is single-symbol

// handlers_trading.go ~line 417
newSession := &db.TradingSession{
    Symbol: req.Symbol,  // first trade's symbol becomes the session symbol
    ...
}

If the session model is intended to be multi-asset, setting Symbol to the first traded pair is misleading. Consider leaving it empty or using a sentinel like "PAPER".

12. collectClosedReturns nil check is redundant
GetAllClosedPositions already filters realized_pnl IS NOT NULL in SQL, so the nil check in the loop is dead code:

if p.RealizedPnL != nil {  // always true given the SQL filter
    returns = append(returns, *p.RealizedPnL)
}

Remove the guard or add a comment explaining the defense-in-depth intent.

---

✅ What's done well

• The TOCTOU handling for concurrent session creation (retry lookup) is a nice touch and correctly handled.
• The slippage model (* 1.001 / * 0.999) is consistent with the existing mock exchange behavior.
• ptrStr/ptrF64 helpers are a clean, Go-idiomatic solution for pointer literals.
• The integration test (TestPaperTrade_PersistsAllRows) correctly validates the three key DB rows (order, trade fill, position) — exactly what was broken before.
• Removal of the catch { return mockData } fallback pattern in hooks is the right call; silent fallback to stale hardcoded data was masking real connectivity issues.
• CountAllTrades being non-fatal is sensible API design.

The transaction issue (#1) is the highest priority to fix before merge — the rest are improvements that can be addressed in follow-up PRs given the January 2026 roadmap pressure.

[claude]

PR Review: Paper Trade Data Pipeline

This is a solid and well-motivated fix. The root cause (missing InsertTrade/CreatePosition calls) is correctly identified and the transaction wrapping the fill writes is the right approach. Below are findings ranging from critical to minor.

---

Critical / Correctness

1. Silent error suppression on unrealized_pnl UPDATE (handlers_trading.go)

_, _ = tx.Exec(ctx,
    `UPDATE positions SET unrealized_pnl = $1 WHERE id = $2`,
    unrealizedPnL, posID,
)

Errors here are completely swallowed. If the UPDATE fails the committed position row has a stale or null unrealized_pnl. More importantly, the value being set is 0.0 — this can just be added to the INSERT column list (unrealized_pnl = $15) and the entire second statement can be deleted. It was explicitly excluded from the INSERT for no apparent reason.

---

2. TOCTOU race on position lookup (handlers_trading.go)

// Check for an opposite-side position BEFORE opening a transaction
existingPos, posErr := s.db.GetPositionBySymbolAndSession(ctx, req.Symbol, *sessionID)
...
// Then the position ID is used inside the transaction
_, txErr = tx.Exec(ctx, `UPDATE positions SET ... WHERE id = $1 AND exit_time IS NULL`, existingPos.ID, ...)

The lookup happens before BeginTx. Two concurrent requests for the same symbol can both see existingPos == nil, both succeed the CREATE branch, and create duplicate open positions for the same symbol/session. This can be solved by moving the lookup inside the transaction and using SELECT ... FOR UPDATE to serialize concurrent access:

SELECT ... FROM positions WHERE symbol = $1 AND session_id = $2 AND exit_time IS NULL FOR UPDATE

---

3. CountAllTrades returns a stale estimate, but total implies an exact count (internal/api/trades.go, internal/db/trades.go)

// pg_class.reltuples is an estimate updated by ANALYZE/autovacuum
SELECT reltuples::bigint AS estimate FROM pg_class WHERE relname = 'trades'

The response field is named total, which is conventionally an exact count. For a freshly-created table before the first autovacuum, this returns 0 even with thousands of rows. This misleads pagination clients. Either rename to estimated_total in the JSON response, or add a "total_is_estimate": true flag. The comment in the DB function is good but the API contract hides it.

---

Design / Architecture

4. Paper session lookup on every trade request (handlers_trading.go)

ListActiveSessions(ctx) is called on every call to handlePaperTrade to find the paper session. This is a full table scan (or at minimum a filtered index scan) on a hot path. Consider caching the paper session ID in the APIServer struct (protected by the existing sessionMu) after the first creation.

5. GetExposureBySymbol / GetPairPerformance do not scope to session type (internal/db/positions.go)

Both queries aggregate across all sessions (paper and live). When live trading is added, exposure and pair performance data will be incorrectly mixed. A WHERE session_id IN (SELECT id FROM trading_sessions WHERE mode = $1) scope, or a JOIN on trading_sessions, should be added. At minimum, add a TODO comment so this is not forgotten.

6. buildBreaker edge case: threshold = 0 (internal/api/risk.go)

if current >= threshold {  // 0 >= 0 → TRIGGERED for any current value
    status = "TRIGGERED"
} else if threshold > 0 && current/threshold >= 0.8 {
    ...
}

If MaxTradeCount or another config value is accidentally set to 0, every status check immediately shows TRIGGERED — including at 0 trades. The first branch should guard: if threshold > 0 && current >= threshold.

---

Code Quality

7. Inlined SQL in transaction vs. DB layer methods (handlers_trading.go)

The transaction executes raw SQL for UPDATE orders, INSERT INTO trades, and INSERT INTO positions directly in the handler, bypassing the internal/db abstractions (InsertTrade, UpdateOrderStatus, CreatePosition). This has two downsides: the handler must stay in sync with the schema manually, and the DB layer methods cannot be reused elsewhere in a transaction context.

The standard Go pattern for this is to accept pgx.Tx (or a shared DBTX interface) in the DB-layer methods so callers can pass either a pool connection or a transaction:

func (db *DB) InsertTrade(ctx context.Context, tx pgx.Tx, trade *Trade) error { ... }

This is a bigger refactor, but the current approach of duplicating SQL in the handler is a maintenance burden.

8. Hardcoded commissionAsset = "USDT" (handlers_trading.go)

commissionAsset := "USDT"

This is incorrect for BTC-quoted pairs (e.g., ETH/BTC). The asset should be derived from the quote currency of the symbol. This is acceptable as a short-term simplification but should be a tracked TODO, not a silent wrong assumption.

---

Testing

9. Integration test only covers happy-path market BUY (paper_trade_pipeline_test.go)

The test is a good start, but the following branches are completely untested:

• Limit order persistence
• SELL order → PositionSideShort is set correctly
• Averaging into an existing position (the UpdatePositionAveraging branch)
• Opposite-side rejection (StatusUnprocessableEntity)
• Transaction rollback on mid-flight DB failure

Given the previous data loss bug being in this exact code path, higher test coverage is warranted.

10. No test for the new REST endpoints

GET /api/v1/trades, GET /api/v1/risk/metrics, GET /api/v1/risk/circuit-breakers, GET /api/v1/risk/exposure, and GET /api/v1/performance/pairs have no handler-level tests. Even simple response-shape tests would catch regressions.

---

Minor / Nits

• ptrF64 comment: The comment // Not duplicated elsewhere in the codebase (verified via grep) is unusual in production code. Either move this to a shared utility or just drop the comment.
• mockDrawdown in risk/page.tsx: Still hardcoded with Math.random(), meaning the chart re-renders with different data on every refresh. This is pre-existing but the random data makes the page look unstable. A static seed or empty state would be better.
• useRiskExposure query key: [...PERFORMANCE_QUERY_KEYS.risk, 'exposure'] will share the first element with useRiskMetrics. If you call queryClient.invalidateQueries({ queryKey: PERFORMANCE_QUERY_KEYS.risk }) it will invalidate both, which is fine, but worth verifying this is intentional.

---

Summary

Priority	Count
Critical / correctness	3
Design / architecture	3
Code quality	2
Testing gaps	2
Minor	3

The core fix is correct and the transaction wrapping is exactly right. The TOCTOU race on position lookup and the silent unrealized_pnl error swallowing are the most important items to address before merging.

[claude]

PR Review: fix: paper trade data pipeline — persist fills, positions, and wire dashboard

This is a solid fix for a critical data pipeline gap. The transaction wrapping, TOCTOU race handling on session creation, and the sentinel errOppositeSide pattern are all well-thought-out. Below is a mix of bugs, risks, and minor suggestions.

Bugs

1. Race condition in position creation — SELECT FOR UPDATE missing

GetOpenPositionBySymbolTx uses a plain SELECT with default READ COMMITTED isolation. Two concurrent market orders for the same symbol can both observe existingPos == nil, both proceed past the guard, and both INSERT a new position row — defeating the stated goal of eliminating the TOCTOU race.

The comment correctly describes the intent but the fix is incomplete. Fix: add FOR UPDATE to the query in GetOpenPositionBySymbolTx:

SELECT ... FROM positions
WHERE symbol =  AND session_id =  AND exit_time IS NULL
ORDER BY entry_time DESC
LIMIT 1
FOR UPDATE

2. Order row left in NEW status if transaction fails

The order is inserted before WithTx begins. If the DB transaction subsequently fails (connection drop, constraint violation, etc.), the order row remains in NEW status with no corresponding fill or position — creating orphan orders that silently inflate OrderStatusNew counts.

Consider either inserting the order inside the same WithTx call, or explicitly marking the order as CANCELED/ERROR in the txErr != nil error path.

3. Frontend Trade type mismatch in useTrades

The hook casts the raw API array directly to Trade[]:

const tradeList: Trade[] =
  raw && typeof raw === 'object' && 'trades' in raw
    ? (raw as { trades: Trade[] }).trades
    : []

The backend serialises the Go Trade struct with snake_case JSON tags (price, executed_at, is_maker, etc.), but the frontend Trade type uses camelCase (entryPrice, currentPrice, pnl, etc.). This is a TypeScript-only assertion — no field mapping occurs at runtime. Code consuming tradeList[n].entryPrice will get undefined at runtime. Compare: usePositions explicitly maps fields. useTrades needs the same treatment.

4. DISABLED circuit breaker status silently mapped to normal

buildBreaker in internal/api/risk.go returns status: "DISABLED" when threshold <= 0. In risk/page.tsx, the mapping falls through to 'normal':

status: cb.status === 'TRIGGERED' ? 'triggered' : cb.status === 'WARNING' ? 'warning' : 'normal',

A disabled breaker renders as green/OK rather than inactive, which could mislead operators into thinking risk controls are active when they are unconfigured.

Medium Concerns

5. CountAllTrades not schema-scoped

"SELECT reltuples::bigint AS estimate FROM pg_class WHERE relname = 'trades'"

pg_class.relname is not schema-unique. A second schema with a trades table returns a wrong count. Fix:

SELECT reltuples::bigint FROM pg_class
WHERE relname = 'trades'
  AND relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'public')

6. Hardcoded 0.1% slippage vs configurable commission rate

CommissionRate is now configurable (good), but market-order slippage is hardcoded at 1.001/0.999. Both values should come from config or at least be named constants.

7. GetAllClosedPositions LIMIT 1000 silently truncates VaR sample

If the system runs more than 1000 paper trades (easy in automated testing), VaR is computed on a truncated sample with no warning to the caller. Consider emitting a log.Warn when the limit is hit.

Minor / Style

8. pgx.TxOptions{} defaults to READ COMMITTED — document intent or upgrade

The main caller relies on isolation for position deduplication. Consider documenting the isolation level explicitly or using pgx.TxOptions{IsoLevel: pgx.RepeatableRead} alongside the FOR UPDATE fix above.

9. ptrF64 helper — consider moving to internal/db

The "verified via grep" comment will go stale. A one-liner in db/utils.go is safer long-term.

10. TradingSession.Symbol = "PAPER" magic value

Symbol on the session model now has two semantically different meanings (real trading symbol vs mode tag). Worth a TODO to add a session_type or is_multi_asset column in a follow-up migration.

What's good

• Transaction wrapping in WithTx with panic-safe rollback is clean and reusable
• TOCTOU retry on 23505 for concurrent session creation is correct
• errOppositeSide sentinel + 422 response prevents silent data corruption
• buildBreaker zero-threshold guard (DISABLED status) avoids false positives on unconfigured values
• CountAllTrades using pg_class.reltuples instead of COUNT(*) is a good performance choice
• Integration test TestPaperTrade_PersistsAllRows directly validates the three previously-missing rows (order fill, trade, position)
• Frontend runtime type guards are defensive without being noisy
• Removing hardcoded mock constants and riskScore = null on empty data vs the old "always 100" is a real correctness improvement

---

Critical before merge: Items 1, 2, and 3 (race condition, orphan orders, Trade type mismatch). Items 4-7 can be filed as follow-up issues.

[claude]

Code Review — PR #89: Paper trade data pipeline fix

Great direction overall — fixing a silent data loss bug and wiring the dashboard to real data in one PR is valuable. The transaction design and TOCTOU handling show careful thinking. Below are issues from critical to low.

---

Critical

1. FOR UPDATE with LIMIT 1 does not prevent phantom inserts (internal/db/positions.go:GetOpenPositionBySymbolTx)

The comment says "RepeatableRead + FOR UPDATE on positions prevents concurrent orders from both observing existingPos == nil and each inserting a duplicate position." This is not accurate. FOR UPDATE locks the rows returned — when no row exists, no lock is acquired and the phantom problem remains. Two concurrent transactions that both see existingPos == nil can both proceed to INSERT a new position row.

The correct fix is a partial unique index:

CREATE UNIQUE INDEX positions_open_symbol_session_uniq
  ON positions (session_id, symbol)
  WHERE exit_time IS NULL;

With that index, one of the two concurrent INSERTs will hit a unique constraint violation (23505), which can be caught and retried. FOR UPDATE only helps when a row already exists (preventing concurrent updates to the same row), not for the no-row case.

---

2. Order row inserted outside the transaction (cmd/api/handlers_trading.go ~line 460)

s.db.InsertOrder is called before s.db.WithTx. If the process crashes or the transaction is rolled back, the order sits in NEW status forever. The cancel-on-failure cleanup code (UpdateOrderStatus → CANCELED) mitigates this at runtime but not on server crash.

Better approach: move InsertOrder inside the WithTx callback so the order row is only committed if the entire fill succeeds atomically. The order struct can be populated before the transaction and the insert placed as the first operation inside fn.

---

3. Raw SQL embedded in handler bypasses the DB layer (cmd/api/handlers_trading.go)

The trade INSERT and position INSERT/UPDATE are raw tx.Exec calls embedded directly in handlers_trading.go. The project already has db.InsertTrade, db.CreatePosition, and db.UpdatePositionAveraging. These should be converted to transactional variants (accepting pgx.Tx) like the pattern already established with UpdateOrderStatusTx and GetOpenPositionBySymbolTx. Keeping SQL in the handler layer:

• Creates divergence — schema changes must be made in two places
• Makes the handler nearly 300 lines and harder to test in isolation
• Breaks the existing abstraction contract

---

Medium

4. GetAllClosedPositions unbounded on every /risk/metrics request (internal/db/positions.go:354)

GetAllClosedPositions is called on every GET /api/v1/risk/metrics request and fetches up to 1000 rows to compute VaR. This will be expensive at scale. Consider:

• Caching the VaR calculation (e.g., Redis TTL of 30s)
• Pre-computing on a background goroutine rather than on each request
• Or at minimum, adding a simple in-memory cache with a short TTL

5. GetPairPerformance has no LIMIT (internal/db/positions.go:GetPairPerformance)

No limit on the GROUP BY query — at scale this could return thousands of symbols. Add LIMIT 100 or a configurable cap.

6. Integration test does not cover the GetMarketPrice code path (cmd/api/paper_trade_pipeline_test.go)

The test body includes "price":45000, which means the handler uses req.Price directly and never calls s.exchange.GetMarketPrice. Consider adding a second test case that omits the price field for market orders to verify the fallback path works.

7. Circuit breaker label vs config field naming mismatch (internal/api/risk.go:GetCircuitBreakers, internal/config/config.go)

The config field is MaxDailyLossDollars but it is used as a session lifetime PnL threshold (acknowledged in a comment). The dashboard label "Session Total Loss" is clearer, but the config name will mislead anyone who reads the config file. Consider renaming the field to MaxSessionLossDollars or documenting it explicitly in configs/config.yaml.

---

Low

8. buildBreaker returns gin.H (untyped map) (internal/api/risk.go)

gin.H (alias for map[string]interface{}) makes callers rely on string keys and interface{} values. A small struct with json tags would make this safer and testable without HTTP round-trips.

9. RiskHandler nil-cfg comment is misleading (internal/api/risk.go:NewRiskHandler)

"which causes setDefaults values to be applied at load time"

Viper's SetDefault runs at startup via the config loader, not when a zero-value struct is instantiated. A nil cfg results in a zero-value struct with all thresholds at 0, which causes every circuit breaker to return DISABLED (the threshold <= 0 guard), not the intended defaults. The comment should say this explicitly.

10. CircuitBreakerStatus empty state (web/dashboard/components/risk/CircuitBreakerStatus.tsx)

When circuitBreakers is an empty array (e.g., first load, config all-zero), the component renders with no breakers and no empty-state message. Users see a blank card with no indication of why. A simple "No circuit breakers configured" placeholder would improve UX.

11. quoteAsset has no tests and is order-dependent (cmd/api/handlers_trading.go)

The suffix matching loop is correct for standard symbols but silently falls back to "USDT" for unknown symbols, which means commission is credited to the wrong asset. A short unit test table covering BTCUSDT, ETHBTC, BNBBUSD, XRPBNB would prevent regressions.

---

Positive observations

• The TOCTOU race on paper session creation (23505 retry) is handled thoughtfully.
• WithTx with panic recovery is solid boilerplate.
• The errOppositeSide sentinel pattern for cross-cutting error handling is clean.
• Frontend type guards (isRiskMetrics, isCircuitBreakers, isExposure) and the console.warn on unexpected shapes are good defensive practices.
• The riskScore = null → "Unknown" state instead of misleading "Low Risk 100" is a nice UX fix.
• Test coverage for useTrades hook (4 cases) is thorough.

The phantom-insert race (#1) and the order-outside-transaction atomicity issue (#2) are the blockers to address before merging. The raw SQL in handler (#3) is a strong suggestion for a follow-up if not addressed here.

[claude]

PR Review: fix: paper trade data pipeline — persist fills, positions, and wire dashboard

This is a solid fix for a critical missing piece of the paper trading pipeline. The root cause (fills/positions never persisted) is correctly identified and fixed, the transaction design is thoughtful, and the frontend wiring is clean. A few issues below ranging from structural to minor.

High — Non-atomic order insert / fill split

File: cmd/api/handlers_trading.go

The order row is inserted before the WithTx transaction begins. If the transaction fails, the code tries to cancel the orphaned order in a separate non-atomic call — if that also fails, the order is left in NEW status permanently. The cleaner fix is to insert the order inside the transaction so the whole pipeline (order insert -> trade fill -> position create/update) is one atomic unit, eliminating the cancellation cleanup path entirely.

High — Raw SQL inside handler bypasses the DB layer

File: cmd/api/handlers_trading.go (the WithTx callback)

The trade fill and position inserts use tx.Exec with inline SQL directly in the HTTP handler, bypassing the existing db.InsertTrade, db.CreatePosition, and db.UpdatePositionAveraging methods. This means insert logic can drift from those methods over time and the handler becomes harder to test. The right pattern is to add transactional variants (similar to the new UpdateOrderStatusTx and GetOpenPositionBySymbolTx) and call those from the WithTx callback.

Medium — GetAllClosedPositions LIMIT 1000 warning will spam logs

File: internal/db/positions.go

GetMetrics is polled frequently by the dashboard. Once more than 1000 closed positions accumulate, the log.Warn fires on every request. Consider switching to a time-window query (e.g. WHERE exit_time > NOW() - INTERVAL '90 days') which is more meaningful for VaR anyway and naturally bounds the result set without a hard limit.

Medium — Missing DB index for GetOpenPositionBySymbolTx

File: internal/db/positions.go

The query WHERE symbol = $1 AND session_id = $2 AND exit_time IS NULL ... FOR UPDATE runs on the critical transaction path for every paper trade. There is no migration adding a composite index on (session_id, symbol, exit_time). Without it this is a full table scan under a row lock. A follow-up migration should add this index.

Medium — CountAllTrades hardcodes schema name public

File: internal/db/trades.go

Hardcoding nspname = 'public' breaks in multi-schema deployments (e.g. K8s with a non-default search_path). Using to_regclass('trades')::oid or current_schema() would be more portable.

Medium — VaR semantic mismatch between backend and frontend

File: internal/api/risk.go + web/dashboard/app/risk/page.tsx

The backend computes VaR over fractional returns (dimensionless, e.g. 0.023 = 2.3%). The frontend comment says var95/var99 are dollar PnL values and formats them with formatCurrency. These are different things — VaR over fractional returns needs to be multiplied by portfolio notional to yield a dollar figure. As written, the displayed "Value at Risk (95%)" will show something like $0.02 rather than a meaningful dollar amount. Please clarify what risk.VaRResult.VaR actually returns and ensure the formatting matches.

Medium — Hardcoded $100k initial capital for auto-created paper session

File: cmd/api/handlers_trading.go

configs/config.yaml has trading.initial_capital: 10000.0 but the auto-created paper session uses 100_000.0 (10x). Prefer s.config.Trading.InitialCapital so both are consistent.

Low — status: 'closed' for fill records is semantically misleading

File: web/dashboard/hooks/useTradeData.ts

A fill record is an execution event, not necessarily a closed position. A BUY fill opens a position; it is only closed by a matching SELL fill. Setting status: 'closed' on every fill means all BUY fills on currently open positions appear as closed in the UI.

What is done well

• Transaction design: RepeatableRead + FOR UPDATE on position lookup correctly prevents the duplicate-position race. The errOppositeSide sentinel for communicating a domain error through the transaction boundary is clean.
• WithTx helper: The panic recovery + rollback pattern is correct and reusable across the codebase.
• TOCTOU session creation: The 23505 retry-on-duplicate-key pattern correctly handles the race between ListActiveSessions and CreateSession.
• Frontend type guards: The runtime guards with console.warn on unexpected shapes are a solid defensive pattern.
• Removing all hardcoded mock constants from risk/page.tsx is exactly right — no more misleading "Low Risk" scores from static data.
• Integration test TestPaperTrade_PersistsAllRows: Tests all three rows (order, fill, position) in sequence — exactly the right scope for this bug fix.
• disabled circuit breaker state: Correctly handled at every layer — no false positives from unconfigured thresholds.
• Commission rate from config: Using s.config.Trading.CommissionRate with a safe fallback is the right approach.

---

Generated with Claude Code

[claude]

PR Review — fix: paper trade data pipeline

This is a solid, well-scoped fix. The core problem (missing InsertTrade/CreatePosition after order fill) is correctly diagnosed and the transaction wrapping is a real improvement. The detailed inline comments, TODOs, and limitation callouts make the intent clear. Here are my findings:

---

Bugs / Correctness Issues

1. db.Trade has no JSON struct tags — PascalCase fields leak to the frontend

db.Trade fields are serialized by encoding/json using their Go names (ID, OrderID, ExecutedAt, …), while every other API response uses snake_case DTOs. The RawApiTrade interface in useTradeData.ts handles this correctly today, but it is a fragile contract: if JSON tags are ever added to db.Trade to fix the inconsistency (which they should be), the frontend mapping will break silently. The right fix is a dedicated response DTO struct in internal/api/trades.go:

type tradeResponse struct {
    ID              string  `json:"id"`
    OrderID         string  `json:"order_id"`
    Symbol          string  `json:"symbol"`
    ...
}

This is the pattern already used by symbolExposureJSON and pairPerf in this same PR.

2. Integration test mock bypasses mapApiTrade — doesn't test the actual mapping

In useTradeData.test.tsx, fakeTrade is a camelCase Trade value, not a RawApiTrade (PascalCase). It is passed via as unknown as to bypass the type system. This means the test never exercises mapApiTrade, so regressions in that function (e.g. field name changes) won't be caught:

// Current: doesn't exercise the real mapping path
data: { trades: [fakeTrade], count: 1 } as unknown as { trades: Trade[]; count: number }

// Better: use a RawApiTrade-shaped object so mapApiTrade is actually called
data: { trades: [{ ID: 'trade-abc', Symbol: 'BTCUSDT', Side: 'BUY', Price: 45045, ... }] }

3. maxDrawdown unit mismatch risk in GetCircuitBreakers

buildBreaker("Max Drawdown %", maxDrawdown*100, h.cfg.MaxDrawdownPct)

This multiplies s.MaxDrawdown by 100 assuming it is stored as a fraction (e.g. 0.10). If trading_sessions.max_drawdown is already stored in percentage points (e.g. 10.0), the circuit breaker fires at 0.1% instead of 10%. Worth adding a comment asserting the stored unit, or a schema/DB comment to make this contract explicit.

4. FOR UPDATE + LIMIT 1 in GetOpenPositionBySymbolTx — semantics are correct but fragile

PostgreSQL's SELECT … ORDER BY entry_time DESC LIMIT 1 FOR UPDATE locks the single returned row, which is the intended behavior here. However, if two concurrent BUYs both see existingPos == nil (before either inserts), one will block on the lock and then find the row the other inserted — correct. If two concurrent BUYs see the same existing position, both will lock (one blocks) and then both call UpdatePositionAveragingTx — the second will use the already-updated existingPos.EntryPrice from its pre-transaction snapshot for the weighted average, producing a wrong result. Consider re-fetching existingPos after acquiring the lock to get the freshest values within the RepeatableRead snapshot.

5. WithTx does not attempt rollback after a failed Commit

return tx.Commit(ctx)

If Commit fails (network drop, server-side timeout), the function returns an error but never calls tx.Rollback. PostgreSQL will eventually roll back the connection-level transaction, but explicit cleanup is safer:

if err := tx.Commit(ctx); err != nil {
    _ = tx.Rollback(ctx)
    return fmt.Errorf("commit transaction: %w", err)
}
return nil

---

Missing Test Coverage

• Opposite-side rejection (422): no test exercises the errOppositeSide path.
• Position averaging: no test for a second BUY on the same symbol (the UpdatePositionAveragingTx path is untested).
• Limit order persistence: the limit-order branch (InsertOrder only, no transaction) has no integration test.
• TOCTOU retry on unique constraint (23505): the concurrent-session creation retry path is untested.

---

Performance Considerations

collectClosedReturns loads all closed positions for every /risk/metrics request

positions, err := h.db.GetAllClosedPositions(ctx)  // up to 90 days, no limit

In a busy paper-trading environment this query could scan thousands of rows on every dashboard refresh. Consider:

• Caching the result in Redis with a short TTL (30–60 s), or
• Adding a LIMIT to GetAllClosedPositions and documenting that VaR is computed on the last N trades.

Similarly GetExposureBySymbol and GetPairPerformance run GROUP BY aggregates on every request with no caching layer.

---

Minor / Nits

• MaxDailyLossDollars naming: The field name says "daily" but the comment and usage measure cumulative session loss. This creates a misleading config key and API response label. A follow-up rename to max_session_loss_dollars / MaxSessionLossDollars would avoid confusion before more users depend on the key name.

• quoteAsset suffix ordering: Checking "BTC" before "ETH" means a hypothetical "ETHBTC" pair returns "BTC" (correct), but "BTCETH" would return "ETH" instead of the correct "BTC". Adding a comment about why the order matters, or switching to an explicit split at the known suffix lengths, would make the intent clearer.

• mapApiTrade status heuristic: BUY → 'open', SELL → 'closed' is a rough approximation; a BUY fill after a partial close would incorrectly show as 'open'. The comment acknowledges this, but it's worth a TODO to drive resolution.

• sessions2/err2 on 23505 retry: The shadowed err2 is only checked for nil; a non-nil error is silently swallowed before the sessionID == nil check fails. Logging the lookup error at Warn level would help diagnose cloud-connectivity issues.

---

Summary

Category	Count
Bugs / Correctness	5
Missing tests	4
Performance	1
Minor / Nits	4

The transaction-wrapped fill pipeline, the FOR UPDATE lock to prevent duplicate positions, and the session-creation TOCTOU guard are all good engineering. The PascalCase JSON serialization of db.Trade is the most impactful issue to fix before merging, as it sets an inconsistent API contract that will be harder to correct later. The others are lower-priority but worth tracking.

[claude]

PR Review — fix: paper trade data pipeline

Great work fixing a fundamental gap in the paper trading pipeline. The transaction design is solid and the frontend wiring is clean. A few issues worth addressing before merge.

---

Critical

db.Trade lacks JSON struct tags — frontend mapping is fragile

mapApiTrade in useTradeData.ts relies on Go's default PascalCase encoding because db.Trade has no json: tags:

// db.Trade today — no json tags, so Go emits PascalCase keys
type Trade struct {
    ID    uuid.UUID
    Price float64
    ...
}

// frontend breaks silently if json tags are ever added
interface RawApiTrade {
  ID: string
  Price: number

This is a landmine: the moment anyone adds json:"id" or json:"price" to db.Trade (which is standard Go practice), the dashboard silently maps everything to undefined. Consider either (a) adding json tags to db.Trade now and updating RawApiTrade to use lowercase, or (b) adding a dedicated API response struct with explicit json tags so the DB model and API contract are decoupled.

---

Moderate

initial_capital config change from $10k → $100k is a silent breaking change

# configs/config.yaml
-  initial_capital: 10000.0
+  initial_capital: 100000.0

Any new PAPER session created after this deploy starts with $100k, which changes position sizing behaviour and circuit breaker math across the whole system. This looks like it belongs in a separate config discussion, not bundled into a bug fix PR. If intentional, it deserves a config migration note in TASKS.md.

VaR dollar scaling uses InitialCapital, not current portfolio value

In internal/api/risk.go:

for _, s := range activeSessions {
    portfolioValue += s.InitialCapital  // ← stale; should be CurrentCapital
}
// ...
response["var_95"] = varResult.VaR * portfolioValue

Dollar VaR (e.g. "$2,345 at 95% confidence") should be scaled by the current portfolio value, not the starting capital. After a winning or losing streak the number will be wrong. If TradingSession doesn't yet have a CurrentCapital column, the comment in the code should call this out explicitly rather than silently producing a misleading value.

GetAllClosedPositions hard-codes a 90-day window for VaR

The 90-day cutoff is reasonable for a fresh system, but in production a long-running system may have relevant older returns. More importantly, a fresh system with < 10 closed positions will always return nil VaR, which is fine — but there's no feedback to the operator about why. Worth logging data_points and noting the threshold in the response (the data_points field already does this partially, good).

---

Minor

console.warn fires on every render for bad API shapes

// risk/page.tsx — called directly in render, not in useMemo/useEffect
if (rawMetrics !== undefined && !isRiskMetrics(rawMetrics)) {
  console.warn('Unexpected API response shape for risk metrics:', rawMetrics)
}

If the API is returning an unexpected shape, this will spam the console on every re-render. Wrap the warn in a useEffect keyed on the response, or move the check inside the useMemo that already guards it.

StatusDot receives status='disabled' — verify it accepts that value

status={overallStatus === 'disabled' ? 'disabled' : 'healthy'}

StatusDot is not in this diff. If it doesn't accept 'disabled' as a prop value, TypeScript should catch it — but worth confirming npm run type-check was run against the CircuitBreakerStatus.tsx change specifically.

quoteAsset suffix ordering

for _, suffix := range []string{"USDT", "BUSD", "BTC", "ETH", "BNB"} {

This works correctly for all common Binance pairs today. The TODO to make slippage configurable should also note making the suffix list configurable, since exchanges like Kraken use different quote assets.

---

What's done well

• Transaction design: wrapping order insert + fill insert + position upsert in a single RepeatableRead tx with FOR UPDATE on the position is exactly right. The TOCTOU race on the session creation (unique-constraint retry) is handled cleanly.
• Sentinel error pattern (errOppositeSide → 422) is idiomatic and avoids corrupting position data.
• CountAllTrades via pg_class.reltuples: good O(1) estimate instead of a full scan; the reltuples = -1 guard is a nice touch.
• Partial index migration (idx_positions_session_symbol_open) is appropriately scoped and the CONCURRENTLY omission is correctly documented.
• Type guards in the frontend (isRiskMetrics, isCircuitBreakers, isExposure) are a solid pattern for handling unknown API responses defensively.
• riskScore returning null when no circuit breaker data is available (Issue Phase 10: Production Hardening - Complete Implementation #7 fix) is the right behaviour — avoids a misleading "Low Risk" score of 100.
• Test coverage: the integration test TestPaperTrade_PersistsAllRows directly verifies the three DB rows that were missing before (order fill, trade fill, open position). The four useTrades hook tests cover the key branches cleanly.

---

Overall this is a solid, well-structured fix. Resolve the JSON tag fragility before merge; the capital config change and VaR scaling should at minimum be called out explicitly in comments or follow-up tickets.

[claude]

PR Review — fix: paper trade data pipeline

Overall this is a solid, well-scoped fix. The core idea (wrapping all fill writes in a single RepeatableRead transaction) is correct and the code is clearly commented. A few issues below ranging from critical to minor.

Critical — Phantom insert race in GetOpenPositionBySymbolTx

FOR UPDATE in PostgreSQL only locks existing rows. If no position exists for a symbol, two concurrent market-buy requests will both execute SELECT ... FOR UPDATE (both return nil), both decide existingPos == nil, and both try to INSERT a new position. Result: duplicate open positions for the same (session_id, symbol).

RepeatableRead prevents phantom reads for rows that existed at snapshot time, but it does not block two transactions from each inserting a new row for the same key.

Fix options (pick one):

1. Add a UNIQUE partial index/constraint on (session_id, symbol) WHERE exit_time IS NULL. The second concurrent insert will fail with a 23505 and the caller can handle it.
2. Use SERIALIZABLE isolation (eliminates all phantom anomalies).
3. Use INSERT ... ON CONFLICT DO UPDATE for the position row.

Migration 018_positions_symbol_index.sql adds a partial index, not a unique constraint, so this race is still live.

High — Silent VaR unit mismatch when ListActiveSessions fails

In internal/api/risk.go, if the ListActiveSessions call fails, portfolioValue stays 0 and the API silently returns fractional VaR (e.g. 0.023) instead of dollar VaR (e.g. 2300). The frontend uses formatCurrency() and will display $0.02 — valid-looking but meaningless. Suggest adding a "var_unit" field or surfacing the session error as a warning so the frontend can detect degraded mode.

High — Tx helper methods duplicate SQL across non-Tx counterparts

InsertOrderTx/InsertOrder, InsertTradeTx/InsertTrade, and UpdateOrderStatusTx/UpdateOrderStatus each duplicate their SQL string and bind argument list. Any schema change must be applied to both copies. Consider extracting a shared SQL const and a helper that accepts a db.Querier interface so there is one source of truth.

Medium — CountAllTrades is search_path-sensitive

The query uses current_schema() to find the trades table in pg_class. This can return the wrong schema if the connection overrides search_path (TimescaleDB adds its own schema). Prefer 'public'::regnamespace or to_regclass('public.trades') for a stable reference.

Medium — WithTx panic rollback should use context.Background()

The deferred rollback in WithTx calls tx.Rollback(ctx), but if the panic was triggered by a cancelled context, this rollback will also fail. Use context.Background() for the rollback to ensure it always runs.

Medium — Paper session resolution is outside the trade transaction

The session lookup and optional CreateSession happen before WithTx. If CreateSession succeeds but WithTx fails, an empty session is committed. If AggregateSessionStats (also outside the tx) crashes after a successful commit, stats are stale until the next trade. Low probability, but worth a comment explaining the intentional consistency trade-off.

---

Positives

• errOppositeSide sentinel is clean — avoids logging a 422 as a 500.
• buildBreaker correctly guards threshold <= 0 to prevent false-positive TRIGGERED states for unconfigured values.
• GetAllClosedPositions scoped to 90 days is sensible for VaR relevance.
• Frontend type guards (isRiskMetrics, isCircuitBreakers, isExposure) in risk/page.tsx are solid defensive patterns.
• Integration test verifies all three DB rows (order + trade fill + position), not just the HTTP status code.
• Slippage constants and commission fallback carry TODO comments pointing to config — good hygiene.

Minor nits

• TASKS.md: the new CONFIG entry has no task ID or estimate and reads as a changelog note, not a task. Consider moving it to a CHANGELOG section or the commit message.
• mapApiTrade in useTradeData.ts sets agent: raw.exchange as a proxy. A TODO noting that a future agent_id field on the trade row would replace this helps future readers.
• quoteAsset() suffix ordering is correct for current pairs; a brief comment explaining why more-specific suffixes (BUSD before BTC) appear first would help future maintainers.

[claude]

PR Review: fix: paper trade data pipeline — persist fills, positions, and wire dashboard

This is a well-structured fix for a critical data-pipeline gap. The transaction design, locking strategy, and test coverage are solid. A few items worth addressing before merge:

---

Issues

1. var_unit ignored by frontend — VaR displayed incorrectly when no sessions exist

internal/api/risk.go — the backend sends either "dollar" or "fractional" in the var_unit field to signal whether VaR values are scaled by portfolio value. web/dashboard/app/risk/page.tsx ignores this field and always calls formatCurrency. If portfolioValue == 0 (no active sessions yet), the backend returns fractional values like 0.023 but the frontend formats them as $0.02, which is misleading.

// risk/page.tsx — needs a guard
const varUnit = (rawMetrics as any)?.var_unit  // "dollar" | "fractional" | undefined
const formatVar = (v: number) =>
  varUnit === 'dollar' ? formatCurrency(v) : formatPercentage(v)

Alternatively, consider always returning dollar-scaled VaR from the backend (skip the calculation when portfolioValue == 0 rather than switching units).

---

2. Unbounded queries in GetPairPerformance and GetExposureBySymbol

internal/db/positions.go — both queries have no LIMIT. With many historical trading pairs or open positions these will grow unbounded. Add a reasonable cap (e.g. LIMIT 200) or accept limit / offset parameters consistent with ListAllTrades.

// GetPairPerformance
ORDER BY realized_pnl DESC
LIMIT 200   // ← add

---

3. Integration test bypasses GetMarketPrice path

cmd/api/paper_trade_pipeline_test.go — the test sends "price":45000 with "type":"market". Since refPrice = req.Price when price > 0, GetMarketPrice is never exercised. Consider adding a second sub-test that omits price (and seeds a mock price) to cover the market-price lookup and its 400 error path.

---

Minor

4. Duplicate exposure data in stat cards

app/risk/page.tsx — Open Positions shows Exposure: $X as a subtitle, and Total Exposure is a separate card displaying the same value. One of the two is redundant. The Open Positions subtitle could show something more useful like average position size, or the Total Exposure card could show exposure as a % of initial capital.

5. migrations/018 is immediately superseded by 019

Migration 018 creates a non-unique index; migration 019 drops it and creates the unique version. Since 018 has no production release, collapsing them into a single migration would avoid the DROP/CREATE cycle on fresh installs. The two-migration approach is fine if there are environments where 018 is already applied, but the comment in 019 suggests this is a design iteration rather than a compatibility concern.

6. PtrFloat64 used only once

internal/db/utils.go — the helper is called at one site (db.PtrFloat64(req.Price)). That's fine as a utility, but noting it in case it was intended to replace the several other &req.X patterns elsewhere in the same file.

7. Hardcoded mockDrawdown still in production render path

app/risk/page.tsx — mockDrawdown is still wired to DrawdownChart (no real API backing). The PR summary mentions "no API equivalent yet" which is fair, but a // TODO comment and a placeholder that shows an empty state when data is unavailable would be cleaner than silently rendering fabricated data.

---

What's good

• Transaction design is correct: RepeatableRead + FOR UPDATE + unique partial index (019) gives three layers of protection against duplicate open positions on concurrent requests. The TOCTOU analysis in the comments is thorough.
• Sentinel error pattern (errOppositeSide) for 422 without logging noise is clean.
• SQL constants (sqlInsertOrder, sqlUpdateOrderStatus, sqlInsertTrade) eliminating the inline-string duplication between Tx and non-Tx variants is a nice cleanup.
• CountAllTrades using pg_class.reltuples is the right call for a paginated list endpoint — avoids a full scan on every page load.
• Runtime type guards in risk/page.tsx (isRiskMetrics, isCircuitBreakers, isExposure) prevent silent data corruption when the API shape changes.
• buildBreaker guards against threshold <= 0 to avoid false positives from unconfigured circuit breakers — good defensive coding.
• 4 new frontend hook tests + 1 integration test cover the happy path and several error cases well.

[claude]

PR Review: Paper Trade Data Pipeline

Good foundational work — the core bug fix is well-reasoned and the transaction design is solid. A few issues worth addressing before merge.

---

Bugs / Correctness

1. WithTx rolls back with a potentially cancelled context

In internal/db/db.go:

if err := fn(tx); err != nil {
    _ = tx.Rollback(ctx)   // ctx may be Done() here
    return err
}

If ctx is already cancelled when fn() returns an error (e.g. client disconnected), Rollback(ctx) will fail immediately and the connection stays in an aborted state until the pool reclaims it. Use context.Background() for the rollback call, same as the panic path already does.

---

2. GetAllClosedPositions has no LIMIT — could be very slow

collectClosedReturns in internal/api/risk.go calls GetAllClosedPositions, which fetches all closed positions in the last 90 days with no row cap. On a production system with thousands of closed positions this is an unbounded full scan. VaR with more than ~500 samples doesn't materially improve accuracy. Add a LIMIT 500 ORDER BY exit_time DESC (or make it configurable) to keep this O(1) in practice.

---

3. var_unit field in frontend is dead code

app/risk/page.tsx reads rawMetrics.var_unit to decide whether to format VaR as currency or percentage:

const varUnit = isRiskMetrics(rawMetrics) ? (rawMetrics.var_unit ?? 'dollar') : 'dollar'

But GetMetrics in internal/api/risk.go never sets var_unit in the response map. varUnit is always 'dollar', the default. The RawRiskMetrics type includes var_unit?: 'dollar' | 'fractional' but nothing ever populates it. Either add the field to the backend response or remove the dead branch from the frontend. As-is it creates a false impression that fractional fallback works.

---

4. Session creation retry loses the error context on lookup failure

sessions2, err2 := s.db.ListActiveSessions(ctx)
if err2 == nil {
    ...
}
if sessionID == nil {
    log.Error().Err(err).Msg("Failed to find paper session after unique constraint violation")

If err2 != nil the log message only shows the original unique-constraint error (err), not err2. The operator sees the wrong root cause. Log both or wrap:

if err2 != nil {
    log.Error().Err(err2).Msg("Retry lookup after 23505 also failed")
    ...
}

---

Performance

5. No index on trades(executed_at) for pagination

ListAllTrades orders by executed_at DESC and is the hot path for the dashboard. Without an index on that column every page request triggers a sequential scan + sort. Consider adding:

CREATE INDEX IF NOT EXISTS idx_trades_executed_at ON trades (executed_at DESC);

(Could go in migration 020 or alongside migration 018/019.)

6. GetMetrics makes two sequential DB round-trips

GetMetrics calls GetAllOpenPositions, then GetAllClosedPositions, then conditionally ListActiveSessions. These are three sequential queries. For a monitoring endpoint that auto-refreshes every few seconds this is noticeable. GetAllOpenPositions + ListActiveSessions could be combined into one query, or the VaR path could be a separate endpoint.

---

Design Notes (non-blocking)

7. handlePaperTrade is 278 lines — consider extracting helpers

The session resolution, price calculation, and fill transaction are all inline in one handler. This makes unit-testing individual pieces difficult. Extracting resolveOrCreatePaperSession, calculateExecPrice, and executeFillTx into unexported helpers would make this much easier to test and reason about.

8. Limit order path doesn't go through WithTx

Market orders are now fully atomic via WithTx, but the limit order else branch still calls s.db.InsertOrder(ctx, order) outside any transaction. This is probably fine since there's only one write, but it's worth noting the asymmetry — a future reader might wonder why.

9. mapApiTrade status heuristic

status: (raw.side === 'SELL' ? 'closed' : 'open') as Trade['status'],

The comment acknowledges this is a simplification, which is fine. But it means the trades list shows every BUY fill as "open" even after the position is closed. Consider whether this creates user confusion on the dashboard; if so a TODO comment pointing at the follow-up issue would be helpful.

---

What's Done Well

• Transaction design is correct: RepeatableRead + FOR UPDATE on the position row, combined with the UNIQUE partial index in migration 019 as a final-safety-net for the concurrent-insert race — this is a textbook defense-in-depth approach.
• errOppositeSide sentinel: clean pattern; separates control flow from logging without leaking implementation details to the caller.
• SQL constants extracted (sqlInsertOrder, sqlUpdateOrderStatus, sqlInsertTrade): reduces duplication between *Tx and non-Tx variants.
• Type guards in the frontend (isRiskMetrics, isCircuitBreakers, isExposure): these prevent silent crashes when the API shape changes, and the useEffect warnings are a good DX touch.
• CountAllTrades uses pg_class.reltuples: the doc comment clearly explains the trade-off and edge cases (fresh table = -1, stale after bulk inserts). Good O(1) approximation for pagination UI.
• Migration sequencing (018 → 019): keeping them as separate migrations instead of collapsing handles the case where 018 was already applied — IF EXISTS / IF NOT EXISTS idempotency is exactly right.

---

Summary

Severity	Count
Bug	4
Performance	2
Design / non-blocking	3

Items 1–4 are worth fixing before merge. Items 5–9 are suggestions.

[claude]

PR Review: fix: paper trade data pipeline

This is a substantial, well-structured PR that fixes a genuine data pipeline gap. The transaction design, migration strategy, and inline documentation are all high quality.

---

What is Done Well

• Transaction integrity: WithTx with RepeatableRead + FOR UPDATE + the unique partial index in migration 019 form a solid three-layer defense against duplicate open positions.
• Atomic insert pattern: moving InsertOrder inside the transaction as the first step ensures no orphaned order rows if a fill fails.
• Session TOCTOU handling: the 23505 retry pattern for concurrent paper session creation is correct and well-commented.
• SQL constant extraction: promoting inline SQL to package-level const avoids duplication between pool and Tx variants.
• buildBreaker disabled-guard: treating zero/negative threshold as DISABLED prevents false positives from unconfigured values.
• Test coverage: integration test covers three key cases (explicit price, derived price, missing price). Unit tests cover pure functions. Frontend hook tests use proper mocking and waitFor.

---

Bugs / Correctness Issues

1. err2 is silently swallowed in the 23505 retry block (cmd/api/handlers_trading.go)

When err2 != nil (the retry lookup itself fails), the error is silently discarded and the subsequent log still reports the original 23505 error. This makes it impossible to distinguish session-not-found-after-retry from the-retry-DB-call-failed. Suggest early-returning on non-nil err2 and logging err2 separately before the existing sessionID == nil check.

2. Extra UPDATE on a just-inserted row (minor)

InsertOrderTx writes OrderStatusNew, then UpdateOrderStatusTx immediately overwrites it to FILLED within the same transaction. Logically correct but adds an unnecessary round-trip. Consider inserting market orders with OrderStatusFilled directly.

---

Design Concerns

3. Config field name mismatch: MaxDailyLossDollars vs actual semantics

The field tracks total session loss, not daily loss (acknowledged in code comments). The config key is max_daily_loss_dollars, the struct field is MaxDailyLossDollars, and the circuit breaker label is "Session Total Loss" -- three different names for the same thing. A rename to MaxSessionLossDollars in a follow-up would prevent confusion if this value is ever referenced by the safety guard or alerting.

4. GetAllOpenPositions in GetMetrics is unbounded

GetExposureBySymbol caps at LIMIT 200 in SQL. GetAllOpenPositions has no cap. If sessions accumulate many positions, this endpoint does a full table scan and loads every row into Go memory on every request. An aggregating SQL query (similar to GetExposureBySymbol) that computes count and total exposure directly would be more efficient.

5. CountAllTrades returns an estimate with no indication to the caller

CountAllTrades uses pg_class.reltuples (documented as approximate). A client building pagination UI on the total field will see inconsistent page counts as autovacuum lags behind actual row count. Consider adding total_is_estimate to the response, or use a bounded subquery which is fast but exact for small tables.

---

Security

• All three new handler groups (/trades, /risk/, /performance/) apply AuthMiddleware. OK
• All DB queries use parameterized placeholders -- no injection surface. OK
• Error responses do not leak internal query detail. OK

---

Testing Gaps

6. No test for quoteAsset

The function has a non-trivial suffix-ordering dependency. A table-driven test covering ETHBTC, BNBETH, XRPBUSD, and an unrecognized symbol would pin the ordering contract and catch regressions.

7. GetCircuitBreakers with zero-value config is untested at handler level

TestBuildBreaker covers buildBreaker well, but there is no handler-level test verifying that a zero-value RiskConfig (all thresholds unset) produces DISABLED status for all breakers. Since the dashboard renders these values directly, a handler test would provide confidence.

---

Minor / Style

8. Limit order path uses non-transactional InsertOrder

Technically correct (limit orders have no downstream rows to roll back) but asymmetric with the market order path. A short inline comment explaining why no transaction is needed would help future contributors adding limit order fill logic.

9. Two migrations where one immediately supersedes the other

Migration 018 (non-unique index) is immediately superseded by 019 (drop + unique index). For a fresh install both run: create index, drop it, create a different one. The forward-compatibility rationale is valid and well-commented. If 018 has not been deployed to any environment yet, collapsing them into a single migration would be cleaner.

---

Summary

Category	Notes
Correctness	One silent error discard to fix (item 1)
Concurrency safety	Solid: RepeatableRead + FOR UPDATE + unique index
Performance	One unbounded query in GetMetrics (item 4)
Security	No concerns
Test coverage	Good; two minor gaps noted
Code quality	High; comments thorough and honest about known limitations

Item 1 (silent err2 swallow) is the only issue I would want addressed before merge. Items 4 and 5 are worth a TASKS.md entry if not fixed now. Everything else is follow-up material or style preference.