-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add Binance websocket support #211
Conversation
d3ce995
to
cb30875
Compare
I was doing some profiling of this and I found that in the case where the ticker between a coin and BNB doesn't exist (for example ETHBNB) the I added some prints to show how drastic this is. Here is the output before:
And after:
From 3 seconds to basically 0! How I fixed it was like this, but I'm sure there are any number of ways to cache this value (possibly with a timeout in case they add the ticker later).
I'll test next with a ticker that exists. Edit: When ticker exists loop time is around |
I tried this brunch and I got stuck several times when jumping from one currency to another.
At the same time, there was no order on binance. and there all the currency was USDT upd; Hmm. I hurried to turn it off. this transaction went through. just walked for a long time. |
@GrigorovskyA yeah that happens sometimes, I don't think this PR should have changed anything in that area |
Yeah, I agree, just keeping up with my observation of the work of this branch. |
Now orders are being fetched using the websocket, which means the |
It happens sometimes that the bot got stuck in the loop to get balance in
It didn't happen without this PR (at least, for me), so I guess this might be related? |
dc9cf65
to
898093b
Compare
@Blaklis ah good catch. I've added some code to force a balance update when we want to be sure we've got the right values. I've also added more debug logs, that will show up in the log file but not in the console. Hopefully they'll make issues like this easier to debug. |
For me it shows the following:
Is it normal for it to be running |
@cornytrace yes, that's part of the |
Maybe it's an idea to only print debug to log or console when the program is started with a --debug option or similar. |
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.
Ok, while reviewing this I noticed that a bunch of seemingly unrelated stuff was also changed - for the better, I believe. Although these changes don't necessairly belong in this PR, they should be accepted and it would probably be too much of a hassle to take them out now. In the future we should probably seperate these changes though... I left some comments, but this looks generally ok to me.
Started testing...
@@ -21,7 +21,7 @@ def __init__(self, logging_service="crypto_trading"): | |||
|
|||
# logging to console | |||
ch = logging.StreamHandler() | |||
ch.setLevel(logging.DEBUG) | |||
ch.setLevel(logging.INFO) |
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.
Makes sense I guess, but is this the right PR for this?
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.
You're right it's probably not, although we don't use any DEBUG
logs anywhere else so it won't affect anything. My logic here was that with the introduction of websockets there is the possibility of hard to debug issues being reported down the line, and it would be good to log as much as possible so that we get more information from users reporting issues. Logging all this extra information to the console isn't very helpful which is what this change prevents
I also had the problem with the updated version of python-binance. Seems like it happens less frequently, but it's still a problem. We might need to investigate this and at least mitigate for the rare cases it happens. |
Here is my changes based on this PR for the issue (infinity waiting order complete) mentioned above, and I didn't meet the issue at least in last 7 days: The main idea is check the order status manually instead of waiting the order status change event. But please take it in mind that it also includes below changes:
|
Unfortunately I can confirm that the problem of the bot not receiving user socket messages is still not solved. The problem seems to be that the websocket somehow times out or is unreliable. |
@j-waters can you rebase your change on top of master? PR for rebase is open in: j-waters#1 |
Thanks @erenatas, I'll try and look at that properly soon, if anyone else that's familiar with this PR could look at it as well I'd appreciate it as I'm still rather busy |
I'll have a look at it too. |
That should be rebased, thank you very much for the help @erenatas. I haven't tested it too thoroughly and I haven't added any of the previous suggestions yet, again please make a PR on my fork or take over the PR completely if you'd like to try fixing some of the problems I'm still rather busy Also is it just me or is the multiple_coins strategy a bit broken? |
Squashed as some commits were broken by the rebase: Add stream manager class Remove all_tickers Add method for starting all mark price socket Add handler for closing websockets on exit whitespace refactor Refactor naming and move sleep timer Update connect_args Disable check for same thread Add initial implementation of websockets for getting all mark prices Co-authored-by: Ryan Sonshine <sonshine@amazon.com>
Currently using this branch and its working great 👍 thanks guys |
Would be glad if you can give credit as a co-author in commit message. Thanks. |
@j-waters the multiple is broken |
@bloodf This doesn't seems to be related to this pull request. Also, please be specific next time, instead of just saying it's broken. |
need to change this print(
f"{datetime.now()} - CONSOLE - INFO - I am scouting the best trades. "
f"Current coin: {current_coin + self.config.BRIDGE}",
end="\r",
) to this print(
f"{datetime.now()} - CONSOLE - INFO - I am scouting the best trades. "
f"Current coin: {current_coin} {self.config.BRIDGE}",
end="\r",
) |
This is definitely not related to this PR. |
@j-waters when bot jumps between coins its stuck , but not always , and the logs displays that the bot is trying to buy 2021-04-29 15:42:57,871 - crypto_trading_logger - DEBUG - Waiting for order 622088162 to be created |
me too pip install --upgrade --no-deps --force-reinstall git+git://github.com/sammchardy/python-binance.git and restarting the bot cant fix this bug |
Closing this in favor of #312. To see any changes from my PR: j-waters/binance-trade-bot@websockets...erenatas:websockets-eren |
Continuation of #205, thank you so much to @ryansonshine for the initial implementation!
A cache of the current ticker prices and the user's current balance are now stored in the
BinanceApiManager
. This cache is updated live by two webscoket connections.Eventually we'll also be able to use this to wait for trades to complete, but I don't want to add anything that will conflict with #195 too much.
In the event of the socket disconnecting, it will attempt to reconnect. I had to extend the binance library a bit to make sure that was working. If it can reconnect, it will clear the caches to make sure new data is fetched, just in case a socket event was missed. If it can't, currently the bot will just continue running without the sockets. Since ticker prices will never update, the bot won't buy anything, so this won't do too much damage. I'll think of how to best handle this, any ideas would be appreciated.
This massively reduces the number of API calls we have to make, and makes scouting so much faster. The
get_fees
method also runs instantly now.