chore: cleanup duplicate trade save, misleading cycle counters, and /api/summary inconsistencies
CI/CD / build-and-push (push) Successful in 7s
CI/CD / build-and-push (push) Successful in 7s
Bug #5: metrics.record_trade() only delegated to save_trade(), which executor.execute() already calls — every trade was written twice (deduped only by ON CONFLICT DO NOTHING). Remove the redundant call and the now-dead method. RealExecutor.execute() raises NotImplementedError, so real mode is unaffected. Bug #6 (CYCLE SUMMARY): manifold accepted/rejected counters only increment on the active-signal path, so with MANIFOLD_SIGNAL_ENABLED=false they always printed 0/0 — print 'manifold_signal: disabled' instead. family_conflicts_prevented duplicated blocked_by_family (same counter printed twice); removed. gnews_cap was a dead variable with a misleading comment; removed. Bug #7 (/api/summary): total_trades was len() over a LIMIT-500 query — capped once history grows; counts now come from COUNT(*) via compute_metrics_from_db. cash_available was reimplemented in the API; extract cash_available() in paper.py (same formula, unchanged) and feed it from get_open_position_data() — the exact source/helper PaperExecutor.initialize() uses. Test asserts API and executor report identical cash for the same DB state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
02cbfc0b94
commit
7ebb87aede
+14
-9
@@ -11,6 +11,7 @@ from fastapi import FastAPI
|
|||||||
from fastapi.middleware.cors import CORSMiddleware
|
from fastapi.middleware.cors import CORSMiddleware
|
||||||
|
|
||||||
from bot.data.db import Database
|
from bot.data.db import Database
|
||||||
|
from bot.executor.paper import cash_available
|
||||||
|
|
||||||
# Phase 6 format (Phase 6+): values already in log-odds space.
|
# Phase 6 format (Phase 6+): values already in log-odds space.
|
||||||
# "fg_lo=+0.1200 mom_lo=+0.0000 news_lo=+0.0000 mfld_lo=-0.7483 btc_dom_lo=+0.0000"
|
# "fg_lo=+0.1200 mom_lo=+0.0000 news_lo=+0.0000 mfld_lo=-0.7483 btc_dom_lo=+0.0000"
|
||||||
@@ -280,27 +281,31 @@ async def get_summary():
|
|||||||
which is written by the bot every cycle via MetricsTracker.update_daily_summary().
|
which is written by the bot every cycle via MetricsTracker.update_daily_summary().
|
||||||
After Fix 3, that snapshot is also DB-computed — not dependent on pod restarts.
|
After Fix 3, that snapshot is also DB-computed — not dependent on pod restarts.
|
||||||
"""
|
"""
|
||||||
latest_metrics, open_trades, all_trades, inverted, legacy_count = await asyncio.gather(
|
latest_metrics, counts, position_data, inverted, legacy_count = await asyncio.gather(
|
||||||
db.get_metrics_history(days=1),
|
db.get_metrics_history(days=1),
|
||||||
db.get_recent_trades(limit=500, status="open"),
|
db.compute_metrics_from_db(),
|
||||||
db.get_recent_trades(limit=500),
|
db.get_open_position_data(),
|
||||||
db.get_recently_closed_inverted(hours=24),
|
db.get_recently_closed_inverted(hours=24),
|
||||||
db.get_legacy_incomplete_count(),
|
db.get_legacy_incomplete_count(),
|
||||||
)
|
)
|
||||||
|
|
||||||
latest = latest_metrics[0] if latest_metrics else {}
|
latest = latest_metrics[0] if latest_metrics else {}
|
||||||
paper_bankroll = float(os.getenv("PAPER_BANKROLL", "10000"))
|
paper_bankroll = float(os.getenv("PAPER_BANKROLL", "10000"))
|
||||||
total_deployed = sum(t.get("net_cost", 0) for t in open_trades)
|
total_trades = int(counts["total_trades"] or 0)
|
||||||
|
# Same source PaperExecutor.initialize() uses to reconstruct cash:
|
||||||
|
# total_net_cost_open = SUM(net_cost) over open trades, uncapped.
|
||||||
|
_, total_net_cost_open = position_data
|
||||||
|
total_deployed = total_net_cost_open
|
||||||
|
|
||||||
return {
|
return {
|
||||||
# ── Portfolio state (live from DB) ──────────────────────────────────
|
# ── Portfolio state (live from DB) ──────────────────────────────────
|
||||||
"paper_mode": os.getenv("PAPER_MODE", "true") == "true",
|
"paper_mode": os.getenv("PAPER_MODE", "true") == "true",
|
||||||
"paper_bankroll": paper_bankroll,
|
"paper_bankroll": paper_bankroll,
|
||||||
"total_trades": len(all_trades), # exact, from DB
|
"total_trades": total_trades, # COUNT(*), uncapped
|
||||||
"open_trades_count": len(open_trades), # exact, from DB
|
"open_trades_count": int(counts["open_count"] or 0), # COUNT(*), uncapped
|
||||||
"closed_trades_count": len(all_trades) - len(open_trades), # exact
|
"closed_trades_count": int(counts["closed_count"] or 0), # COUNT(*), uncapped
|
||||||
"total_deployed": total_deployed, # exact, from DB
|
"total_deployed": total_deployed, # exact, from DB
|
||||||
"cash_available": max(0.0, paper_bankroll - total_deployed), # exact
|
"cash_available": cash_available(paper_bankroll, total_net_cost_open),
|
||||||
"legacy_incomplete_count": legacy_count, # exact, from DB
|
"legacy_incomplete_count": legacy_count, # exact, from DB
|
||||||
"reentry_guard_blocks_24h": len(inverted), # exact, from DB
|
"reentry_guard_blocks_24h": len(inverted), # exact, from DB
|
||||||
|
|
||||||
@@ -333,6 +338,6 @@ async def get_summary():
|
|||||||
(latest.get("sharpe_ratio") or 0) >= 0.5
|
(latest.get("sharpe_ratio") or 0) >= 0.5
|
||||||
and (latest.get("win_rate") or 0) >= 0.52
|
and (latest.get("win_rate") or 0) >= 0.52
|
||||||
and (latest.get("calibration_score") or 0) >= 0.7
|
and (latest.get("calibration_score") or 0) >= 0.7
|
||||||
and len(all_trades) >= 50
|
and total_trades >= 50
|
||||||
),
|
),
|
||||||
}
|
}
|
||||||
|
|||||||
+12
-1
@@ -36,6 +36,17 @@ def _notify_in_background(coro) -> None:
|
|||||||
task.add_done_callback(_background_tasks.discard)
|
task.add_done_callback(_background_tasks.discard)
|
||||||
|
|
||||||
|
|
||||||
|
def cash_available(bankroll: float, total_net_cost_open: float) -> float:
|
||||||
|
"""Cash left after the net cost (fees included) of all open positions.
|
||||||
|
|
||||||
|
Single source of truth for the cash figure, shared by
|
||||||
|
PaperExecutor.initialize() and the /api/summary endpoint so both always
|
||||||
|
report the same number for the same DB state.
|
||||||
|
total_net_cost_open comes from Database.get_open_position_data().
|
||||||
|
"""
|
||||||
|
return max(0.0, bankroll - total_net_cost_open)
|
||||||
|
|
||||||
|
|
||||||
@dataclass
|
@dataclass
|
||||||
class Trade:
|
class Trade:
|
||||||
id: str
|
id: str
|
||||||
@@ -121,7 +132,7 @@ class PaperExecutor:
|
|||||||
|
|
||||||
positions_value = sum(positions_size.values())
|
positions_value = sum(positions_size.values())
|
||||||
self._portfolio.positions = positions_size
|
self._portfolio.positions = positions_size
|
||||||
self._portfolio.cash = max(0.0, self._portfolio.cash - total_net_cost)
|
self._portfolio.cash = cash_available(self._portfolio.cash, total_net_cost)
|
||||||
|
|
||||||
total_value = self._portfolio.cash + positions_value
|
total_value = self._portfolio.cash + positions_value
|
||||||
exposure_pct = positions_value / total_value if total_value > 0 else 0.0
|
exposure_pct = positions_value / total_value if total_value > 0 else 0.0
|
||||||
|
|||||||
+19
-9
@@ -11,7 +11,12 @@ from bot.data.polymarket import PolymarketClient, Market, market_family_key
|
|||||||
from bot.data.external import ExternalDataClient
|
from bot.data.external import ExternalDataClient
|
||||||
from bot.data.news import NewsClient
|
from bot.data.news import NewsClient
|
||||||
from bot.data.manifold import ManifoldClient
|
from bot.data.manifold import ManifoldClient
|
||||||
from bot.strategy.bayesian import BayesianStrategy, gnews_priority, MAX_NEWS_QUERIES_PER_CYCLE
|
from bot.strategy.bayesian import (
|
||||||
|
BayesianStrategy,
|
||||||
|
gnews_priority,
|
||||||
|
MAX_NEWS_QUERIES_PER_CYCLE,
|
||||||
|
MANIFOLD_SIGNAL_ENABLED,
|
||||||
|
)
|
||||||
from bot.risk.manager import RiskManager
|
from bot.risk.manager import RiskManager
|
||||||
from bot.executor.paper import PaperExecutor
|
from bot.executor.paper import PaperExecutor
|
||||||
from bot.metrics.tracker import MetricsTracker
|
from bot.metrics.tracker import MetricsTracker
|
||||||
@@ -199,7 +204,6 @@ async def run_trading_loop(
|
|||||||
# 7. Execute (paper)
|
# 7. Execute (paper)
|
||||||
trade = await executor.execute(order)
|
trade = await executor.execute(order)
|
||||||
if trade:
|
if trade:
|
||||||
await metrics.record_trade(trade)
|
|
||||||
log.info("Trade executed: %s", trade)
|
log.info("Trade executed: %s", trade)
|
||||||
# Block this family for the rest of the cycle (Phase 2)
|
# Block this family for the rest of the cycle (Phase 2)
|
||||||
occupied_families.add(signal.family_key)
|
occupied_families.add(signal.family_key)
|
||||||
@@ -221,7 +225,17 @@ async def run_trading_loop(
|
|||||||
if denom == 0:
|
if denom == 0:
|
||||||
return "0% (0/0)"
|
return "0% (0/0)"
|
||||||
return f"{n * 100 // denom}% ({n}/{denom})"
|
return f"{n * 100 // denom}% ({n}/{denom})"
|
||||||
gnews_cap = strategy._news_queries_this_cycle # already updated by reset below
|
|
||||||
|
# The accepted/rejected counters only increment on the active-signal
|
||||||
|
# path, so with the signal disabled they always print 0/0 — say
|
||||||
|
# "disabled" instead of pretending the matcher found nothing.
|
||||||
|
if MANIFOLD_SIGNAL_ENABLED:
|
||||||
|
manifold_summary = (
|
||||||
|
f" manifold_matches_accepted: {stats['manifold_matches_accepted']}\n"
|
||||||
|
f" manifold_matches_rejected: {stats['manifold_matches_rejected']}"
|
||||||
|
)
|
||||||
|
else:
|
||||||
|
manifold_summary = " manifold_signal: disabled"
|
||||||
|
|
||||||
log.info(
|
log.info(
|
||||||
"[CYCLE SUMMARY]\n"
|
"[CYCLE SUMMARY]\n"
|
||||||
@@ -239,9 +253,7 @@ async def run_trading_loop(
|
|||||||
" gnews_queries_used: %d/%d\n"
|
" gnews_queries_used: %d/%d\n"
|
||||||
" reentry_guard_blocked: %d\n"
|
" reentry_guard_blocked: %d\n"
|
||||||
" legacy_incomplete_seen: %d\n"
|
" legacy_incomplete_seen: %d\n"
|
||||||
" family_conflicts_prevented: %d\n"
|
"%s",
|
||||||
" manifold_matches_accepted: %d\n"
|
|
||||||
" manifold_matches_rejected: %d",
|
|
||||||
n_total,
|
n_total,
|
||||||
n_uncertainty,
|
n_uncertainty,
|
||||||
stats["max_edge_gross"],
|
stats["max_edge_gross"],
|
||||||
@@ -256,9 +268,7 @@ async def run_trading_loop(
|
|||||||
stats["gnews_queries_used"], MAX_NEWS_QUERIES_PER_CYCLE,
|
stats["gnews_queries_used"], MAX_NEWS_QUERIES_PER_CYCLE,
|
||||||
reentry_guard_count,
|
reentry_guard_count,
|
||||||
legacy_incomplete_count,
|
legacy_incomplete_count,
|
||||||
stats["skip_family"],
|
manifold_summary,
|
||||||
stats["manifold_matches_accepted"],
|
|
||||||
stats["manifold_matches_rejected"],
|
|
||||||
)
|
)
|
||||||
|
|
||||||
# 9. Update daily metrics
|
# 9. Update daily metrics
|
||||||
|
|||||||
@@ -21,7 +21,6 @@ import logging
|
|||||||
from datetime import datetime, UTC
|
from datetime import datetime, UTC
|
||||||
|
|
||||||
from bot.data.db import Database
|
from bot.data.db import Database
|
||||||
from bot.executor.paper import Trade
|
|
||||||
|
|
||||||
log = logging.getLogger(__name__)
|
log = logging.getLogger(__name__)
|
||||||
|
|
||||||
@@ -30,11 +29,6 @@ class MetricsTracker:
|
|||||||
def __init__(self, db: Database) -> None:
|
def __init__(self, db: Database) -> None:
|
||||||
self._db = db
|
self._db = db
|
||||||
|
|
||||||
async def record_trade(self, trade: Trade) -> None:
|
|
||||||
"""Persist a trade to the DB. No in-memory accumulation."""
|
|
||||||
await self._db.save_trade(trade)
|
|
||||||
log.info("Trade recorded: %s", trade)
|
|
||||||
|
|
||||||
async def update_daily_summary(self) -> None:
|
async def update_daily_summary(self) -> None:
|
||||||
"""Compute metrics from DB and write a metrics_daily snapshot.
|
"""Compute metrics from DB and write a metrics_daily snapshot.
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,102 @@
|
|||||||
|
"""
|
||||||
|
Tests for bug #7 — /api/summary must agree with the executor's cash model.
|
||||||
|
|
||||||
|
Regression: /api/summary computed total_trades as len() over a LIMIT-500
|
||||||
|
query (capped once history grows) and reimplemented cash as
|
||||||
|
bankroll - sum(net_cost of open trades) from that same capped query.
|
||||||
|
|
||||||
|
Fix: counts come from COUNT(*) (compute_metrics_from_db) and cash comes from
|
||||||
|
cash_available() — the same helper PaperExecutor.initialize() uses — fed by
|
||||||
|
the same source (get_open_position_data). This test runs both consumers
|
||||||
|
against one fake DB state and asserts they report identical cash.
|
||||||
|
"""
|
||||||
|
import asyncio
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
import api.main as api_main
|
||||||
|
from bot.executor.paper import PaperExecutor, cash_available
|
||||||
|
|
||||||
|
|
||||||
|
BANKROLL = 10_000.0 # PAPER_BANKROLL default used by both bot and API
|
||||||
|
|
||||||
|
|
||||||
|
class FakeDB:
|
||||||
|
"""One DB state served to both the API endpoint and the executor."""
|
||||||
|
|
||||||
|
def __init__(self, positions: dict[str, float], total_net_cost: float,
|
||||||
|
total_trades: int, open_count: int):
|
||||||
|
self._positions = positions
|
||||||
|
self._total_net_cost = total_net_cost
|
||||||
|
self._total = total_trades
|
||||||
|
self._open = open_count
|
||||||
|
|
||||||
|
# Shared source: executor.initialize() and /api/summary both call this.
|
||||||
|
async def get_open_position_data(self):
|
||||||
|
return dict(self._positions), self._total_net_cost
|
||||||
|
|
||||||
|
# /api/summary only:
|
||||||
|
async def get_metrics_history(self, days=1):
|
||||||
|
return []
|
||||||
|
|
||||||
|
async def compute_metrics_from_db(self):
|
||||||
|
return {
|
||||||
|
"total_trades": self._total,
|
||||||
|
"open_count": self._open,
|
||||||
|
"closed_count": self._total - self._open,
|
||||||
|
}
|
||||||
|
|
||||||
|
async def get_recently_closed_inverted(self, hours=24):
|
||||||
|
return set()
|
||||||
|
|
||||||
|
async def get_legacy_incomplete_count(self):
|
||||||
|
return 0
|
||||||
|
|
||||||
|
|
||||||
|
def _run(db: FakeDB, monkeypatch) -> tuple[dict, PaperExecutor]:
|
||||||
|
monkeypatch.setattr(api_main, "db", db)
|
||||||
|
monkeypatch.delenv("PAPER_BANKROLL", raising=False)
|
||||||
|
|
||||||
|
async def run():
|
||||||
|
summary = await api_main.get_summary()
|
||||||
|
ex = PaperExecutor(db=db, bankroll=BANKROLL)
|
||||||
|
await ex.initialize()
|
||||||
|
return summary, ex
|
||||||
|
|
||||||
|
return asyncio.run(run())
|
||||||
|
|
||||||
|
|
||||||
|
def test_api_and_executor_report_same_cash(monkeypatch):
|
||||||
|
db = FakeDB(
|
||||||
|
positions={"m1": 100.0, "m2": 80.0},
|
||||||
|
total_net_cost=183.60, # 180 + fees
|
||||||
|
total_trades=12,
|
||||||
|
open_count=2,
|
||||||
|
)
|
||||||
|
summary, ex = _run(db, monkeypatch)
|
||||||
|
assert summary["cash_available"] == pytest.approx(ex.get_portfolio().cash)
|
||||||
|
assert summary["cash_available"] == pytest.approx(
|
||||||
|
cash_available(BANKROLL, 183.60)
|
||||||
|
)
|
||||||
|
assert summary["total_deployed"] == pytest.approx(183.60)
|
||||||
|
|
||||||
|
|
||||||
|
def test_total_trades_not_capped_by_query_limit(monkeypatch):
|
||||||
|
"""700 trades in DB: the old len(LIMIT 500) reported 500."""
|
||||||
|
db = FakeDB(
|
||||||
|
positions={"m1": 100.0},
|
||||||
|
total_net_cost=102.0,
|
||||||
|
total_trades=700,
|
||||||
|
open_count=1,
|
||||||
|
)
|
||||||
|
summary, _ = _run(db, monkeypatch)
|
||||||
|
assert summary["total_trades"] == 700
|
||||||
|
assert summary["open_trades_count"] == 1
|
||||||
|
assert summary["closed_trades_count"] == 699
|
||||||
|
|
||||||
|
|
||||||
|
def test_cash_consistency_with_no_open_positions(monkeypatch):
|
||||||
|
db = FakeDB(positions={}, total_net_cost=0.0, total_trades=0, open_count=0)
|
||||||
|
summary, ex = _run(db, monkeypatch)
|
||||||
|
assert summary["cash_available"] == pytest.approx(BANKROLL)
|
||||||
|
assert ex.get_portfolio().cash == pytest.approx(BANKROLL)
|
||||||
Reference in New Issue
Block a user