From fc684b00917d2d135704e52a2974e0c5a272fb26 Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 17 Apr 2020 06:59:52 +0200 Subject: [PATCH 1/6] Ensure deleting filled is not raising an error if filled does not exist --- freqtrade/freqtradebot.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/freqtrade/freqtradebot.py b/freqtrade/freqtradebot.py index 213444f72..8865dd20b 100644 --- a/freqtrade/freqtradebot.py +++ b/freqtrade/freqtradebot.py @@ -1140,7 +1140,7 @@ class FreqtradeBot: new_amount = self.get_real_amount(trade, order, order_amount) if not isclose(order['amount'], new_amount, abs_tol=constants.MATH_CLOSE_PREC): order['amount'] = new_amount - del order['filled'] + order.pop('filled', None) # Fee was applied, so set to 0 trade.fee_open = 0 trade.recalc_open_trade_price() From 800891a4759b3f2e96f3593be9471f089c7e2e92 Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 17 Apr 2020 07:18:46 +0200 Subject: [PATCH 2/6] Add tests for cancel_order_with_result --- freqtrade/exchange/exchange.py | 30 ++++++++++++++++++++++++++++++ tests/exchange/test_exchange.py | 14 ++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/freqtrade/exchange/exchange.py b/freqtrade/exchange/exchange.py index f2f8cd69f..412a60386 100644 --- a/freqtrade/exchange/exchange.py +++ b/freqtrade/exchange/exchange.py @@ -937,6 +937,36 @@ class Exchange: except ccxt.BaseError as e: raise OperationalException(e) from e + def is_cancel_order_result_suitable(self, corder) -> bool: + if not isinstance(corder, dict): + return False + + required = ('fee', 'status', 'amount') + return all(k in corder for k in required) + + def cancel_order_with_result(self, order_id: str, pair: str, amount: float) -> Dict: + """ + Cancel order returning a result. + Creates a fake result if cancel order returns a non-usable result + and get_order does not work (certain exchanges don't return cancelled orders) + :param order_id: Orderid to cancel + :param pair: Pair corresponding to order_id + :param amount: Amount to use for fake response + :return: Result from either cancel_order if usable, or fetch_order + """ + if self._config['dry_run']: + return {'fee': {}, 'status': 'canceled', 'amount': amount, 'info': {}} + corder = self.cancel_order(order_id, pair) + if self.is_cancel_order_result_suitable(corder): + return corder + try: + order = self.get_order(order_id, pair) + except InvalidOrderException: + logger.warning(f"Could not fetch cancelled order {order_id}.") + order = {'fee': {}, 'status': 'canceled', 'amount': amount, 'info': {}} + + return order + @retrier def get_order(self, order_id: str, pair: str) -> Dict: if self._config['dry_run']: diff --git a/tests/exchange/test_exchange.py b/tests/exchange/test_exchange.py index b74ea1a6b..1f0df5aa5 100644 --- a/tests/exchange/test_exchange.py +++ b/tests/exchange/test_exchange.py @@ -1745,6 +1745,20 @@ def test_check_order_canceled_empty(mocker, default_conf, exchange_name, order, assert exchange.check_order_canceled_empty(order) == result +@pytest.mark.parametrize("exchange_name", EXCHANGES) +@pytest.mark.parametrize("order,result", [ + ({'status': 'closed', 'amount': 10, 'fee': {}}, True), + ({'status': 'closed', 'amount': 0.0, 'fee': {}}, True), + ({'status': 'canceled', 'amount': 0.0, 'fee': {}}, True), + ({'status': 'canceled', 'amount': 10.0}, False), + ({'amount': 10.0, 'fee': {}}, False), + ({'result': 'testest123'}, False), + ('hello_world', False), +]) +def test_is_cancel_order_result_suitable(mocker, default_conf, exchange_name, order, result): + exchange = get_patched_exchange(mocker, default_conf, id=exchange_name) + assert exchange.is_cancel_order_result_suitable(order) == result + # Ensure that if not dry_run, we should call API @pytest.mark.parametrize("exchange_name", EXCHANGES) def test_cancel_order(default_conf, mocker, exchange_name): From 5e3e0e819f20869b4d3e9a04abe4cb3b040bede9 Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 17 Apr 2020 17:53:18 +0200 Subject: [PATCH 3/6] Add tests for cancel_order_with_result --- freqtrade/exchange/exchange.py | 11 ++++++----- tests/exchange/test_exchange.py | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/freqtrade/exchange/exchange.py b/freqtrade/exchange/exchange.py index 412a60386..d3a520eaa 100644 --- a/freqtrade/exchange/exchange.py +++ b/freqtrade/exchange/exchange.py @@ -954,11 +954,12 @@ class Exchange: :param amount: Amount to use for fake response :return: Result from either cancel_order if usable, or fetch_order """ - if self._config['dry_run']: - return {'fee': {}, 'status': 'canceled', 'amount': amount, 'info': {}} - corder = self.cancel_order(order_id, pair) - if self.is_cancel_order_result_suitable(corder): - return corder + try: + corder = self.cancel_order(order_id, pair) + if self.is_cancel_order_result_suitable(corder): + return corder + except InvalidOrderException: + logger.warning(f"Could not cancel order {order_id}.") try: order = self.get_order(order_id, pair) except InvalidOrderException: diff --git a/tests/exchange/test_exchange.py b/tests/exchange/test_exchange.py index 1f0df5aa5..e3ab76b92 100644 --- a/tests/exchange/test_exchange.py +++ b/tests/exchange/test_exchange.py @@ -1759,6 +1759,40 @@ def test_is_cancel_order_result_suitable(mocker, default_conf, exchange_name, or exchange = get_patched_exchange(mocker, default_conf, id=exchange_name) assert exchange.is_cancel_order_result_suitable(order) == result + +@pytest.mark.parametrize("exchange_name", EXCHANGES) +@pytest.mark.parametrize("corder,call_corder,call_forder", [ + ({'status': 'closed', 'amount': 10, 'fee': {}}, 1, 0), + ({'amount': 10, 'fee': {}}, 1, 1), +]) +def test_cancel_order_with_result(default_conf, mocker, exchange_name, corder, + call_corder, call_forder): + default_conf['dry_run'] = False + api_mock = MagicMock() + api_mock.cancel_order = MagicMock(return_value=corder) + api_mock.fetch_order = MagicMock(return_value={}) + exchange = get_patched_exchange(mocker, default_conf, api_mock, id=exchange_name) + res = exchange.cancel_order_with_result('1234', 'ETH/BTC', 1234) + assert isinstance(res, dict) + assert api_mock.cancel_order.call_count == call_corder + assert api_mock.fetch_order.call_count == call_forder + + +@pytest.mark.parametrize("exchange_name", EXCHANGES) +def test_cancel_order_with_result_error(default_conf, mocker, exchange_name, caplog): + default_conf['dry_run'] = False + api_mock = MagicMock() + api_mock.cancel_order = MagicMock(side_effect=ccxt.InvalidOrder("Did not find order")) + api_mock.fetch_order = MagicMock(side_effect=ccxt.InvalidOrder("Did not find order")) + exchange = get_patched_exchange(mocker, default_conf, api_mock, id=exchange_name) + + res = exchange.cancel_order_with_result('1234', 'ETH/BTC', 1541) + assert isinstance(res, dict) + assert log_has("Could not cancel order 1234.", caplog) + assert log_has("Could not fetch cancelled order 1234.", caplog) + assert res['amount'] == 1541 + + # Ensure that if not dry_run, we should call API @pytest.mark.parametrize("exchange_name", EXCHANGES) def test_cancel_order(default_conf, mocker, exchange_name): From 1069cb3616001e44bdc17e68e36d46bb93544d4a Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 17 Apr 2020 17:53:56 +0200 Subject: [PATCH 4/6] Use cancel_order_with_result when cancelling orders after timeout --- freqtrade/freqtradebot.py | 12 +++++------- tests/test_freqtradebot.py | 13 ++++++------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/freqtrade/freqtradebot.py b/freqtrade/freqtradebot.py index 8865dd20b..f4aeba3fe 100644 --- a/freqtrade/freqtradebot.py +++ b/freqtrade/freqtradebot.py @@ -891,11 +891,8 @@ class FreqtradeBot: if order['status'] != 'canceled': reason = "cancelled due to timeout" try: - corder = self.exchange.cancel_order(trade.open_order_id, trade.pair) - # Some exchanges don't return a dict here. - if not isinstance(corder, dict): - corder = {} - logger.info('Buy order %s for %s.', reason, trade) + corder = self.exchange.cancel_order_with_result(trade.open_order_id, trade.pair, + trade.amount) except InvalidOrderException: corder = {} logger.exception( @@ -904,7 +901,8 @@ class FreqtradeBot: # Order was cancelled already, so we can reuse the existing dict corder = order reason = "cancelled on exchange" - logger.info('Buy order %s for %s.', reason, trade) + + logger.info('Buy order %s for %s.', reason, trade) if safe_value_fallback(corder, order, 'remaining', 'remaining') == order['amount']: logger.info('Buy order fully cancelled. Removing %s from database.', trade) @@ -921,7 +919,7 @@ class FreqtradeBot: trade.amount = order['amount'] - safe_value_fallback(corder, order, 'remaining', 'remaining') trade.stake_amount = trade.amount * trade.open_rate - self.update_trade_state(trade, corder if 'fee' in corder else order, trade.amount) + self.update_trade_state(trade, corder, trade.amount) trade.open_order_id = None logger.info('Partial buy order timeout for %s.', trade) diff --git a/tests/test_freqtradebot.py b/tests/test_freqtradebot.py index 1102dd344..c3e78c9be 100644 --- a/tests/test_freqtradebot.py +++ b/tests/test_freqtradebot.py @@ -1948,7 +1948,7 @@ def test_check_handle_timedout_buy(default_conf, ticker, limit_buy_order_old, op 'freqtrade.exchange.Exchange', fetch_ticker=ticker, get_order=MagicMock(return_value=limit_buy_order_old), - cancel_order=cancel_order_mock, + cancel_order_with_result=cancel_order_mock, get_fee=fee ) freqtrade = FreqtradeBot(default_conf) @@ -2055,7 +2055,7 @@ def test_check_handle_cancelled_sell(default_conf, ticker, limit_sell_order_old, 'freqtrade.exchange.Exchange', fetch_ticker=ticker, get_order=MagicMock(return_value=limit_sell_order_old), - cancel_order=cancel_order_mock + cancel_order_with_result=cancel_order_mock ) freqtrade = FreqtradeBot(default_conf) @@ -2082,7 +2082,7 @@ def test_check_handle_timedout_partial(default_conf, ticker, limit_buy_order_old 'freqtrade.exchange.Exchange', fetch_ticker=ticker, get_order=MagicMock(return_value=limit_buy_order_old_partial), - cancel_order=cancel_order_mock + cancel_order_with_result=cancel_order_mock ) freqtrade = FreqtradeBot(default_conf) @@ -2109,7 +2109,7 @@ def test_check_handle_timedout_partial_fee(default_conf, ticker, open_trade, cap 'freqtrade.exchange.Exchange', fetch_ticker=ticker, get_order=MagicMock(return_value=limit_buy_order_old_partial), - cancel_order=cancel_order_mock, + cancel_order_with_result=cancel_order_mock, get_trades_for_order=MagicMock(return_value=trades_for_order), ) freqtrade = FreqtradeBot(default_conf) @@ -2146,7 +2146,7 @@ def test_check_handle_timedout_partial_except(default_conf, ticker, open_trade, 'freqtrade.exchange.Exchange', fetch_ticker=ticker, get_order=MagicMock(return_value=limit_buy_order_old_partial), - cancel_order=cancel_order_mock, + cancel_order_with_result=cancel_order_mock, get_trades_for_order=MagicMock(return_value=trades_for_order), ) mocker.patch('freqtrade.freqtradebot.FreqtradeBot.get_real_amount', @@ -2208,7 +2208,7 @@ def test_handle_timedout_limit_buy(mocker, caplog, default_conf, limit_buy_order patch_RPCManager(mocker) patch_exchange(mocker) cancel_order_mock = MagicMock(return_value=limit_buy_order) - mocker.patch('freqtrade.exchange.Exchange.cancel_order', cancel_order_mock) + mocker.patch('freqtrade.exchange.Exchange.cancel_order_with_result', cancel_order_mock) freqtrade = FreqtradeBot(default_conf) @@ -2226,7 +2226,6 @@ def test_handle_timedout_limit_buy(mocker, caplog, default_conf, limit_buy_order mocker.patch('freqtrade.exchange.Exchange.cancel_order', side_effect=InvalidOrderException) assert not freqtrade.handle_timedout_limit_buy(trade, limit_buy_order) - assert log_has_re(r"Could not cancel buy order", caplog) @pytest.mark.parametrize('cancelorder', [ From 0273539f0614d58578f354bbc55448152d5036c8 Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 17 Apr 2020 19:55:53 +0200 Subject: [PATCH 5/6] Remove exceptionhandler, this exception is handled in cancel_with_response --- freqtrade/freqtradebot.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/freqtrade/freqtradebot.py b/freqtrade/freqtradebot.py index f4aeba3fe..65ba46987 100644 --- a/freqtrade/freqtradebot.py +++ b/freqtrade/freqtradebot.py @@ -890,13 +890,8 @@ class FreqtradeBot: """ if order['status'] != 'canceled': reason = "cancelled due to timeout" - try: - corder = self.exchange.cancel_order_with_result(trade.open_order_id, trade.pair, - trade.amount) - except InvalidOrderException: - corder = {} - logger.exception( - f"Could not cancel buy order {trade.open_order_id} for pair {trade.pair}") + corder = self.exchange.cancel_order_with_result(trade.open_order_id, trade.pair, + trade.amount) else: # Order was cancelled already, so we can reuse the existing dict corder = order From c775d6512670916f206d39d02ca2ec8dc52ecfd8 Mon Sep 17 00:00:00 2001 From: Matthias Date: Sat, 18 Apr 2020 06:55:25 +0200 Subject: [PATCH 6/6] Update typehint for cancel_order --- freqtrade/exchange/exchange.py | 4 ++-- tests/exchange/test_exchange.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/freqtrade/exchange/exchange.py b/freqtrade/exchange/exchange.py index d3a520eaa..1a0565959 100644 --- a/freqtrade/exchange/exchange.py +++ b/freqtrade/exchange/exchange.py @@ -922,9 +922,9 @@ class Exchange: return order.get('status') in ('closed', 'canceled') and order.get('filled') == 0.0 @retrier - def cancel_order(self, order_id: str, pair: str) -> None: + def cancel_order(self, order_id: str, pair: str) -> Dict: if self._config['dry_run']: - return + return {} try: return self._api.cancel_order(order_id, pair) diff --git a/tests/exchange/test_exchange.py b/tests/exchange/test_exchange.py index e3ab76b92..3c92612a0 100644 --- a/tests/exchange/test_exchange.py +++ b/tests/exchange/test_exchange.py @@ -1728,7 +1728,7 @@ def test_get_historic_trades_notsupported(default_conf, mocker, caplog, exchange def test_cancel_order_dry_run(default_conf, mocker, exchange_name): default_conf['dry_run'] = True exchange = get_patched_exchange(mocker, default_conf, id=exchange_name) - assert exchange.cancel_order(order_id='123', pair='TKN/BTC') is None + assert exchange.cancel_order(order_id='123', pair='TKN/BTC') == {} @pytest.mark.parametrize("exchange_name", EXCHANGES)