Repository navigation
PnL Engine + Tests + CI/CD - #3
Conversation
…date .gitignore to ignore dev notes
…d github actions ci/cd draft for running the build + ruff + mypy + tests
Test Results26 tests 26 ✅ 0s ⏱️ Results for commit e8a4ecc. ♻️ This comment has been updated with latest results. |
…ue to issues with setuptools (which is overkill atm)
📝 WalkthroughWalkthroughThis pull request establishes a complete trading system foundation by introducing CI/CD workflows, project infrastructure (pyproject.toml, requirements.txt, .gitignore, pre-commit hooks, environment configuration), domain models (Trade, Position, Side), a RealTimePnLEngine for real-time profit/loss attribution, and comprehensive unit tests validating the engine's trade processing, position tracking, and PnL calculations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Engine as RealTimePnLEngine
participant TradeHandler as Trade Handler
participant PosManager as Position Manager
participant PnLTracker as PnL Tracker
Client->>Engine: on_trade(trade)
activate Engine
Engine->>Engine: validate trade (qty>0, price>0)
Engine->>PosManager: get_position(symbol)
activate PosManager
PosManager-->>Engine: position
deactivate PosManager
alt Same Direction
Engine->>TradeHandler: _handle_same_direction_trade(pos, qty, price)
activate TradeHandler
TradeHandler->>PosManager: update avg_cost (weighted avg)
TradeHandler->>PosManager: update qty (increase position)
TradeHandler-->>Engine: position updated
deactivate TradeHandler
else Opposite Direction
Engine->>TradeHandler: _handle_opposite_direction_trade(pos, qty, price)
activate TradeHandler
TradeHandler->>PnLTracker: _realize_pnl(symbol, closing_qty, price)
activate PnLTracker
PnLTracker->>PnLTracker: calculate realized PnL
PnLTracker->>PnLTracker: accumulate per-symbol realized PnL
PnLTracker-->>TradeHandler: pnl computed
deactivate PnLTracker
TradeHandler->>PosManager: update position (close or flip)
TradeHandler-->>Engine: position settled
deactivate TradeHandler
end
deactivate Engine
Client->>Engine: on_price(symbol, price)
activate Engine
Engine->>Engine: store latest_prices[symbol]
deactivate Engine
Client->>Engine: get_total_pnl()
activate Engine
Engine->>PnLTracker: sum realized_pnl across all symbols
Engine->>Engine: compute unrealized PnL for each position
Engine-->>Client: realized + unrealized total
deactivate Engine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@core/pnl_engine.py`:
- Around line 112-121: The None check in get_unrealized_pnl is dead because
last_prices is a defaultdict(float); either switch last_prices to a plain dict
in __init__ (self.last_prices: dict[str, float] = {}) so last_prices.get(symbol)
can return None and the existing None-check in get_unrealized_pnl remains
correct, or keep defaultdict(float) and remove the unreachable "if last_px is
None" branch and treat 0.0 as the no-price sentinel; update the code paths
referencing last_prices, including get_unrealized_pnl and any other usage, to
follow the chosen approach.
In `@models/domain.py`:
- Line 44: The Position.updated_at field uses
default_factory=datetime.now().astimezone which evaluates once and is shared
across instances; change it to a callable that returns a fresh timestamp each
instantiation (e.g., use a lambda that calls datetime.now().astimezone()) so
Position.updated_at produces a new datetime per instance—update the Position
dataclass field definition accordingly (same fix pattern as for
Trade.timestamp).
- Line 27: The Trade dataclass's timestamp field uses
default_factory=datetime.now().astimezone which is evaluated at import time;
change the default_factory to a zero-arg callable that produces a fresh
timezone-aware datetime for each instance (e.g. use a lambda or function that
returns datetime.now().astimezone()). Update the timestamp field (symbol:
timestamp in the Trade dataclass) to use that callable so each new Trade gets
the current time when constructed.
In `@requirements.txt`:
- Line 14: requirements.txt pins ruff to 0.1.11 while .pre-commit-config.yaml
uses v0.14.13, causing inconsistent linting; pick one canonical version and make
both files match (preferably update requirements.txt to ruff==0.14.13 to match
.pre-commit-config.yaml or update .pre-commit-config.yaml to v0.1.11 if you must
keep the older version), then run the linter locally to verify no
version-specific failures; ensure the version string "ruff==0.1.11" or
"v0.14.13" is updated accordingly in requirements.txt and
.pre-commit-config.yaml so both reference the same ruff release.
- Line 11: Requirements pin for FastAPI is vulnerable; update the fastapi
package entry in requirements.txt from fastapi[all]==0.104.1 to a secure version
(e.g., fastapi[all]>=0.109.1) to remediate PYSEC-2024-38 / CVE-2024-24762,
keeping the extras ([all]) intact and ensuring any dependency resolution or
lockfile regeneration is performed after the change.
In `@tests/conftest.py`:
- Around line 1-20: Remove the unused pytest fixture named mock_logger from
tests/conftest.py: locate the fixture definition "def mock_logger() ->
MagicMock" and delete that whole fixture so only the used fixtures sample_trade
and sample_position remain; ensure imports still needed (remove MagicMock import
if no longer used) to keep the file clean.
In `@tests/unit/test_pnl_engine.py`:
- Around line 62-64: The test's weighted average cost assertion is dividing by
sample_trade.price instead of the total quantity; update the assertion to divide
the combined notional by the total quantity (e.g. use pos.qty or the explicit
total 150) so avg_cost is computed as (sample_trade.notional_value() + 50 *
160.0) / pos.qty; locate this in the test where pos: Position =
engine.get_position("AAPL") and replace the incorrect denominator
sample_trade.price with pos.qty (or 150).
🧹 Nitpick comments (7)
pyproject.toml (1)
47-50: Consolidate pytest marker definitions:acceptancemarker missing inpyproject.toml.
pytest.inidefines markersunit,integration,acceptance, andslow, butpyproject.tomlonly definesunit,integration, andslow. The missingacceptancemarker will cause inconsistency when marker definitions are split across files. Move all marker definitions to a single location to ensure consistency and prevent drift..github/workflows/ci.yml (1)
3-5: Use list syntax for branches.For consistency with GitHub Actions conventions and to avoid potential parsing issues, use the list syntax for the branches filter.
Suggested fix
on: pull_request: - branches: master + branches: [master].gitignore (1)
56-57: Minor: Add space after#in comment.For consistency with other comments in the file.
-#Dev Notes +# Dev Notes notes/core/pnl_engine.py (2)
14-16: Consider using a regular dict instead ofdefaultdictfor positions.The
defaultdictcreates aPositionwithsymbol=""for missing keys, which could lead to confusing behavior. Sinceon_tradeexplicitly checks and creates positions with the correct symbol (lines 39-40), the default factory is never used properly. A regular dict with explicit key checks would be clearer.Suggested simplification
- self.positions: dict[str, Position] = defaultdict( - lambda: Position(account_id=self.account_id, symbol="") - ) + self.positions: dict[str, Position] = {}
88-95: Type hints inconsistent with actual usage.Parameters
closing_qty,prev_cost, andprev_qtyare typed asfloat, butqtyinTradeandPositionisint. Consider usingintforclosing_qtyandprev_qtyto match the domain model.Suggested fix
def _realize_pnl( self, symbol: str, - closing_qty: float, + closing_qty: int, trade_price: float, prev_cost: float, - prev_qty: float, + prev_qty: int, ):models/domain.py (1)
11-13:Sideenum is defined but unused.The
Sideenum is not used anywhere in this codebase. Either remove it if not needed, or add it to the public exports inmodels/__init__.pyif it's intended for future use.tests/unit/test_pnl_engine.py (1)
15-15: Consider using comments instead of standalone string literals for section headers.Lines 15, 51, 93, 148, 178, 213, 256, and 289 use bare string literals as section headers. While this works (they're no-ops), conventional Python style uses
#comments for inline documentation that isn't a docstring.- """Basic Tests""" + # Basic Tests
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…update dependencies in requirements.txt
…ives into feat/pnl-engine
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/unit/test_pnl_engine.py`:
- Around line 1-330: The file tests/unit/test_pnl_engine.py fails the Ruff
formatting check; run the formatter (e.g., ruff format
tests/unit/test_pnl_engine.py) to reformat the file and stage the changes before
pushing; ensure the test class TestRealTimePnLEngine and its test_* functions
(e.g., test_initial_state_empty, test_on_trade_buy_creates_position,
test_get_total_pnl_combines_realized_and_unrealized) remain unchanged in
behavior after formatting.
- Around line 156-157: The inline comment after engine.on_trade(trade_sell_more)
is incorrect: it says "Sell 150@140" while the trade object trade_sell_more was
created with price=160.0; update the comment to reflect the actual trade (e.g.,
"Sell 150@160") or change the trade_sell_more creation to price=140.0 to match
the comment—locate the usage at engine.on_trade(trade_sell_more) and the trade
definition for trade_sell_more to keep the test and comment consistent.
🧹 Nitpick comments (1)
tests/unit/test_pnl_engine.py (1)
15-15: Standalone string literals are no-ops; use comments for section headers.
"""Basic Tests"""and similar strings at lines 51, 93, 148, 178, 213, 256, 289 are not docstrings (they're not immediately following a class/function definition). They're compiled as unused expressions.♻️ Suggested fix
- """Basic Tests""" + # --- Basic Tests ---Apply the same pattern to all section headers throughout the file.
…f positive/negative qty; minor nits; test updates
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| last_px = self.last_prices[symbol] | ||
| return pos.unrealized_pnl(last_px) |
There was a problem hiding this comment.
🔴 Unrealized PnL calculated with price=0 when no market price has been set
When get_unrealized_pnl is called for a symbol that has a position but no price has been set via on_price, it silently uses 0.0 as the price due to last_prices being a defaultdict(float). This leads to wildly incorrect PnL calculations.
Click to expand
How the bug is triggered
- A trade is processed via
on_trade(), creating a position get_unrealized_pnl()is called beforeon_price()sets the market price- At line 126,
last_px = self.last_prices[symbol]returns 0.0 (defaultdict default) - The unrealized PnL is calculated as
qty * (0.0 - avg_cost)=-qty * avg_cost
Example
engine = RealTimePnLEngine()
engine.on_trade(Trade(symbol='AAPL', side=Side.BUY, qty=100, price=150.0))
unrealized = engine.get_unrealized_pnl('AAPL') # Returns -15000.0 instead of 0 or errorImpact
get_unrealized_pnl()returns incorrect valuesget_total_pnl()at line 131 returns incorrect totalsget_pnl_by_symbol()at line 149 returns incorrect unrealized/totallog_summary()at line 164 displays incorrect unrealized PnL
For a long position bought at $150, the unrealized PnL would be reported as -$15,000 (a massive loss) when no price update has been received, rather than indicating that the price is unknown.
Recommendation: Either check if the symbol exists in last_prices and raise an error or return 0/None when the price is missing, or use a regular dict instead of defaultdict for last_prices and handle KeyError appropriately. For example:
def get_unrealized_pnl(self, symbol: str) -> float:
pos = self.get_position(symbol)
if pos.qty == 0:
return 0.0
if symbol not in self.last_prices:
raise ValueError(f"No price available for {symbol}")
return pos.unrealized_pnl(self.last_prices[symbol])
Was this helpful? React with 👍 or 👎 to provide feedback.
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 6a5bb99...e8a4ecc
Analysis
-
Precision Issues (CRITICAL): Using Python
floatfor financial calculations will accumulate precision errors over time. Must switch toDecimaltype for all monetary values before production use. -
Concurrency & Thread Safety (CRITICAL): The engine maintains mutable shared state without any synchronization mechanisms. This will lead to race conditions and data corruption in concurrent environments. Needs locks, actor model, or immutable approach.
-
Memory Management Problems: Unbounded memory growth as positions are never removed and price history grows indefinitely. Implement cleanup for closed positions and stale data.
-
Persistence & Resilience Gaps: No state persistence or crash recovery means all positions and PnL are lost on restart. Missing event sourcing for audit trails and replay capability.
-
Error Handling Weaknesses: Silent failures instead of fail-fast approach, with KeyError risks when prices are missing for unrealized PnL calculation. Needs explicit error handling with proper exceptions.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
13 files reviewed | 4 comments | Edit Agent Settings • Read Docs
|
|
||
| def __init__(self, account_id: str = "system"): | ||
| self.account_id = account_id | ||
| self.positions: dict[str, Position] = {} |
There was a problem hiding this comment.
This PnL engine lacks thread safety mechanisms, making it unsafe for concurrent access in real-time trading systems where multiple threads may process trades simultaneously. Consider adding thread synchronization:
from threading import RLock
class RealTimePnLEngine:
def __init__(self, account_id: str = "system"):
self._lock = RLock()
# ...
def on_trade(self, trade: Trade) -> None:
with self._lock:
# existing logicWithout this, race conditions can corrupt position state and PnL calculations.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: inditilve/trading-primitives#3
File: core/pnl_engine.py#L14
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
This PnL engine lacks thread safety mechanisms, making it unsafe for concurrent access in real-time trading systems where multiple threads may process trades simultaneously. Consider adding thread synchronization:
```python
from threading import RLock
class RealTimePnLEngine:
def __init__(self, account_id: str = "system"):
self._lock = RLock()
# ...
def on_trade(self, trade: Trade) -> None:
with self._lock:
# existing logic
Without this, race conditions can corrupt position state and PnL calculations.
</details>
| symbol: str | ||
| side: Side | ||
| qty: int # Always positive; side determines direction | ||
| price: float |
There was a problem hiding this comment.
Using float for financial calculations introduces precision errors that compound over time. For a PnL engine handling real money, this can lead to significant discrepancies. Consider using Decimal from the decimal module instead:
from decimal import Decimal
@dataclass
class Trade:
price: Decimal
# ...This is critical for production financial systems where rounding errors of even fractions of a cent can accumulate to material amounts.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: inditilve/trading-primitives#3
File: models/domain.py#L25
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Using `float` for financial calculations introduces precision errors that compound over time. For a PnL engine handling real money, this can lead to significant discrepancies. Consider using `Decimal` from the `decimal` module instead:
```python
from decimal import Decimal
@dataclass
class Trade:
price: Decimal
# ...
This is critical for production financial systems where rounding errors of even fractions of a cent can accumulate to material amounts.
</details>
|
|
||
| def __init__(self, account_id: str = "system"): | ||
| self.account_id = account_id | ||
| self.positions: dict[str, Position] = {} |
There was a problem hiding this comment.
The positions dict will grow unbounded as symbols are traded, even after positions are fully closed (qty=0). This creates a memory leak over time. Consider implementing a cleanup mechanism:
def _cleanup_closed_positions(self) -> None:
"""Remove positions with zero qty to prevent memory bloat"""
self.positions = {sym: pos for sym, pos in self.positions.items() if pos.qty != 0}Call this periodically or after position closes. For a long-running system, this can become significant.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: inditilve/trading-primitives#3
File: core/pnl_engine.py#L14
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The `positions` dict will grow unbounded as symbols are traded, even after positions are fully closed (qty=0). This creates a memory leak over time. Consider implementing a cleanup mechanism:
```python
def _cleanup_closed_positions(self) -> None:
"""Remove positions with zero qty to prevent memory bloat"""
self.positions = {sym: pos for sym, pos in self.positions.items() if pos.qty != 0}
Call this periodically or after position closes. For a long-running system, this can become significant.
</details>
| """Update position and realized PnL""" | ||
|
|
||
| # Validate trade | ||
| if trade.qty <= 0: |
There was a problem hiding this comment.
The validation silently ignores invalid trades with only a warning. For a financial system, invalid trades should either raise an exception or return a validation result so the caller can handle the error appropriately. Consider:
def on_trade(self, trade: Trade) -> bool:
"""Update position and realized PnL. Returns True if trade was processed."""
if trade.qty <= 0:
raise ValueError(f"Invalid trade {trade.trade_id}: qty must be positive")
if trade.price < 0:
raise ValueError(f"Invalid trade {trade.trade_id}: price cannot be negative")
# ... rest of logic
return TrueThis allows upstream systems to detect and handle validation failures.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: inditilve/trading-primitives#3
File: core/pnl_engine.py#L23
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The validation silently ignores invalid trades with only a warning. For a financial system, invalid trades should either raise an exception or return a validation result so the caller can handle the error appropriately. Consider:
```python
def on_trade(self, trade: Trade) -> bool:
"""Update position and realized PnL. Returns True if trade was processed."""
if trade.qty <= 0:
raise ValueError(f"Invalid trade {trade.trade_id}: qty must be positive")
if trade.price < 0:
raise ValueError(f"Invalid trade {trade.trade_id}: price cannot be negative")
# ... rest of logic
return True
This allows upstream systems to detect and handle validation failures.
</details>
1 participant
Basic PnL Engine
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.