danpaquin / coinbasepro-python

The unofficial Python client for the Coinbase Pro API
MIT License
1.82k stars 732 forks source link

Add error handling, fix broken unit tests #260

Open bpross opened 6 years ago

bpross commented 6 years ago

PROBLEM

  1. Currently this library does not support error handling for HTTP requests. This causes library clients to implement error handling on their end. A big problem with this is that clients do not have access to the response object, so clients are prevented from checking the status code. The only method for checking error states is to look for message in the response body.

  2. In their current form, unit tests are broken. The test_public_client.py file imports a library that is not listed in the requirements.txt file. If running tests in a virtualenv, py.test will fail to collect the tests. Additionally, test_get_historic_rates sends a request to GDAX with an invalid granularity, which causes a 400 to be returned.

SOLUTION

  1. Implement error handling for public_client and authenticated_client requests.
  2. Require python-dateutil in requirements.txt. Send valid granularity.

CHANGES

  1. Created exceptions.py. Mapped all reported HTTP status codes from GDAX to appropriate exceptions.
  2. Created _determine_response method to raise appropriate exception or return JSON body.
  3. Created unit tests to verify error handling functions correctly.
  4. Added README information about error handling.
  5. Added python-dateutil to requirements.txt to fix unit tests.
  6. Changed granularity to 21600 instead of 10000, so unit test will pass.
  7. Bumped version to 2.0, as error handling will cause issues with implementations based on previous version.
  8. Added myself to contributors.txt

ETC Addresses #245

bpross commented 6 years ago

@duffar12 @acontry @alex0527 @js931 please have a look

alimcmaster1 commented 6 years ago

Firstly thanks for a well defined PR and for adding the test dependency, I missed this here #207.

Changes generally look good to me at a first glance. One comment I would make those is in "public_client.py" do we have to define all the HTTP Status codes? Might be neater to use standard library such as https://docs.python.org/3/library/http.html, instead of declaring these?

Thanks

bpross commented 6 years ago

@alimcmaster1 yes, I will make that change. I did not know those were defined in the PSL.

danpaquin commented 6 years ago

This is great. It looks like there are a few oversights, but other than that this looks solid. I will make these changes myself if this is left for another week.

gdax/exceptions.py ~ Line 31 gdax/authenticated_client.py ~ Line 34

bpross commented 6 years ago

@danpaquin updated based on your concerns. Please let me know if there is anything else you want me to change.

Cheers.

danpaquin commented 6 years ago

Because this will cause braking changes, I believe we will need to table this feature for future use. I would rather push this into a separate branch as some users may not prefer this implementation.

danpaquin commented 6 years ago

There was a lot of good work done here so I'm reopening this to gauge interest in this error handling implementation. How does the community feel about error handling in general?

Would you find this useful?

bpross commented 6 years ago

I definitely find it useful and am using it in my current setup. Let me know if we want to move forward with this and I can fix the rebase issues and make any improvements that the community would see fit.

TheKeyboardKowboy commented 5 years ago

I would like to see error handling added.

danpaquin commented 5 years ago

I've hesitated on this since it's not backwards compatible.

Any thoughts from the community on this?

py-pirate-readonly commented 5 years ago

really like this. would like to see this PR updated for Coinbase-Pro, replacing "gdax" with "cbpro".

bpross commented 5 years ago

Updated things since this got a little out of date.

geegonzalez commented 5 years ago

Would love better error handling built into the library. Error handling is such an essential thing when working with API's.

Because of the current limitations I copy/pasted the library into my own project and hacked in error handling of my own (not very pretty, I know).

noah222 commented 5 years ago

I don't think the code here is being maintained anymore, but it is very fast already. The best way to handle errors is with a try, except block surrounding each api call. This will allow you to catch each error and deal with it based on your specific needs. In the past 30 days I have traded many thousands with this method, so it is stable.

lukastribus commented 3 years ago

@danpaquin I understand your reluctance to break the API, but I believe this is absolutely necessary. The library currently doesn't even raise an exception on simple authentication failures. It's very painful to debug strange behavior in your application, only to discover that a trivial authentication issue is causing this, because the library hides all errors.

vsai commented 2 years ago

@danpaquin Would definitely like to see error handling implemented. There are several errors that can come about based on API restrictions and limitations, and without it, we're hesitant about the robustness of our usage of this API.

For people to migrate their code is relatively simple, assuming they just replace all references of response with response.json() / response.text.