coinbasepro-python
coinbasepro-python copied to clipboard
Add error handling, fix broken unit tests
PROBLEM
-
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. -
In their current form, unit tests are broken. The
test_public_client.py
file imports a library that is not listed in therequirements.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
- Implement error handling for public_client and authenticated_client requests.
- Require
python-dateutil
inrequirements.txt
. Send valid granularity.
CHANGES
- Created
exceptions.py
. Mapped all reported HTTP status codes from GDAX to appropriate exceptions. - Created
_determine_response
method to raise appropriate exception or return JSON body. - Created unit tests to verify error handling functions correctly.
- Added README information about error handling.
- Added
python-dateutil
torequirements.txt
to fix unit tests. - Changed granularity to 21600 instead of 10000, so unit test will pass.
- Bumped version to 2.0, as error handling will cause issues with implementations based on previous version.
- Added myself to contributors.txt
ETC Addresses #245
@duffar12 @acontry @alex0527 @js931 please have a look
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 yes, I will make that change. I did not know those were defined in the PSL.
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 updated based on your concerns. Please let me know if there is anything else you want me to change.
Cheers.
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.
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?
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.
I would like to see error handling added.
I've hesitated on this since it's not backwards compatible.
Any thoughts from the community on this?
really like this. would like to see this PR updated for Coinbase-Pro, replacing "gdax" with "cbpro".
Updated things since this got a little out of date.
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).
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.
@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.
@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.