Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e681b1f
trade and position model classes
inditilve Jan 14, 2026
cb0932e
add basic pnl-engine, test cases, working pytest fixtures
inditilve Jan 14, 2026
50862bb
Add more tests, fix bugs, refactor helper methods for on_trade
inditilve Jan 15, 2026
251e5e6
Enhance unit tests for RealTimePnLEngine with additional coverage; Up…
inditilve Jan 15, 2026
270902f
add helper methods and corresponding tests
inditilve Jan 16, 2026
47b3728
added ruff, mypy, conda and project run configs, pre-commit hooks, an…
inditilve Jan 19, 2026
318d587
update upload-artifact action to v4
inditilve Jan 19, 2026
c5cd8ff
add setuptools config
inditilve Jan 19, 2026
09f5db7
remove requirements.txt
inditilve Jan 19, 2026
d6af5d0
update ci/project to use requirements.txt instead of pyproject.toml d…
inditilve Jan 19, 2026
28c472f
fix dupe install step; split ci into build and test
inditilve Jan 19, 2026
0f4a26b
test job needs build-and-lint to succeed before running
inditilve Jan 19, 2026
8e1232e
merge the two ci jobs into one to not install dependencies/python twice
inditilve Jan 19, 2026
3a1460d
change action name
inditilve Jan 19, 2026
63af7fc
Apply suggestions from CodeRabbit code review
inditilve Jan 19, 2026
f7ac722
fix test using price instead of qty for wavg cost; remove dead code; …
inditilve Jan 19, 2026
3a67701
Merge branch 'feat/pnl-engine' of github.com:inditilve/trading-primit…
inditilve Jan 19, 2026
e8a4ecc
Refactor PnL engine and update trade model to use side enum instead o…
inditilve Jan 19, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
name: Build, Lint, and Run Tests

on:
pull_request:
branches: [master]

jobs:
build-lint-test:
runs-on: ubuntu-latest
permissions:
checks: write
pull-requests: write

steps:
- uses: actions/checkout@v4

- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: '3.12'
cache: 'pip'

- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt

- name: Lint with ruff check
run: ruff check .

- name: Format check with ruff
run: ruff format . --check

- name: Type check with mypy
run: mypy . --ignore-missing-imports

- name: Run tests with pytest
run: pytest tests/ --junitxml=junit/test-results.xml --html=htmlcov/report.html --self-contained-html

- name: Upload test results artifact
if: always()
uses: actions/upload-artifact@v4
with:
name: test-results
path: junit/test-results.xml

- name: Upload HTML report artifact
if: always()
uses: actions/upload-artifact@v4
with:
name: test-report
path: htmlcov/report.html

- name: Publish test results to PR
if: always()
uses: EnricoMi/publish-unit-test-result-action@v2
with:
files: junit/test-results.xml
check_name: Test Results
comment_mode: always
40 changes: 36 additions & 4 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# Python
__pycache__/
*.py[cod]
*$py.class
Expand All @@ -15,11 +16,42 @@ parts/
sdist/
var/
wheels/
*.egg-info/
.installed.cfg
*.egg

# Testing
.pytest_cache/
.coverage
htmlcov/
junit/
.tox/

# Type checking
.mypy_cache/
.dmypy.json
dmypy.json
.ruff_cache/

# Pre-commit
.pre-commit/

# IDE
.vscode/
.idea/
*.swp
*.swo
*~

# Environment
.env
.venv
env/
venv/
.vscode/
.idea/
conda-env/

# Conda
.conda/
*.db
.DS_Store

# Dev Notes
notes/
15 changes: 15 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
repos:
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.14.13
hooks:
- id: ruff-check
args: [ --fix ]
- id: ruff-format

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.7.1
hooks:
- id: mypy
args: [--ignore-missing-imports]
pass_filenames: true
stages: [commit]
1 change: 1 addition & 0 deletions core/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
"""Core trading modules."""
182 changes: 182 additions & 0 deletions core/pnl_engine.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
import logging
from collections import defaultdict

