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

feat: error handling overhaul #65

Merged
merged 4 commits into from
Feb 22, 2024
Merged

feat: error handling overhaul #65

merged 4 commits into from
Feb 22, 2024

Conversation

heilhead
Copy link
Collaborator

@heilhead heilhead commented Feb 15, 2024

Description

This reworks the RPC errors to be serializable and deserializable from the data field of JSONRPC error response. Both the HTTP and websocket clients in this crate make use of the new errors by deserializing relay RPC responses into strongly-typed errors.

Clients in other SDKs should implement their own reconstruction based on the following JSONRPC error response fields:

  • code: the general class of errors to distinguish between retryable and permanent errors;
  • data: the string tag specifying error kind within that class;
  • message: string message detailing the error.

The error codes are (mostly) consistent with the old error codes & the JSONRPC specs. What's changed:

  • Parameter validation error code changed from -32602 (invalid params) to a more generic -32600 (invalid request);
  • Payload size limit error code changed from -32700 (parse error) to -32600 (invalid request);

The rest of the changes are non-breaking, in terms of error codes.

How Has This Been Tested?

Existing tests, relay integration tests and also new tests covering error tags.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@heilhead heilhead force-pushed the feat/error-overhaul branch 2 times, most recently from 5719c65 to 803b7ca Compare February 15, 2024 18:03
@heilhead heilhead marked this pull request as ready for review February 16, 2024 13:30
relay_client/src/error.rs Outdated Show resolved Hide resolved
relay_rpc/src/rpc/error.rs Outdated Show resolved Hide resolved
relay_rpc/src/rpc/tests.rs Outdated Show resolved Hide resolved
}

#[test]
fn error_tags() {
Copy link
Contributor

Choose a reason for hiding this comment

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

imageimageimage

Rpc {
code: i32,
message: String,
data: Option<String>,
Copy link

@jakubuid jakubuid Feb 17, 2024

Choose a reason for hiding this comment

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

data field is not handled currently in Kotlin client. It should not break tho, would be just ignored.

Copy link

@jakubuid jakubuid left a comment

Choose a reason for hiding this comment

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

It shouldn't break but to be 100% sure can we try staging release first on the clients side?

Copy link

@llbartekll llbartekll left a comment

Choose a reason for hiding this comment

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

Same here, let's just share the staging url before the release, so we can test the clients manually.

Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

LGTM

relay_rpc/src/rpc/tests.rs Outdated Show resolved Hide resolved
@heilhead heilhead merged commit be9d7b2 into main Feb 22, 2024
8 checks passed
@heilhead heilhead deleted the feat/error-overhaul branch February 22, 2024 07:34
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.

5 participants