-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: exceeding max open trades #462
fix: exceeding max open trades #462
Conversation
@chrisleekr Please, don't merge yet as there is still an issue with less traded symbols. As the |
572d4ec
to
9b7b396
Compare
9b7b396
to
fea6d39
Compare
@uhliksk |
@chrisleekr Not yet. I was busy and I didn't finished the commit which will help to cancel the buy order even when there is no price change since the max. open trade limit is reached. |
635128d
to
6dfc0d0
Compare
@habibalkhabbaz I'm modifying |
@uhliksk Ah, just to confirm, do you want to include this fix in the release? Then we can wait for the release. |
@chrisleekr Yes, I was just waiting until #464 is merged to replace my simple queue to the new queueing mechanism. I thought it'll be simple function replace but I have an issue with |
Hello @uhliksk |
@habibalkhabbaz Sorry, I forgot to push last commit. |
@habibalkhabbaz This is the line throwing the error. binance-trading-bot/app/cronjob/trailingTrade/step/ensure-grid-trade-order-executed.js Line 262 in 1fe9e72
|
@chrisleekr If it's not throwing the error for you, then you can merge, but for me it's logging this when buy order is filled:
|
@@ -254,6 +255,13 @@ const execute = async (logger, rawData) => { | |||
}, | |||
temporaryDisableActionAfterConfirmingOrder | |||
); | |||
|
|||
// Queue other symbols to check if max. open trade is reached | |||
symbols.map(async symbolToQueue => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map
can be replaced by forEach
and I think no need for async
here because we are not expecting a result.
@uhliksk |
@chrisleekr |
That's great @uhliksk |
@habibalkhabbaz Maybe. It's still throwing |
@habibalkhabbaz Another observation, the error is logged single time when buy order is executed, but forEach will run it multiple times for all symbols so it looks like just a single symbol is throwing the error. That's should proof it's just my local problem and the code is really good to go. Edit: Actually I removed |
Ops. |
@uhliksk That's great but I think it will be complicated and it will be hard to debug. |
@habibalkhabbaz |
@uhliksk
Maybe we have to refactor the queue as you said. |
@habibalkhabbaz |
@habibalkhabbaz Edit: Or will it also cause the circular dependency to call |
@uhliksk |
@habibalkhabbaz Edit: I was wrong. There is a time of creation for orders. |
Hello @uhliksk |
@habibalkhabbaz |
Just a note: Orders are cancelled on Binance instantly, notifications in bot are triggered instantly, but from |
It's the same with manual orders. I'll create order, it will show on Binance and bot almost instantly. I'll cancel order, it is cancelled on Binance instantly, but in bot on another price change. Do you know @habibalkhabbaz where it should be fixed? |
I pushed a fix for this.
However, for manual order cancellation, it should be reflected on the frontend directly. |
I fixed a fix for this. :) |
@uhliksk @chrisleekr |
Sorry for joining the late party. Let me review and merge in! |
|
||
// Reset buy open orders | ||
if (cancelResult === true) { | ||
data.buy.openOrders = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancelling the order often fails even if it was successful.
And when it succeeds to cancel, it should process the same as stop > limit price cancel process.
Agree? @uhliksk @habibalkhabbaz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me quickly refactor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It'll be cleaner way even as this is actually cancelling the first buy order which have no impact on anything. It will save some headache in future if it will be reused for another purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was gonna just update this file, but @uhliksk is right. I will make a generic function in the common.js and update other steps to reduce duplicated codes.
Allow me some time. There will be additional commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a generic function refreshOpenOrdersAndAccountInfo
to common.js
- https://github.com/chrisleekr/binance-trading-bot/pull/462/files#diff-478c565f3dc02bbdb099f674677ebcd7d1f16fa4ddd06220dd3d6119b8467b84R1130
And I updated handle-open-orders.js
to use refreshOpenOrdersAndAccountInfo
- https://github.com/chrisleekr/binance-trading-bot/pull/462/files#diff-48b71d5ac0a2a4fd94708519e33e808e09b1a98ab8df1b8996f499d8b71a244bR127
I checked and found some other steps have exactly the same code.
However, refactoring those steps seems out of scope. Hence, I've added FIXME comments instead of refactoring them - https://github.com/chrisleekr/binance-trading-bot/pull/462/files#diff-ad373fc5bc2ac395a075a4ef44bdaebd17d66fc02707c5c07c83e1483199bab6R205
Let's test it out for another day and if there is no issue, I will merge in.
This is a very good fix @uhliksk @habibalkhabbaz
Let’s merge it! |
Description
Grid Trade #1
buy order is executed and max. open trades limit is reached, all otherGrid Trade #1
buy orders are cancelled. This will prevent overshooting the limit while it will allow to wait in multiple symbol for the first buy order to be triggered.Related Issue
#398, #457
Motivation and Context
This will solve the problem with more opened trades than allowed while also will fix the issue when buy orders in already open trades weren't allowed if the limit was exceeded.
How Has This Been Tested?
I tested changing the limit on the fly to open more trades and then limiting them to lower value. Then waiting if excess buy orders are cancelled and buy orders in already opened trades are allowed.
Screenshots (if appropriate):