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

Rpc Error handling #724

Closed
sander2 opened this issue Nov 21, 2022 · 4 comments
Closed

Rpc Error handling #724

sander2 opened this issue Nov 21, 2022 · 4 comments

Comments

@sander2
Copy link
Contributor

sander2 commented Nov 21, 2022

#634 generalized the code over rpc implementations. This PR changed the subxt::Error::Rpc error to a generalized one. First to a string, later to a dyn std::Error:

subxt/subxt/src/error.rs

Lines 111 to 118 in 15ffbb6

pub enum RpcError {
// Dev note: We need the error to be safely sent between threads
// for `subscribe_to_block_headers_filling_in_gaps` and friends.
/// Error related to the RPC client.
ClientError(Box<dyn std::error::Error + Send + Sync + 'static>),
/// The RPC subscription dropped.
SubscriptionDropped,
}

The problem is that this removes our ability to react to different rpc errors. For some errors we can recover, such as the POOL_TOO_LOW_PRIORITY error that we get when two connections use the same nonce at the same time. Other errors are unrecoverable, such as JsonRpseeError::RestartNeeded. Would it be possible to make subxt::Error generic over the rpc error type so that we can use proper pattern matching in our client?

@sander2
Copy link
Contributor Author

sander2 commented Nov 22, 2022

Currently using a fork that makes the error concrete again. Obviously this will be a problem for non-jsonrpc transport implementations though. Ideally, the ClientT should take an associated Error type, and the global subxt::Error struct should be generic over the TransportError. That's quite a refactor, though..

@niklasad1
Copy link
Member

niklasad1 commented Nov 22, 2022

@sander2

The current approach is that you have to downcast the ClientError(Box<dyn std::error::Error + Send + Sync + 'static> to jsonrpsee::Error such as this which is not super nice but that's intention IIRC.

or am I misunderstanding something?

@jsdw
Copy link
Collaborator

jsdw commented Nov 22, 2022

Currently using a fork that makes the error concrete again. Obviously this will be a problem for non-jsonrpc transport implementations though. Ideally, the ClientT should take an associated Error type, and the global subxt::Error struct should be generic over the TransportError. That's quite a refactor, though..

Yeah; It was so nice to be able to remove a generic param from the error type (which infests and complicates everything else) that I'd be somewhat reluctant to put it back!

@sander2
Copy link
Contributor Author

sander2 commented Nov 22, 2022

Thanks, I wasn't aware of the downcast function - that'll solve my issue. Sorry for the noise!

@sander2 sander2 closed this as completed Nov 22, 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

No branches or pull requests

3 participants