coinbasepro-python icon indicating copy to clipboard operation
coinbasepro-python copied to clipboard

Add error handling, fix broken unit tests

Open bpross opened this issue 6 years ago • 16 comments

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 avatar Feb 25 '18 20:02 bpross

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

bpross avatar Feb 25 '18 20:02 bpross

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

alimcmaster1 avatar Feb 25 '18 22:02 alimcmaster1

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

bpross avatar Feb 25 '18 22:02 bpross

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

danpaquin avatar Mar 22 '18 02:03 danpaquin

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

Cheers.

bpross avatar Mar 26 '18 00:03 bpross

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 avatar Aug 19 '18 15:08 danpaquin

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?

danpaquin avatar Aug 24 '18 03:08 danpaquin

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.

bpross avatar Aug 24 '18 16:08 bpross

I would like to see error handling added.

TheKeyboardKowboy avatar Oct 27 '18 00:10 TheKeyboardKowboy

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

Any thoughts from the community on this?

danpaquin avatar Nov 26 '18 19:11 danpaquin

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

py-pirate-readonly avatar Nov 29 '18 06:11 py-pirate-readonly

Updated things since this got a little out of date.

bpross avatar Nov 30 '18 02:11 bpross

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).

geegonzalez avatar Jun 02 '19 05:06 geegonzalez

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.

noah222 avatar Jun 02 '19 13:06 noah222

@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.

lukastribus avatar Dec 20 '20 19:12 lukastribus

@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.

vsai avatar Jan 03 '22 11:01 vsai