from models import Position, Trade

logger = logging.getLogger(__name__)


class RealTimePnLEngine:
"""Real-time PnL attribution with realized/unrealized split"""

def __init__(self, account_id: str = "system"):
self.account_id = account_id
self.positions: dict[str, Position] = {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High

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 logic

Without this, race conditions can corrupt position state and PnL calculations.

Agent: 🏛 Architecture • Fix in Cursor • Fix in Claude

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>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium

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 • Fix in Cursor • Fix in Claude

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>

self.realized_pnl: dict[str, float] = defaultdict(float)
self.last_prices: dict[str, float] = defaultdict(float)
logger.debug(f"RealTimePnLEngine initialized for account {account_id}")

def on_trade(self, trade: Trade) -> None:
"""Update position and realized PnL"""

# Validate trade
if trade.qty <= 0:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium

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 True

This allows upstream systems to detect and handle validation failures.

Agent: 🏛 Architecture • Fix in Cursor • Fix in Claude

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>

logger.warning(f"Ignoring trade {trade.trade_id} with qty={trade.qty}")
return

if trade.price < 0:
logger.warning(f"Ignoring trade {trade.trade_id} with negative price")
return

sym = trade.symbol
signed_qty = trade.signed_qty()
trade_price = trade.price

logger.info(
f"Trade {trade.trade_id}: {sym} {trade.side.value} qty={trade.qty} "
f"price={trade_price:.2f} notional={trade.notional_value():.2f}"
)

if sym not in self.positions:
self.positions[sym] = Position(account_id=self.account_id, symbol=sym)

pos = self.positions[sym]
prev_qty = pos.qty

# Determine if trade is same direction or opposite
if prev_qty * signed_qty >= 0:
self._handle_same_direction_trade(pos, signed_qty, trade_price)
else:
self._handle_opposite_direction_trade(pos, signed_qty, trade_price)

pos.qty += signed_qty
logger.debug(f"{sym}: Position updated to qty={pos.qty}")

def _handle_same_direction_trade(
self, pos: Position, signed_qty: int, trade_price: float
) -> None:
"""Handle weighted average cost updates for same-direction trades"""
sym = pos.symbol
prev_qty = pos.qty
prev_cost = pos.avg_cost
new_qty = prev_qty + signed_qty

if new_qty != 0:
pos.avg_cost = (prev_qty * prev_cost + signed_qty * trade_price) / new_qty
logger.debug(f"{sym}: avg_cost updated to {pos.avg_cost:.2f}")

def _handle_opposite_direction_trade(
self, pos: Position, signed_qty: int, trade_price: float
) -> None:
"""Handle opposite-direction trade: realize PnL and update position"""
sym = pos.symbol
prev_qty = pos.qty
prev_cost = pos.avg_cost
closing_qty = min(abs(prev_qty), abs(signed_qty))

self._realize_pnl(sym, closing_qty, trade_price, prev_cost, prev_qty)

# After closing, check remaining qty
new_qty = prev_qty + signed_qty

if new_qty == 0:
pos.avg_cost = 0.0
logger.debug(f"{sym}: Position fully closed")
elif prev_qty * new_qty > 0:
# Partial close, same direction continues
logger.debug(f"{sym}: Partial close, avg_cost remains {prev_cost:.2f}")
else:
# Direction flipped: open new position at trade price
pos.avg_cost = trade_price
logger.debug(
f"{sym}: Direction flipped, new position opened at {trade_price:.2f} "
f"(qty={new_qty})"
)

def _realize_pnl(
self,
symbol: str,
closing_qty: int,
trade_price: float,
prev_cost: float,
prev_qty: int,
) -> None:
"""Calculate and log realized PnL"""
sign = 1 if prev_qty > 0 else -1
pnl_increment = closing_qty * (trade_price - prev_cost) * sign
self.realized_pnl[symbol] += pnl_increment
logger.info(
f"{symbol}: Realized PnL {self.realized_pnl[symbol]:.2f} "
f"(closed {closing_qty} @ {trade_price:.2f} vs cost {prev_cost:.2f})"
)

def on_price(self, symbol: str, price: float) -> None:
"""Update latest market price"""
self.last_prices[symbol] = price

def get_position(self, symbol: str) -> Position:
"""Get position for symbol, or empty position if none exists"""
return self.positions.get(symbol, Position(account_id=self.account_id, symbol=symbol))

def get_unrealized_pnl(self, symbol: str) -> float:
"""Compute unrealized PnL for symbol"""
pos = self.get_position(symbol)
if pos.qty == 0:
return 0.0
last_px = self.last_prices[symbol]
return pos.unrealized_pnl(last_px)
Comment thread
inditilve marked this conversation as resolved.
Comment on lines +126 to +127

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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

  1. A trade is processed via on_trade(), creating a position
  2. get_unrealized_pnl() is called before on_price() sets the market price
  3. At line 126, last_px = self.last_prices[symbol] returns 0.0 (defaultdict default)
  4. 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 error

Impact

  • get_unrealized_pnl() returns incorrect values
  • get_total_pnl() at line 131 returns incorrect totals
  • get_pnl_by_symbol() at line 149 returns incorrect unrealized/total
  • log_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])
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


