From ef585a6e133e9ec4bebef80c86437df2e76da99b Mon Sep 17 00:00:00 2001 From: Sam Germain Date: Fri, 2 Jul 2021 03:01:33 -0600 Subject: [PATCH 1/2] Wrote all tests for shorting --- tests/conftest.py | 25 +++++++- tests/conftest_trades.py | 8 +-- tests/test_persistence.py | 2 +- tests/test_persistence_short.py | 105 +++++++------------------------- 4 files changed, 52 insertions(+), 88 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index a19dbac2b..256204263 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -204,7 +204,30 @@ def create_mock_trades(fee, use_db: bool = True): add_trade(trade) trade = mock_trade_6(fee) add_trade(trade) - # TODO: margin trades + + +def create_mock_trades_with_leverage(fee, use_db: bool = True): + """ + Create some fake trades ... + """ + def add_trade(trade): + if use_db: + Trade.query.session.add(trade) + else: + LocalTrade.add_bt_trade(trade) + # Simulate dry_run entries + trade = mock_trade_1(fee) + add_trade(trade) + trade = mock_trade_2(fee) + add_trade(trade) + trade = mock_trade_3(fee) + add_trade(trade) + trade = mock_trade_4(fee) + add_trade(trade) + trade = mock_trade_5(fee) + add_trade(trade) + trade = mock_trade_6(fee) + add_trade(trade) trade = short_trade(fee) add_trade(trade) trade = leverage_trade(fee) diff --git a/tests/conftest_trades.py b/tests/conftest_trades.py index 9bf334b48..bc728dd44 100644 --- a/tests/conftest_trades.py +++ b/tests/conftest_trades.py @@ -310,7 +310,7 @@ def mock_trade_6(fee): def short_order(): return { - 'id': '1235', + 'id': '1236', 'symbol': 'ETC/BTC', 'status': 'closed', 'side': 'sell', @@ -324,7 +324,7 @@ def short_order(): def exit_short_order(): return { - 'id': '12366', + 'id': '12367', 'symbol': 'ETC/BTC', 'status': 'closed', 'side': 'buy', @@ -397,7 +397,7 @@ def short_trade(fee): def leverage_order(): return { - 'id': '1235', + 'id': '1237', 'symbol': 'ETC/BTC', 'status': 'closed', 'side': 'buy', @@ -412,7 +412,7 @@ def leverage_order(): def leverage_order_sell(): return { - 'id': '12366', + 'id': '12368', 'symbol': 'ETC/BTC', 'status': 'closed', 'side': 'sell', diff --git a/tests/test_persistence.py b/tests/test_persistence.py index 6709a526f..febfd5c2a 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -907,7 +907,7 @@ def test_get_open(fee, use_db): Trade.reset_trades() create_mock_trades(fee, use_db) - assert len(Trade.get_open_trades()) == 5 + assert len(Trade.get_open_trades()) == 4 Trade.use_db = True diff --git a/tests/test_persistence_short.py b/tests/test_persistence_short.py index 782782118..58a2f1e13 100644 --- a/tests/test_persistence_short.py +++ b/tests/test_persistence_short.py @@ -10,7 +10,7 @@ from sqlalchemy import create_engine, inspect, text from freqtrade import constants from freqtrade.exceptions import DependencyException, OperationalException from freqtrade.persistence import LocalTrade, Order, Trade, clean_dry_run_db, init_db -from tests.conftest import create_mock_trades, log_has, log_has_re +from tests.conftest import create_mock_trades_with_leverage, log_has, log_has_re @pytest.mark.usefixtures("init_persistence") @@ -655,21 +655,19 @@ def test_adjust_stop_loss(fee): assert trade.stop_loss_pct == 0.1 # TODO-mg: Do a test with a trade that has a liquidation price -# TODO-mg: I don't know how to do this test, but it should be tested for shorts - @pytest.mark.usefixtures("init_persistence") @pytest.mark.parametrize('use_db', [True, False]) def test_get_open(fee, use_db): Trade.use_db = use_db Trade.reset_trades() - create_mock_trades(fee, use_db) + create_mock_trades_with_leverage(fee, use_db) assert len(Trade.get_open_trades()) == 5 Trade.use_db = True def test_stoploss_reinitialization(default_conf, fee): - # TODO-mg: I don't understand this at all, I was going in the opposite direction as the matching function form test_persistance.py + # TODO-mg: I don't understand this at all, I was just going in the opposite direction as the matching function form test_persistance.py init_db(default_conf['db_url']) trade = Trade( pair='ETH/BTC', @@ -721,83 +719,26 @@ def test_stoploss_reinitialization(default_conf, fee): assert trade_adj.initial_stop_loss == 1.04 assert trade_adj.initial_stop_loss_pct == 0.04 -# @pytest.mark.usefixtures("init_persistence") -# @pytest.mark.parametrize('use_db', [True, False]) -# def test_total_open_trades_stakes(fee, use_db): -# Trade.use_db = use_db -# Trade.reset_trades() -# res = Trade.total_open_trades_stakes() -# assert res == 0 -# create_mock_trades(fee, use_db) -# res = Trade.total_open_trades_stakes() -# assert res == 0.004 -# Trade.use_db = True -# @pytest.mark.usefixtures("init_persistence") -# def test_get_overall_performance(fee): -# create_mock_trades(fee) -# res = Trade.get_overall_performance() -# assert len(res) == 2 -# assert 'pair' in res[0] -# assert 'profit' in res[0] -# assert 'count' in res[0] +@pytest.mark.usefixtures("init_persistence") +@pytest.mark.parametrize('use_db', [True, False]) +def test_total_open_trades_stakes(fee, use_db): + Trade.use_db = use_db + Trade.reset_trades() + res = Trade.total_open_trades_stakes() + assert res == 0 + create_mock_trades_with_leverage(fee, use_db) + res = Trade.total_open_trades_stakes() + assert res == 15.133 + Trade.use_db = True -# @pytest.mark.usefixtures("init_persistence") -# def test_get_best_pair(fee): -# res = Trade.get_best_pair() -# assert res is None -# create_mock_trades(fee) -# res = Trade.get_best_pair() -# assert len(res) == 2 -# assert res[0] == 'XRP/BTC' -# assert res[1] == 0.01 -# @pytest.mark.usefixtures("init_persistence") -# def test_update_order_from_ccxt(caplog): -# # Most basic order return (only has orderid) -# o = Order.parse_from_ccxt_object({'id': '1234'}, 'ETH/BTC', 'buy') -# assert isinstance(o, Order) -# assert o.ft_pair == 'ETH/BTC' -# assert o.ft_order_side == 'buy' -# assert o.order_id == '1234' -# assert o.ft_is_open -# ccxt_order = { -# 'id': '1234', -# 'side': 'buy', -# 'symbol': 'ETH/BTC', -# 'type': 'limit', -# 'price': 1234.5, -# 'amount': 20.0, -# 'filled': 9, -# 'remaining': 11, -# 'status': 'open', -# 'timestamp': 1599394315123 -# } -# o = Order.parse_from_ccxt_object(ccxt_order, 'ETH/BTC', 'buy') -# assert isinstance(o, Order) -# assert o.ft_pair == 'ETH/BTC' -# assert o.ft_order_side == 'buy' -# assert o.order_id == '1234' -# assert o.order_type == 'limit' -# assert o.price == 1234.5 -# assert o.filled == 9 -# assert o.remaining == 11 -# assert o.order_date is not None -# assert o.ft_is_open -# assert o.order_filled_date is None -# # Order has been closed -# ccxt_order.update({'filled': 20.0, 'remaining': 0.0, 'status': 'closed'}) -# o.update_from_ccxt_object(ccxt_order) -# assert o.filled == 20.0 -# assert o.remaining == 0.0 -# assert not o.ft_is_open -# assert o.order_filled_date is not None -# ccxt_order.update({'id': 'somethingelse'}) -# with pytest.raises(DependencyException, match=r"Order-id's don't match"): -# o.update_from_ccxt_object(ccxt_order) -# message = "aaaa is not a valid response object." -# assert not log_has(message, caplog) -# Order.update_orders([o], 'aaaa') -# assert log_has(message, caplog) -# # Call regular update - shouldn't fail. -# Order.update_orders([o], {'id': '1234'}) +@pytest.mark.usefixtures("init_persistence") +def test_get_best_pair(fee): + res = Trade.get_best_pair() + assert res is None + create_mock_trades_with_leverage(fee) + res = Trade.get_best_pair() + assert len(res) == 2 + assert res[0] == 'ETC/BTC' + assert res[1] == 0.22626016260162551 From e358860a698d51ee6c2e188efb68eb9ef1dbbdd1 Mon Sep 17 00:00:00 2001 From: Matthias Date: Sat, 3 Jul 2021 17:03:12 +0200 Subject: [PATCH 2/2] Fix migrations, revert some parts related to amount properties --- freqtrade/persistence/migrations.py | 25 +++++---- freqtrade/persistence/models.py | 79 +++++++++++++++-------------- tests/test_persistence.py | 3 +- 3 files changed, 58 insertions(+), 49 deletions(-) diff --git a/freqtrade/persistence/migrations.py b/freqtrade/persistence/migrations.py index ef4a5623b..efadc7467 100644 --- a/freqtrade/persistence/migrations.py +++ b/freqtrade/persistence/migrations.py @@ -91,7 +91,8 @@ def migrate_trades_table(decl_base, inspector, engine, table_back_name: str, col stoploss_order_id, stoploss_last_update, max_rate, min_rate, sell_reason, sell_order_status, strategy, timeframe, open_trade_value, close_profit_abs, - leverage, borrowed, borrowed_currency, collateral_currency, interest_rate, liquidation_price, is_short + leverage, borrowed, borrowed_currency, collateral_currency, interest_rate, + liquidation_price, is_short ) select id, lower(exchange), case @@ -115,8 +116,8 @@ def migrate_trades_table(decl_base, inspector, engine, table_back_name: str, col {sell_order_status} sell_order_status, {strategy} strategy, {timeframe} timeframe, {open_trade_value} open_trade_value, {close_profit_abs} close_profit_abs, - {leverage} leverage, {borrowed} borrowed, {borrowed_currency} borrowed_currency, - {collateral_currency} collateral_currency, {interest_rate} interest_rate, + {leverage} leverage, {borrowed} borrowed, {borrowed_currency} borrowed_currency, + {collateral_currency} collateral_currency, {interest_rate} interest_rate, {liquidation_price} liquidation_price, {is_short} is_short from {table_back_name} """)) @@ -152,14 +153,17 @@ def migrate_orders_table(decl_base, inspector, engine, table_back_name: str, col # let SQLAlchemy create the schema as required decl_base.metadata.create_all(engine) + leverage = get_column_def(cols, 'leverage', 'null') + is_short = get_column_def(cols, 'is_short', 'False') with engine.begin() as connection: connection.execute(text(f""" insert into orders ( id, ft_trade_id, ft_order_side, ft_pair, ft_is_open, order_id, status, symbol, order_type, side, price, amount, filled, average, remaining, cost, - order_date, order_filled_date, order_update_date, leverage) + order_date, order_filled_date, order_update_date, leverage, is_short) select id, ft_trade_id, ft_order_side, ft_pair, ft_is_open, order_id, status, symbol, order_type, side, price, amount, filled, null average, remaining, cost, - order_date, order_filled_date, order_update_date, leverage + order_date, order_filled_date, order_update_date, + {leverage} leverage, {is_short} is_short from {table_back_name} """)) @@ -174,8 +178,9 @@ def check_migrate(engine, decl_base, previous_tables) -> None: tabs = get_table_names_for_table(inspector, 'trades') table_back_name = get_backup_name(tabs, 'trades_bak') - # Check for latest column - if not has_column(cols, 'open_trade_value'): + # Last added column of trades table + # To determine if migrations need to run + if not has_column(cols, 'collateral_currency'): logger.info(f'Running database migration for trades - backup: {table_back_name}') migrate_trades_table(decl_base, inspector, engine, table_back_name, cols) # Reread columns - the above recreated the table! @@ -188,9 +193,11 @@ def check_migrate(engine, decl_base, previous_tables) -> None: else: cols_order = inspector.get_columns('orders') - if not has_column(cols_order, 'average'): + # Last added column of order table + # To determine if migrations need to run + if not has_column(cols_order, 'leverage'): tabs = get_table_names_for_table(inspector, 'orders') # Empty for now - as there is only one iteration of the orders table so far. table_back_name = get_backup_name(tabs, 'orders_bak') - migrate_orders_table(decl_base, inspector, engine, table_back_name, cols) + migrate_orders_table(decl_base, inspector, engine, table_back_name, cols_order) diff --git a/freqtrade/persistence/models.py b/freqtrade/persistence/models.py index 380542a20..31af223df 100644 --- a/freqtrade/persistence/models.py +++ b/freqtrade/persistence/models.py @@ -237,7 +237,7 @@ class LocalTrade(): close_profit: Optional[float] = None close_profit_abs: Optional[float] = None stake_amount: float = 0.0 - _amount: float = 0.0 + amount: float = 0.0 amount_requested: Optional[float] = None open_date: datetime close_date: Optional[datetime] = None @@ -269,8 +269,8 @@ class LocalTrade(): interest_rate: float = 0.0 liquidation_price: float = None is_short: bool = False - _borrowed: float = 0.0 - _leverage: float = None # * You probably want to use LocalTrade.leverage instead + borrowed: float = 0.0 + leverage: float = None # @property # def base_currency(self) -> str: @@ -278,42 +278,45 @@ class LocalTrade(): # raise OperationalException('LocalTrade.pair must be assigned') # return self.pair.split("/")[1] - @property - def amount(self) -> float: - if self._leverage is not None: - return self._amount * self.leverage - else: - return self._amount + # TODO: @samgermain: Amount should be persisted "as is". + # I've partially reverted this (this killed most of your tests) + # but leave this here as i'm not sure where you intended to use this. + # @property + # def amount(self) -> float: + # if self._leverage is not None: + # return self._amount * self.leverage + # else: + # return self._amount - @amount.setter - def amount(self, value): - self._amount = value + # @amount.setter + # def amount(self, value): + # self._amount = value - @property - def borrowed(self) -> float: - if self._leverage is not None: - if self.is_short: - # If shorting the full amount must be borrowed - return self._amount * self._leverage - else: - # If not shorting, then the trader already owns a bit - return self._amount * (self._leverage-1) - else: - return self._borrowed + # @property + # def borrowed(self) -> float: + # if self._leverage is not None: + # if self.is_short: + # # If shorting the full amount must be borrowed + # return self._amount * self._leverage + # else: + # # If not shorting, then the trader already owns a bit + # return self._amount * (self._leverage-1) + # else: + # return self._borrowed - @borrowed.setter - def borrowed(self, value): - self._borrowed = value - self._leverage = None + # @borrowed.setter + # def borrowed(self, value): + # self._borrowed = value + # self._leverage = None - @property - def leverage(self) -> float: - return self._leverage + # @property + # def leverage(self) -> float: + # return self._leverage - @leverage.setter - def leverage(self, value): - self._leverage = value - self._borrowed = None + # @leverage.setter + # def leverage(self, value): + # self._leverage = value + # self._borrowed = None # End of margin trading properties @@ -642,7 +645,7 @@ class LocalTrade(): # sec_per_day = Decimal(86400) sec_per_hour = Decimal(3600) total_seconds = Decimal((now - open_date).total_seconds()) - #days = total_seconds/sec_per_day or zero + # days = total_seconds/sec_per_day or zero hours = total_seconds/sec_per_hour or zero rate = Decimal(interest_rate or self.interest_rate) @@ -880,7 +883,7 @@ class Trade(_DECL_BASE, LocalTrade): close_profit = Column(Float) close_profit_abs = Column(Float) stake_amount = Column(Float, nullable=False) - _amount = Column(Float) + amount = Column(Float) amount_requested = Column(Float) open_date = Column(DateTime, nullable=False, default=datetime.utcnow) close_date = Column(DateTime) @@ -907,8 +910,8 @@ class Trade(_DECL_BASE, LocalTrade): timeframe = Column(Integer, nullable=True) # Margin trading properties - _leverage: float = None # * You probably want to use LocalTrade.leverage instead - _borrowed = Column(Float, nullable=False, default=0.0) + leverage = Column(Float, nullable=True) # TODO: can this be nullable, or should it default to 1? (must also be changed in migrations eventually) + borrowed = Column(Float, nullable=False, default=0.0) interest_rate = Column(Float, nullable=False, default=0.0) liquidation_price = Column(Float, nullable=True) is_short = Column(Boolean, nullable=False, default=False) diff --git a/tests/test_persistence.py b/tests/test_persistence.py index febfd5c2a..82829cd6b 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -723,13 +723,11 @@ def test_migrate_new(mocker, default_conf, fee, caplog): order_date DATETIME, order_filled_date DATETIME, order_update_date DATETIME, - leverage FLOAT, PRIMARY KEY (id), CONSTRAINT _order_pair_order_id UNIQUE (ft_pair, order_id), FOREIGN KEY(ft_trade_id) REFERENCES trades (id) ) """)) - # TODO-mg @xmatthias: Had to add field leverage to this table, check that this is correct connection.execute(text(""" insert into orders ( id, ft_trade_id, ft_order_side, ft_pair, ft_is_open, order_id, status, symbol, order_type, side, price, amount, filled, remaining, cost, order_date, @@ -752,6 +750,7 @@ def test_migrate_new(mocker, default_conf, fee, caplog): assert orders[1].order_id == 'stop_order_id222' assert orders[1].ft_order_side == 'stoploss' + assert orders[0].is_short is False def test_migrate_mid_state(mocker, default_conf, fee, caplog):