Skip to content

Commit 3da0989

Browse files
MDUYNMDUYN
committed
fix: correct portfolio total_net_gain accumulation understating gains by ~3x (#397)
Bug 1: In create_order_metadata_with_trade_context, the cost variable accumulated across loop iterations but the entire accumulated value was subtracted from each trade's revenue. With N trades, earlier costs were subtracted N times instead of once, systematically understating net_gain. Fix: use per-iteration trade_cost variable. Bug 2: In _create_trade_metadata_with_sell_order_and_trades, net_gain used sell_amount (total sell order) instead of trade_data['amount'] (per-trade portion), overcounting gain per trade when a sell order was split across multiple trades. Fix: use trade_data['amount']. Added 3 regression tests in tests/services/test_trade_service_net_gain.py.
1 parent cbfd597 commit 3da0989

File tree

2 files changed

+243
-3
lines changed

2 files changed

+243
-3
lines changed

‎investing_algorithm_framework/services/trade_service/trade_service.py‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,8 @@ def _create_trade_metadata_with_sell_order_and_trades(
319319
self.repository.add_order_to_trade(trade, sell_order)
320320

321321
# Update the trade
322-
net_gain = (sell_price * sell_amount) - open_price * sell_amount
322+
net_gain = (sell_price * trade_data["amount"]) \
323+
- open_price * trade_data["amount"]
323324
available_amount = available_amount - trade_data["amount"]
324325
trade_updated_data = {
325326
"available_amount": available_amount,
@@ -434,8 +435,9 @@ def create_order_metadata_with_trade_context(
434435
for metadata in order_metadatas:
435436
if metadata.trade_id is not None:
436437
trade = self.get(metadata.trade_id)
437-
cost += trade.open_price * metadata.amount
438-
net_gain += (sell_price * metadata.amount) - cost
438+
trade_cost = trade.open_price * metadata.amount
439+
cost += trade_cost
440+
net_gain += (sell_price * metadata.amount) - trade_cost
439441

440442
position.cost -= cost
441443
self.position_repository.save(position)

‎tests/services/test_trade_service_net_gain.py‎

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
from investing_algorithm_framework import PortfolioConfiguration, \
2+
MarketCredential, OrderStatus, TradeStatus
3+
from tests.resources import TestBase
4+
5+
6+
class TestTradeServiceNetGain(TestBase):
7+
"""
8+
Regression tests for portfolio net_gain accumulation bug (#397).
9+
10+
Bug 1 (PRIMARY): In create_order_metadata_with_trade_context, the
11+
``cost`` variable accumulated across loop iterations and was
12+
subtracted fully on each iteration, understating
13+
portfolio.total_net_gain when a sell order closed multiple trades.
14+
15+
Bug 2 (SECONDARY): In _create_trade_metadata_with_sell_order_and_trades,
16+
``sell_amount`` (total order amount) was used instead of
17+
``trade_data["amount"]`` (per-trade portion), overstating individual
18+
trade net_gain values.
19+
"""
20+
storage_repo_type = "pandas"
21+
market_credentials = [
22+
MarketCredential(
23+
market="binance",
24+
api_key="api_key",
25+
secret_key="secret_key",
26+
)
27+
]
28+
portfolio_configurations = [
29+
PortfolioConfiguration(
30+
market="binance",
31+
trading_symbol="EUR"
32+
)
33+
]
34+
external_balances = {"EUR": 100000}
35+
36+
# ------------------------------------------------------------------
37+
# helpers
38+
# ------------------------------------------------------------------
39+
40+
def _create_filled_buy_order(
41+
self, target_symbol, amount, price, portfolio_id
42+
):
43+
"""Create a BUY order and immediately fill it, producing an
44+
OPEN trade."""
45+
order_service = self.app.container.order_service()
46+
order = order_service.create({
47+
"target_symbol": target_symbol,
48+
"trading_symbol": "EUR",
49+
"amount": amount,
50+
"order_side": "BUY",
51+
"price": price,
52+
"order_type": "LIMIT",
53+
"portfolio_id": portfolio_id,
54+
})
55+
order_service.update(order.id, {
56+
"status": OrderStatus.CLOSED.value,
57+
"filled": amount,
58+
"remaining": 0,
59+
})
60+
return order
61+
62+
# ------------------------------------------------------------------
63+
# tests
64+
# ------------------------------------------------------------------
65+
66+
def test_net_gain_correct_single_trade(self):
67+
"""Baseline: sell order that closes ONE trade.
68+
69+
net_gain = (sell_price * amount) - (open_price * amount).
70+
"""
71+
portfolio = self.app.context.get_portfolio()
72+
portfolio_id = portfolio.id
73+
trade_service = self.app.container.trade_service()
74+
order_service = self.app.container.order_service()
75+
76+
# Buy 100 ADA at 10 EUR
77+
buy = self._create_filled_buy_order("ADA", 100, 10, portfolio_id)
78+
trade = trade_service.find({"order_id": buy.id})
79+
self.assertEqual(TradeStatus.OPEN.value, trade.status)
80+
self.assertEqual(100, trade.available_amount)
81+
82+
# Sell 100 ADA at 15 EUR
83+
order_service.create({
84+
"target_symbol": "ADA",
85+
"trading_symbol": "EUR",
86+
"amount": 100,
87+
"order_side": "SELL",
88+
"price": 15,
89+
"order_type": "LIMIT",
90+
"portfolio_id": portfolio_id,
91+
})
92+
93+
# Verify trade net_gain: (15 * 100) - (10 * 100) = 500
94+
trade = trade_service.get(trade.id)
95+
self.assertAlmostEqual(500, trade.net_gain)
96+
97+
# Verify portfolio total_net_gain
98+
portfolio = self.app.container.portfolio_repository() \
99+
.get(portfolio_id)
100+
self.assertAlmostEqual(500, portfolio.total_net_gain)
101+
102+
def test_net_gain_correct_multiple_trades(self):
103+
"""Regression for bug #1 — cost accumulation.
104+
105+
Two buy orders at different prices, then one sell order that
106+
closes both. The old code accumulated ``cost`` across loop
107+
iterations, causing portfolio.total_net_gain = -200 instead of
108+
the correct 800.
109+
110+
trade1: buy 100 @ 10, trade2: buy 100 @ 12, sell 200 @ 15.
111+
Expected: total_net_gain = (15-10)*100 + (15-12)*100 = 800.
112+
"""
113+
portfolio = self.app.context.get_portfolio()
114+
portfolio_id = portfolio.id
115+
trade_service = self.app.container.trade_service()
116+
order_service = self.app.container.order_service()
117+
118+
# Trade 1: buy 100 ADA at 10
119+
buy1 = self._create_filled_buy_order("ADA", 100, 10, portfolio_id)
120+
trade1 = trade_service.find({"order_id": buy1.id})
121+
122+
# Trade 2: buy 100 ADA at 12
123+
buy2 = self._create_filled_buy_order("ADA", 100, 12, portfolio_id)
124+
trade2 = trade_service.find({"order_id": buy2.id})
125+
126+
self.assertEqual(TradeStatus.OPEN.value, trade1.status)
127+
self.assertEqual(TradeStatus.OPEN.value, trade2.status)
128+
129+
# Sell 200 ADA at 15 — closes both trades
130+
order_service.create({
131+
"target_symbol": "ADA",
132+
"trading_symbol": "EUR",
133+
"amount": 200,
134+
"order_side": "SELL",
135+
"price": 15,
136+
"order_type": "LIMIT",
137+
"portfolio_id": portfolio_id,
138+
})
139+
140+
# Verify individual trade net_gains
141+
trade1 = trade_service.get(trade1.id)
142+
trade2 = trade_service.get(trade2.id)
143+
expected_gain_1 = (15 * 100) - (10 * 100) # 500
144+
expected_gain_2 = (15 * 100) - (12 * 100) # 300
145+
146+
self.assertAlmostEqual(expected_gain_1, trade1.net_gain)
147+
self.assertAlmostEqual(expected_gain_2, trade2.net_gain)
148+
149+
# Verify portfolio total_net_gain — the key regression check
150+
portfolio = self.app.container.portfolio_repository() \
151+
.get(portfolio_id)
152+
expected_total = expected_gain_1 + expected_gain_2 # 800
153+
self.assertAlmostEqual(
154+
expected_total,
155+
portfolio.total_net_gain,
156+
msg=(
157+
f"portfolio.total_net_gain should be {expected_total}, "
158+
f"got {portfolio.total_net_gain}. "
159+
"Bug #397: cost accumulation gave -200 instead of 800."
160+
),
161+
)
162+
163+
def test_net_gain_correct_with_explicit_trades_list(self):
164+
"""Regression for bug #2 — sell_amount vs per-trade amount.
165+
166+
When a sell order is created with an explicit ``trades`` list
167+
(e.g. from stop-loss / take-profit), the per-trade net_gain must
168+
use ``trade_data["amount"]``, not ``sell_amount``.
169+
170+
trade1: buy 100 @ 10, trade2: buy 50 @ 12, sell 150 @ 15.
171+
With the bug each trade's net_gain was computed against
172+
sell_amount=150 instead of per-trade amounts 100/50.
173+
"""
174+
portfolio = self.app.context.get_portfolio()
175+
portfolio_id = portfolio.id
176+
trade_service = self.app.container.trade_service()
177+
order_service = self.app.container.order_service()
178+
179+
# Trade 1: buy 100 ADA at 10
180+
buy1 = self._create_filled_buy_order("ADA", 100, 10, portfolio_id)
181+
trade1 = trade_service.find({"order_id": buy1.id})
182+
183+
# Trade 2: buy 50 ADA at 12
184+
buy2 = self._create_filled_buy_order("ADA", 50, 12, portfolio_id)
185+
trade2 = trade_service.find({"order_id": buy2.id})
186+
187+
# Sell 150 ADA at 15 with explicit trades in data
188+
order_service.create({
189+
"target_symbol": "ADA",
190+
"trading_symbol": "EUR",
191+
"amount": 150,
192+
"order_side": "SELL",
193+
"price": 15,
194+
"order_type": "LIMIT",
195+
"portfolio_id": portfolio_id,
196+
"trades": [
197+
{"trade_id": trade1.id, "amount": 100},
198+
{"trade_id": trade2.id, "amount": 50},
199+
],
200+
})
201+
202+
# Verify individual trade net_gains use per-trade amounts
203+
trade1 = trade_service.get(trade1.id)
204+
trade2 = trade_service.get(trade2.id)
205+
expected_gain_1 = (15 * 100) - (10 * 100) # 500
206+
expected_gain_2 = (15 * 50) - (12 * 50) # 150
207+
208+
self.assertAlmostEqual(
209+
expected_gain_1,
210+
trade1.net_gain,
211+
msg=(
212+
f"trade1.net_gain should be {expected_gain_1}, "
213+
f"got {trade1.net_gain}. "
214+
"Bug #397: sell_amount used instead of per-trade amount."
215+
),
216+
)
217+
self.assertAlmostEqual(
218+
expected_gain_2,
219+
trade2.net_gain,
220+
msg=(
221+
f"trade2.net_gain should be {expected_gain_2}, "
222+
f"got {trade2.net_gain}. "
223+
"Bug #397: sell_amount used instead of per-trade amount."
224+
),
225+
)
226+
227+
# Verify portfolio total_net_gain (also exercises bug #1 path)
228+
portfolio = self.app.container.portfolio_repository() \
229+
.get(portfolio_id)
230+
expected_total = expected_gain_1 + expected_gain_2 # 650
231+
self.assertAlmostEqual(
232+
expected_total,
233+
portfolio.total_net_gain,
234+
msg=(
235+
f"portfolio.total_net_gain should be {expected_total}, "
236+
f"got {portfolio.total_net_gain}."
237+
),
238+
)

0 commit comments

Comments
 (0)