Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Workaround for issue #1629 #1635

Closed
wants to merge 37 commits into from

Conversation

station384
Copy link
Contributor

There is currently a bug in the engine that allows for a buy to occur when a buy is already active or a sell to occur when a sell is already active causing 2 orders to be be active on the exchange these functions check for this condition and return true or false if this condition exists. see #1629

This doesn't correct the core problem in the engine, but will stop any strategy from triggering the possibility of the condition.

There is currently a bug in the engine that allows for a buy to occur when a buy is already active or a sell to occur when  a sell is already active these functions check for this condition and return true or false if this condition exists.  see DeviaVir#1629

This check will become a useless step once the problem is corrected in the engine,  as this check will be redundant.
Copy link
Contributor

@defkev defkev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather fix the problem than throw a workaround at 37 files.

I currently suspect that the problem is somewhere in the checkOrder timeout, which is responsible to call cancelOrder if the s.buy|sell_order no longer exists.
Maybe its overlapping with a concurrent call or doesn't get set at all?

setTimeout(function() { checkOrder(order, type, cb) }, so.order_poll_time)

@JohnnyVicious
Copy link

Was thinking the same as @defkev when I saw the mail for this commit and the list of files, this probably has a better way of solving with some further investigation

@station384
Copy link
Contributor Author

I agree completely. I dislike workarounds myself, and a really dislike this type of workaround as this type of workaround puts the responsibility on the person designing the strategy, which they should not be responsible for.

I ended up having to go this route in my fork, as I couldn't get a fix in the engine, after 10 or so different tries I either didn't fix the problem, or created a new one. So in frustration ( and my wife demanding that I stop ignoring the family when on vacation ) I just decided to prevent the occurrence from happening in the first place so I could get on with what I was trying to accomplish with the strategy I am working on.

I tried to design the workaround in a way that when the problem is fixed in the engine this code will be easy enough to identify and remove, and if not removed it will have no affect, so I thought I would share it.

------ Reason I created this workaround
I needed this fix for a strategy I was working on to catch edge cases in the market which the bot can't handle due to the bug. for example, I do not want to buy using 99% on first purchase on the first signal of a buy, but buy only 10%, on second signal I want to buy 30% and on third I want to buy using the remaining amount of capital (this an example of the pattern my strategy will try). This allows me to catch the large drops that occur when someone is dumping into the market. that way I haven't bought at just the first drop but spread my purchases out as the price drops getting a better price each purchase, and if it rebounds back there is only the last order sitting on the exchange that the bot still has track of and will cancel. This can happen in reverse conditions when selling as well.

The bot can do this somewhat now, but it will end up creating 2 orders on the exchange, and if the first order doesn't complete, it will be orphaned when the second order is created, and tie up the capital till the first order completes which I seen sit there till I have to log into the exchange and manually cancel.

@station384
Copy link
Contributor Author

station384 commented Jun 23, 2018

@JohnnyVicious I did separate commits so it would be easy to cheery-pick in a merge, the only real/important one is the first one (Library of functions) and maybe an example of how to use it.

The changes to the other strategies are a convenience so other strategies get the workaround. I don't use them, but the problem can occur in them too, but do not need to be included in the merge, others that do use them can implement the workaround in theirs using the library, and if there just using the end product with no coding experience to implement a work around, they will just have to wait till it is fixed in the engine.

I thought "lets have our cake and eat it too", Its a 2 stage fix, 1st being a workaround, 2nd being the fix. I know when a workaround is implemented the problem usually gets shoved to the back burner because "I can wait to fix that, this workaround takes care of it right now", and I expect this to happen somewhat, that is why I put the workaround in a library and made it easy to identify in the code (hopefully for others and not just me).

@station384
Copy link
Contributor Author

station384 commented Jun 23, 2018

@defkev
My findings in the engine was that it was working correctly, but is only designed to detect a switch. It doesn't recognize buy -> buy. the old/currrent one doesn't even go down the code path to get canceled because the new one is not a sell. I tried implementing code to expand on the switch detection but that ended up causing problems during a true switch (buy -> sell), granted my logic could/was most likely at fault, when detecting the condition that caused it not to work and I will be revisiting it, if I don't see a fix from someone else when I finish the strategy I'm playing with.

@JohnnyVicious
Copy link

Really appreciate the work you do @station384 but go back to your vacation time 👍

@station384
Copy link
Contributor Author

@JohnnyVicious LOL
What? this is vacation to me :)

@defkev
Copy link
Contributor

defkev commented Jun 23, 2018

As i wrote, the bot should never execute an order if s.buy|sell_order of the same type is already set, just like it should execute a cancelOrder if s.sell_order is set on a buy, or s.buy_order on a sell.

The problem seems to be that this isn't the case (s.buy|sell_order is undefined in this edge-case) which leads to the observed behavior.

Now the million dollar question is why does this happen, especially since i've never seen it before in the almost 6 months i am using the bot by now, nor can i find anything in the commit history explaining this sudden change of behavior.

Will probably take some time to catch it in debugging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants