diff --git a/investing_algorithm_framework/services/trade_service/trade_service.py b/investing_algorithm_framework/services/trade_service/trade_service.py index 8a329476..509f344d 100644 --- a/investing_algorithm_framework/services/trade_service/trade_service.py +++ b/investing_algorithm_framework/services/trade_service/trade_service.py @@ -295,7 +295,6 @@ def _create_trade_metadata_with_sell_order_and_trades( """ sell_order_id = sell_order.id updated_at = sell_order.updated_at - sell_amount = sell_order.amount sell_price = sell_order.price for trade_data in trades: @@ -319,7 +318,8 @@ def _create_trade_metadata_with_sell_order_and_trades( self.repository.add_order_to_trade(trade, sell_order) # Update the trade - net_gain = (sell_price * sell_amount) - open_price * sell_amount + net_gain = (sell_price * trade_data["amount"]) \ + - open_price * trade_data["amount"] available_amount = available_amount - trade_data["amount"] trade_updated_data = { "available_amount": available_amount, @@ -434,8 +434,9 @@ def create_order_metadata_with_trade_context( for metadata in order_metadatas: if metadata.trade_id is not None: trade = self.get(metadata.trade_id) - cost += trade.open_price * metadata.amount - net_gain += (sell_price * metadata.amount) - cost + trade_cost = trade.open_price * metadata.amount + cost += trade_cost + net_gain += (sell_price * metadata.amount) - trade_cost position.cost -= cost self.position_repository.save(position) diff --git a/tests/services/test_trade_service_net_gain.py b/tests/services/test_trade_service_net_gain.py new file mode 100644 index 00000000..d3466481 --- /dev/null +++ b/tests/services/test_trade_service_net_gain.py @@ -0,0 +1,238 @@ +from investing_algorithm_framework import PortfolioConfiguration, \ + MarketCredential, OrderStatus, TradeStatus +from tests.resources import TestBase + + +class TestTradeServiceNetGain(TestBase): + """ + Regression tests for portfolio net_gain accumulation bug (#397). + + Bug 1 (PRIMARY): In create_order_metadata_with_trade_context, the + ``cost`` variable accumulated across loop iterations and was + subtracted fully on each iteration, understating + portfolio.total_net_gain when a sell order closed multiple trades. + + Bug 2 (SECONDARY): In _create_trade_metadata_with_sell_order_and_trades, + ``sell_amount`` (total order amount) was used instead of + ``trade_data["amount"]`` (per-trade portion), overstating individual + trade net_gain values. + """ + storage_repo_type = "pandas" + market_credentials = [ + MarketCredential( + market="binance", + api_key="api_key", + secret_key="secret_key", + ) + ] + portfolio_configurations = [ + PortfolioConfiguration( + market="binance", + trading_symbol="EUR" + ) + ] + external_balances = {"EUR": 100000} + + # ------------------------------------------------------------------ + # helpers + # ------------------------------------------------------------------ + + def _create_filled_buy_order( + self, target_symbol, amount, price, portfolio_id + ): + """Create a BUY order and immediately fill it, producing an + OPEN trade.""" + order_service = self.app.container.order_service() + order = order_service.create({ + "target_symbol": target_symbol, + "trading_symbol": "EUR", + "amount": amount, + "order_side": "BUY", + "price": price, + "order_type": "LIMIT", + "portfolio_id": portfolio_id, + }) + order_service.update(order.id, { + "status": OrderStatus.CLOSED.value, + "filled": amount, + "remaining": 0, + }) + return order + + # ------------------------------------------------------------------ + # tests + # ------------------------------------------------------------------ + + def test_net_gain_correct_single_trade(self): + """Baseline: sell order that closes ONE trade. + + net_gain = (sell_price * amount) - (open_price * amount). + """ + portfolio = self.app.context.get_portfolio() + portfolio_id = portfolio.id + trade_service = self.app.container.trade_service() + order_service = self.app.container.order_service() + + # Buy 100 ADA at 10 EUR + buy = self._create_filled_buy_order("ADA", 100, 10, portfolio_id) + trade = trade_service.find({"order_id": buy.id}) + self.assertEqual(TradeStatus.OPEN.value, trade.status) + self.assertEqual(100, trade.available_amount) + + # Sell 100 ADA at 15 EUR + order_service.create({ + "target_symbol": "ADA", + "trading_symbol": "EUR", + "amount": 100, + "order_side": "SELL", + "price": 15, + "order_type": "LIMIT", + "portfolio_id": portfolio_id, + }) + + # Verify trade net_gain: (15 * 100) - (10 * 100) = 500 + trade = trade_service.get(trade.id) + self.assertAlmostEqual(500, trade.net_gain) + + # Verify portfolio total_net_gain + portfolio = self.app.container.portfolio_repository() \ + .get(portfolio_id) + self.assertAlmostEqual(500, portfolio.total_net_gain) + + def test_net_gain_correct_multiple_trades(self): + """Regression for bug #1 — cost accumulation. + + Two buy orders at different prices, then one sell order that + closes both. The old code accumulated ``cost`` across loop + iterations, causing portfolio.total_net_gain = -200 instead of + the correct 800. + + trade1: buy 100 @ 10, trade2: buy 100 @ 12, sell 200 @ 15. + Expected: total_net_gain = (15-10)*100 + (15-12)*100 = 800. + """ + portfolio = self.app.context.get_portfolio() + portfolio_id = portfolio.id + trade_service = self.app.container.trade_service() + order_service = self.app.container.order_service() + + # Trade 1: buy 100 ADA at 10 + buy1 = self._create_filled_buy_order("ADA", 100, 10, portfolio_id) + trade1 = trade_service.find({"order_id": buy1.id}) + + # Trade 2: buy 100 ADA at 12 + buy2 = self._create_filled_buy_order("ADA", 100, 12, portfolio_id) + trade2 = trade_service.find({"order_id": buy2.id}) + + self.assertEqual(TradeStatus.OPEN.value, trade1.status) + self.assertEqual(TradeStatus.OPEN.value, trade2.status) + + # Sell 200 ADA at 15 — closes both trades + order_service.create({ + "target_symbol": "ADA", + "trading_symbol": "EUR", + "amount": 200, + "order_side": "SELL", + "price": 15, + "order_type": "LIMIT", + "portfolio_id": portfolio_id, + }) + + # Verify individual trade net_gains + trade1 = trade_service.get(trade1.id) + trade2 = trade_service.get(trade2.id) + expected_gain_1 = (15 * 100) - (10 * 100) # 500 + expected_gain_2 = (15 * 100) - (12 * 100) # 300 + + self.assertAlmostEqual(expected_gain_1, trade1.net_gain) + self.assertAlmostEqual(expected_gain_2, trade2.net_gain) + + # Verify portfolio total_net_gain — the key regression check + portfolio = self.app.container.portfolio_repository() \ + .get(portfolio_id) + expected_total = expected_gain_1 + expected_gain_2 # 800 + self.assertAlmostEqual( + expected_total, + portfolio.total_net_gain, + msg=( + f"portfolio.total_net_gain should be {expected_total}, " + f"got {portfolio.total_net_gain}. " + "Bug #397: cost accumulation gave -200 instead of 800." + ), + ) + + def test_net_gain_correct_with_explicit_trades_list(self): + """Regression for bug #2 — sell_amount vs per-trade amount. + + When a sell order is created with an explicit ``trades`` list + (e.g. from stop-loss / take-profit), the per-trade net_gain must + use ``trade_data["amount"]``, not ``sell_amount``. + + trade1: buy 100 @ 10, trade2: buy 50 @ 12, sell 150 @ 15. + With the bug each trade's net_gain was computed against + sell_amount=150 instead of per-trade amounts 100/50. + """ + portfolio = self.app.context.get_portfolio() + portfolio_id = portfolio.id + trade_service = self.app.container.trade_service() + order_service = self.app.container.order_service() + + # Trade 1: buy 100 ADA at 10 + buy1 = self._create_filled_buy_order("ADA", 100, 10, portfolio_id) + trade1 = trade_service.find({"order_id": buy1.id}) + + # Trade 2: buy 50 ADA at 12 + buy2 = self._create_filled_buy_order("ADA", 50, 12, portfolio_id) + trade2 = trade_service.find({"order_id": buy2.id}) + + # Sell 150 ADA at 15 with explicit trades in data + order_service.create({ + "target_symbol": "ADA", + "trading_symbol": "EUR", + "amount": 150, + "order_side": "SELL", + "price": 15, + "order_type": "LIMIT", + "portfolio_id": portfolio_id, + "trades": [ + {"trade_id": trade1.id, "amount": 100}, + {"trade_id": trade2.id, "amount": 50}, + ], + }) + + # Verify individual trade net_gains use per-trade amounts + trade1 = trade_service.get(trade1.id) + trade2 = trade_service.get(trade2.id) + expected_gain_1 = (15 * 100) - (10 * 100) # 500 + expected_gain_2 = (15 * 50) - (12 * 50) # 150 + + self.assertAlmostEqual( + expected_gain_1, + trade1.net_gain, + msg=( + f"trade1.net_gain should be {expected_gain_1}, " + f"got {trade1.net_gain}. " + "Bug #397: sell_amount used instead of per-trade amount." + ), + ) + self.assertAlmostEqual( + expected_gain_2, + trade2.net_gain, + msg=( + f"trade2.net_gain should be {expected_gain_2}, " + f"got {trade2.net_gain}. " + "Bug #397: sell_amount used instead of per-trade amount." + ), + ) + + # Verify portfolio total_net_gain (also exercises bug #1 path) + portfolio = self.app.container.portfolio_repository() \ + .get(portfolio_id) + expected_total = expected_gain_1 + expected_gain_2 # 650 + self.assertAlmostEqual( + expected_total, + portfolio.total_net_gain, + msg=( + f"portfolio.total_net_gain should be {expected_total}, " + f"got {portfolio.total_net_gain}." + ), + )