def get_total_pnl(self) -> float:
"""Total realized + unrealized PnL across all symbols"""
return sum(self.realized_pnl[sym] + self.get_unrealized_pnl(sym) for sym in self.positions)

def get_long_positions(self) -> dict[str, Position]:
"""Get all long positions"""
return {sym: pos for sym, pos in self.positions.items() if pos.is_long()}

def get_short_positions(self) -> dict[str, Position]:
"""Get all short positions"""
return {sym: pos for sym, pos in self.positions.items() if pos.is_short()}

def get_open_positions(self) -> dict[str, Position]:
"""Get all open positions (non-zero qty)"""
return {sym: pos for sym, pos in self.positions.items() if pos.is_open()}

def get_pnl_by_symbol(self, symbol: str) -> dict[str, float]:
"""Get both realized and unrealized PnL for a symbol"""
return {
"realized": self.realized_pnl[symbol],
"unrealized": self.get_unrealized_pnl(symbol),
"total": self.realized_pnl[symbol] + self.get_unrealized_pnl(symbol),
}

def get_total_notional(self, last_prices: dict[str, float]) -> float:
"""Total notional value across all positions"""
return sum(
pos.notional_value(last_prices.get(pos.symbol, 0.0)) for pos in self.positions.values()
)

def log_summary(self, symbol: str | None = None) -> str:
"""Log engine summary for debugging"""
if symbol:
pos = self.get_position(symbol)
realized = self.realized_pnl[symbol]
unrealized = self.get_unrealized_pnl(symbol)
summary = (
f"{symbol}: qty={pos.qty}, avg_cost={pos.avg_cost:.2f}, "
f"realized={realized:.2f}, unrealized={unrealized:.2f}, "
f"total={realized + unrealized:.2f}"
)
else:
# All symbols
positions_count = len(self.positions)
total_realized = sum(self.realized_pnl.values())
total_unrealized = sum(self.get_unrealized_pnl(sym) for sym in self.positions)
summary = (
f"Account {self.account_id}: {positions_count} symbols, "
f"realized={total_realized:.2f}, unrealized={total_unrealized:.2f}, "
f"total={self.get_total_pnl():.2f}"
)

logger.info(summary)
return summary
9 changes: 9 additions & 0 deletions environment.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name: trading-primitives
channels:
- conda-forge
- defaults
dependencies:
- python=3.12
- pip
- pip:
- -r requirements.txt
5 changes: 5 additions & 0 deletions models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
"""Models package: domain models and API schemas."""

from models.domain import Position, Side, Trade

__all__ = ["Position", "Side", "Trade"]
Loading