Logical error in staggered orders maintenance after bot offline period

[bitphage]

Actual behavior

In the staggered orders strategy we have a following logic at the initialization:

        if self['setup_done']:
            self.place_orders()
        else:
            self.init_strategy()

By this logic a fresh worker will do self.init_strategy(). It will:

1. Create a market orders
2. Save orders to the database

Next, we're stopped worker and let it be offline. During this time some orders will be filled.

Next, we're starting worker again. This time self.place_orders() will be executed. This means bot will place already filled orders again likely causing an error Insufficient buy/sell balance:

    def place_orders(self):
        """ Place all the orders found in the database
        """
        orders = self.fetch_orders()
        for order_id, order in orders.items():
            if not self.get_order(order_id):
                self.place_order(order)

Expected behavior

Instead of trying to repeat already filled order bot must place a reverse order. The logic I expect:

1. Init strategy, place orders, save them to the dadabase
2. Put bot offline
3. Let orders fill
4. Start worker
5. Worker fetches orders from database and check whether they exists on the market
6. Order found in the database but not present in the orderbook. Causes? Order was cancelled manually or filled. Assume we should not manually touch bot's order. Then, the only way order disappeared is that it was filled!
7. So we know order was filled, so place reverse order.

@MarkoPaasila the fix is very easy and I'll push it soon.

[MarkoPaasila]

@bitfag Awesome! Thanks for finding and fixing this!

[mikakoi]

Let's move the discussion from the pr to here:

The problem with resolving this with a paused state is that if a buy order is filled while the worker is offline and then the highest ask rises above the buy order price, you wouldn't want to place a sell order there because it would just be instantly filled.

Instead of bringing a new state 'paused' to the worker, I think it would be better to compare the price of the order we're about to place with the lowest ask or highest bid and determine with that if the order should be a buy or sell order.

[MarkoPaasila]

With Stagggered Orders you can only place orders with the funds you have obtained from filled orders, and you can only place orders that the funds allow. If a filled order provided you some BASE, you can't place a sell order. Even if you wanted to. Your only choice is to make that buy order which may or may not fill immediately, and then you will have some Quote with which to place a sell order.

I think we should allow this to happen, as it won't result in losses - just a little less profit than with a better solution. The next feature to be implemented is an improved maintenance strategy, in which we should try to account for this.

So I opt for approving this PR. If I understand correctly, the strategy was trying to do something it wasn't able to, and shouldn't be able to. And the fix is to make it do what it can and ought to, even though it is sub-optimal?

[bitphage]

@mikakoi you're actually describing another situation. Actually we're discussed it already with @MarkoPaasila in the telegram chat and he said it's not really a problem. Though, logic of order placement in such situation indeed can be improved. I would plan to improve it in next steps.

What you suggesting now is to mix worker states and use common way of handling different states which is looks wrong to me.

The worker now have the following states:

• New (just defined in config, no orders at all)
• Running (orders in the DB + orders in the market)
• Offline (orders in the DB + orders in the market + worker is not running)
• Stopped (orders in DB only)

There is no completely separate "paused" state. You suggesting to mix transition from "Offline" and "Stopped" to "Running". But these are really different states and you need to handle them differently. Not by just dumb-placing orders from the DB.

[mikakoi]

@bitfag I talked with marko about this and turns out staggered orders isn't supposed to cancel all orders on pause. So this pr can be approved once there is a mechanism for canceling orders on pause for relative orders but not for staggered orders. So there is no need for a paused state. Do you think you could do that?

[bitphage]

@mikakoi yeah, this sounds reasonable, I could implement handling for this later today.

[MarkoPaasila]

And I shall be more careful when defining new features - not to forget to mention something as essential as this :-)

[MarkoPaasila]

Probably related to this "feature", I killed my dexbot-cli, upgraded, and started it again. The bot sold everything I had at 1/100 of market value. Only a bit of dust was left on my account. It seems to me that all the orders were indeed reversed.

[bitphage]

@MarkoPaasila how did you killed dexbot-cli? With C-c or with kill -9? Did bot cancelled all orders or not?

This #160 should be fixed in conjunction with #148 because exiting cli with C-c will kill orders from market but will keep them in database, so at the next restart orders will be treated as filled and will be reversed (which should not happen of course).

[bitphage]

I added two commits to #161 which implements @mikakoi proposal for canceling orders on pause for relative orders but not for staggered orders. So #137 is going to be fixed automatically by using this mechanism.

[MarkoPaasila]

I'm pretty sure I stopped it with ctrl + c, although I can't see terminal output so far back as to verify.