search

michaelhly / solana-py

Solana Python SDK
https://michaelhly.github.io/solana-py
MIT License
1.01k stars 260 forks source link

Custom solana-py exceptions #151

Closed broper2 closed 2 years ago

broper2 commented 2 years ago

Has it been considered to raise exceptions custom to solana-py library instead of generic HTTPError created in:

https://github.com/michaelhly/solana-py/blob/f38f01a3c34ddc2d2f67a56d104246e458644717/src/solana/rpc/providers/core.py#L54

This may also be useful in timeout scenario. It would be a simple change to wrap provider code in decorator catching and converting into custom exception. Then clients and classes composed with solana library can more specifically catch exceptions.

Would be happy to create PR, let me know your thoughts.

michaelhly commented 2 years ago

@kevinheavey what do you think? Feel free to push up a PR to have us look :)

kevinheavey commented 2 years ago

@broper2 definitely interested to hear more about what we might catch with this. Most of the time when the RPC returns an error of some kind the status code is still 200 and the error information lives in the JSON itself.

As for the specific timeout question, requests and httpx both appear to raise a ConnectionError if the request times out (though the requests docs suggest otherwise). This happens whether or not we use .raise_for_status()

broper2 commented 2 years ago

@kevinheavey @michaelhly Definitely have noticed the 200 status code and error json with invalid input to request. This behavior is clearly by design and I think it makes sense for solana-py client to pass that up to caller. I am more talking about flaky, unexpected issues that can arise with HTTP requests.

For specific example, I have seen timeout behavior raise a ReadTimeout exception (see below logs) in my application. My WIP idea is to put decorator around http provider function with POST call, catching RequestException type and converting to solana-py internal exception which captures same info. Then users of library can have a single exception type and interface to work with. I'll start working on PR unless any objections from the audience can save me some time. Thanks!

7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/solana/rpc/providers/http.py", line 21, in make_request 7:52:39 AM web.1 | raw_response = requests.post(request_kwargs, timeout=self.timeout) 7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/requests/api.py", line 117, in post 7:52:39 AM web.1 | return request('post', url, data=data, json=json, kwargs) 7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/requests/api.py", line 61, in request 7:52:39 AM web.1 | return session.request(method=method, url=url, kwargs) 7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/requests/sessions.py", line 542, in request 7:52:39 AM web.1 | resp = self.send(prep, send_kwargs) 7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/requests/sessions.py", line 655, in send 7:52:39 AM web.1 | r = adapter.send(request, **kwargs) 7:52:39 AM web.1 | File "/Users/brianroper/.virtualenvs/solana-tracker/lib/python3.8/site-packages/requests/adapters.py", line 529, in send 7:52:39 AM web.1 | raise ReadTimeout(e, request=request) 7:52:39 AM web.1 | requests.exceptions.ReadTimeout: HTTPSConnectionPool(host='api.mainnet-beta.solana.com', port=443): Read timed out. (read timeout=10)

kevinheavey commented 2 years ago

@broper2 is the problem that it's hard to know what exception to catch under the current workflow? I don't get what you're looking for but it doesn't sound insane so maybe this will make more sense to me when your PR is done 😅

broper2 commented 2 years ago

@kevinheavey @michaelhly np, raised PR #152. To summarize, the idea is to have general, custom exceptions raised from solana-py clients. The client RPC implementation should be black-box to callers, it is better if the callers do not need to dig thru client code to know what errors they should catch if they need to do that.

Consider if solana-py moved away from HTTP provider to another way to implement RPC. The exceptions raised would most likely no longer be of "requests" library. Then all code dependent on solana-py would need to be updated. Raising exceptions native to solana-py eliminates this coupling.

BTW, totally open to changes to namings and error messages :)