Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn non-200 HTTP responses into an error instead of trying to parse the body as a JSON-RPC response #1361

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

romac
Copy link
Member

@romac romac commented Sep 28, 2023

Closes: #1359

This PR checks for the status code to be 200 OK, if not it returns an error with the actual status code and the body of the response, but does not attempt to parse the response body as a JSON-RPC payload.

CometBFT will always set the status code to 200 OK and the content type to JSON for response which were successfully handled (which does mean that the JSON-RPC response itself is successful).

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@romac romac requested a review from mzabaluev September 28, 2023 14:46
rpc/src/client/transport/http.rs Outdated Show resolved Hide resolved
rpc/src/client/transport/http.rs Outdated Show resolved Hide resolved
rpc/src/error.rs Outdated Show resolved Hide resolved
@romac romac force-pushed the romac/handle-http-error branch from 7b3ae01 to bf9eead Compare September 28, 2023 15:00
@mzabaluev
Copy link
Contributor

Conditional compilation needs to be fixed for wasm, otherwise looks fine.

@romac
Copy link
Member Author

romac commented Sep 29, 2023

I will do this after we merge #1362

@romac romac force-pushed the romac/handle-http-error branch from 009bd89 to 064e4de Compare September 29, 2023 11:05
When the `reqwest` feature is disabled, this now falls back to `NonZeroU16`,
even though that variant is never actually created.

A better solution would be to feature-guard the error variant itself,
but thats not supported by `flex-error` at the moment.
@romac
Copy link
Member Author

romac commented Sep 29, 2023

Not the cleanest solution (see the commit message) but it's pretty much what we do already for source errors, so let's stick with that until we eventually move away from flex-error.

OK. I'm not a big fan of public enums with conditionally defined members, anyway, so the fallback approach is better. If the enum was private, that would be more disciplined, but still it causes a lot of fragility in match expressions.

@romac
Copy link
Member Author

romac commented Sep 29, 2023

I've tested this from Hermes against a public Injective RPC node which was throttling requests, and it works as expected. We get back the HTTP error instead of trying parse the empty response body.

@mzabaluev
Copy link
Contributor

Conditional compilation needs to be fixed for wasm, otherwise looks fine.

In fact, why do we build RPC stuff for wasm?

@romac
Copy link
Member Author

romac commented Sep 29, 2023

In fact, why do we build RPC stuff for wasm?

Because one can define a Client implementation which uses eg. the browser's fetch method and then compile all that to WASM.

@romac romac merged commit 43cce0b into main Sep 29, 2023
@romac romac deleted the romac/handle-http-error branch September 29, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP client should not try to parse body of response with non-2XX status code
2 participants