search

sammchardy / python-binance

Binance Exchange API python implementation for automated trading
https://python-binance.readthedocs.io/en/latest/
MIT License
5.96k stars 2.19k forks source link

API v1 vs v3 - deprecating or upgrading for unsigned endpoints #624

Open ttamg opened 3 years ago

ttamg commented 3 years ago

Opening a separate issue for this as discussed on PR #622.

The issue is that ...

client.stream_get_listen_key()
client.stream_keepalive(listenKey)
client.stream_close(listenKey)

... are implemented on v1. As their signed is False it picks up v1. For endpoints with signed is True, they use the correct v3 API endpoints.

As @mfiro noted, the issue shouldn't be solved by changing signed to True because this only should be used when a method needs a signature (those which have HMAC SHA256 next to their endpoint address).

Originally posted by @mfiro in https://github.com/sammchardy/python-binance/issues/622#issuecomment-734411129

Opening to work out the best approach for dealing with this issue.

ttamg commented 3 years ago

@mfiro I've done some further digging:

  1. All of the /api/v1 endpoints appear to have been deprecated. The official docs show that there are no current endpoints documented on v1 anymore. All seem to have been moved to /api/v3. A Ctrl-F search on the official Binance API docs confirm this.

  2. This means that the issue is not just on the stream_get_listen_key etc endpoints but affects most or all of the public (not signed) endpoint. For example just looking at client.ping() makes a call to /api/v1/ping but the docs talk about v3 - I count 17-20 endpoints in python-binance that are on the legacy v1 which is no longer in the official docs

  3. A simple thing would be to add a deprecation warning as you said on these endpoints.

  4. In terms of making the upgrade, it seems that perhaps the way to update is a simple change from PUBLIC_API_VERSION = 'v1' to PUBLIC_API_VERSION = 'v3' ... but it's a potentially a breaking change. It is hard for me to tell if the response has changed between v1 and v3 for some of the endpoints. There probably have been some changes. Either way I think we would need to do a side-by-side test on the response objects on every affected endpoint? If there are any differences then we need to find a way to provide legacy and new endpoints for a time. And even if there are not differences we can see, it might be safer to allow an extra parameter legacy=True or similar that will still use V1.

Those are my thoughts so far ...

mfiro commented 3 years ago

Your're right. I can't find also /api/v1/ endpoints anymore in the documentation.

I've just found this PR and this issue regarding this matter. I don't know if it covers all the affected endpoints, but I guess it's a good start. I have tested a few of them like ping() they work in both v1/v3 endpoints, but I agree with you. They need to be tested closely.

ttamg commented 3 years ago

OK that's great. That's the same issue then.

I'm not brilliant with @patch in tests but I was wondering if a test could be easily written to hit all the endpoints using v1 and v3 side by side instantaneously by patching the variable holding the v1 and highlight any diff between them....

I've started a little script to hit the endpoints in turn as an idea to try to do some live tests on the package. Could come in useful.

ttamg commented 3 years ago

Hi @mfiro - is this issue something to work on or shall it be closed then?

If to be worked on, my idea was to create a script that calls the V1 and V3 simultaneously using asyncio or unsync and then compare the JSON response and show the differences. That's a nice way to check there are no fundamental differences.

An idea.

mfiro commented 3 years ago

Hi, your idea sounds good to me. Because these v1 endpoints are already deprecated and who knows when they will stop working. I haven't looked the unit tests of this project sofar, but I guess it should be done there. At the moment I'm busy. But if you are going to start, I would appreciate.

ttamg commented 3 years ago

I had a look at the tests. They are thin should we say! There's not really any decent test coverage I could see. I guess it is hard with an API wrapper project like this. My view would be to have a live set of tests that run against the live API (a developer would need to have a Binance account to do this) and if they run rather then fall over then that's a good check that at least they still work. It's more of a smoke test.

I'm not sure what the maintainers want to do though.

mfiro commented 3 years ago

I see, I think some endpoints like market data endpoints doesn't need keys. So you can test it with dummy api/secret keys. I know what you mean, trading functions are not easy to test. This is however not related to this issue. For this issue, one might add the v1 vs v3 comparison like a method there to check if there's any difference in responses. Full coverage unit tests is definitely more complicated and takes more time.

ttamg commented 3 years ago

sammchardy/python-binance/issues/768 has just been opened that deals with removal of the old Endpoints in August. Suggest If that is being progressed, we close this issue, or create a link somehow. Seems we need to do something on this now! What do you think @mfiro ?