Merge pull request #3181 from freqtrade/fix/cancel_problems

Fix several cancel order problems

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)
@@ -937,6 +937,37 @@ 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
+        """
+        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:
+            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']:

freqtrade/freqtradebot.py

@@ -890,21 +890,14 @@ 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,
-                corder = self.exchange.cancel_order(trade.open_order_id, trade.pair)
+                                                            trade.amount)
-                # Some exchanges don't return a dict here.
-                if not isinstance(corder, dict):
-                    corder = {}
-                logger.info('Buy order %s for %s.', reason, trade)
-            except InvalidOrderException:
-                corder = {}
-                logger.exception(
-                    f"Could not cancel buy order {trade.open_order_id} for pair {trade.pair}")
         else:
             # 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 +914,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)
@@ -1140,7 +1133,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()

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)
@@ -1745,6 +1745,54 @@ 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
+
+
+@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):

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', [