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

Report HTTP failure codes as RPC errors if returned for an RPC call #585

Merged
merged 6 commits into from Feb 4, 2022

Conversation

marmistrz
Copy link
Contributor

At least in the Alchemy JSON RPC API over HTTP, rate limit errors are returned as HTTP 429. Currently these errors are converted to Error::Transport(String) and it's not possible to cleanly access the status code. Moreover, Error::Transport also means errors like failure to deserialize, whose handling will be completely different than handling of capacity limits or rate limit.

This PR changes representations of the errors returned in execute_rpc to rpc::error::Error. Since the HTTP error codes aren't any of the variants defined by jsonrpc-core, the error code will be ErrorCode::ServerError(i64).

cf. https://docs.alchemy.com/alchemy/documentation/error-reference

src/transports/http.rs Outdated Show resolved Hide resolved
Comment on lines 97 to 99
let code = ErrorCode::from(i64::from(status.as_u16()));
return Err(Error::Rpc(RPCError {
code,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of mixing HTTP error codes (i.e. transport specific) with the ones returned by actual JSON-RPC server. What if we have a conflict and conflate an JSON-RPC error code with some HTTP error code?

I'd rather introduce a richer Transport variant, with following fields:

struct TransportError {
  /// Transport implementation self-identification.
  /// 
  /// This can be used to determine the actual transport (HTTP, WS, etc). being used if it's not statically known.
  transport_id: String,
  /// Transport-specific error code.
  code: u64,
  /// Arbitrary, developer-readable description of the occurred error.
  message: String,
}

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON-RPC codes are to be negative, http/ws error codes are positive so there shouldn't be a clash, so I'm fine with both approaches. It makes some sense to use a different variant for transport and rpc errors because the former are much more retriable than the latter in many cases.

This was roughly my initial idea, however, Transport is not used in a compatible way right now as it's also used to report errors like

.map_err(|err| Error::Transport(format!("failed to build client: {}", err)))?;
or
.map_err(|err| Error::Transport(format!("failed to read response bytes: {}", err)))?;
.

We could either introduce a new variant or use code: Option<u64>.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON-RPC codes are to be negative, http/ws error codes are positive so there shouldn't be a clash

JSON-RPC codes can be anything actually, the spec just defines a few reserved ranges which are negative.

This was roughly my initial idea, however, Transport is not used in a compatible way right now as it's also used to report errors like

We can either assign a code to each of such error (and expose constants with that values), use a "catch-all" 0 code for that or use Option<u64> as you suggested - I'm fine with either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a slightly different implementation given that we're unlikely to get a developer-defined message and a code. I also skipped the transport_id because it would be a lot of boilerplate.

@marmistrz
Copy link
Contributor Author

Ping, what do you think about the new implementation?

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Please fix the tiny warnings that can be seen during the CI and we're good to go.

@@ -1,6 +1,8 @@
//! Supported Ethereum JSON-RPC transports.

pub mod batch;
use crate::error::TransportError;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you feature gate the import or use a direct path to prevent warnings?

src/transports/ws.rs Outdated Show resolved Hide resolved
marmistrz and others added 3 commits February 4, 2022 14:26
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
@tomusdrw tomusdrw enabled auto-merge (squash) February 4, 2022 16:43
@tomusdrw tomusdrw merged commit 19e577a into tomusdrw:master Feb 4, 2022
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.

None yet

2 participants