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

Make RpcError generic #692

Closed
gregdhill opened this issue Oct 19, 2022 · 5 comments · Fixed by #694
Closed

Make RpcError generic #692

gregdhill opened this issue Oct 19, 2022 · 5 comments · Fixed by #694

Comments

@gregdhill
Copy link
Contributor

I am unable to upgrade from 0.23.0 to 0.24.0 because we lost the RpcError type (here) propagated from jsonrpsee. We use this to specifically match custom errors for transaction invalidity and for when the background service needs to be restarted. Instead of converting this error into a string the RpcClientT trait should have a generic parameter for the error.

@jsdw
Copy link
Collaborator

jsdw commented Oct 19, 2022

We'll have to have a think about that; I do see that we lose some information now which sucks, but adding a generic parameter would propagate through the entire codebase again, so it'd definitely be preferably to avoid that if we can!

Perhaps we should box the error if possible into a Box<dyn Error> type thing, so at least you can recover the underlying error by a cast, and we can avoid the generic proliferation; what do you reckon?

@lexnv
Copy link
Collaborator

lexnv commented Oct 19, 2022

To quickly unblock the transition to 0.24.0 one could try to match those strings.
The error string should contain enough information, and if it doesn't we should point that for a jsonrpsee update.
It might not be ideal, but just a temporary fix until we roll a Box<dyn Err> as James mentioned.

@gregdhill
Copy link
Contributor Author

I want to avoid comparing strings for obvious reasons, but we also need to parse the JsonRPC error object in some cases. The approach you have suggested @jsdw makes sense to me!

@jsdw
Copy link
Collaborator

jsdw commented Oct 19, 2022

@gregdhill would you be able to check out #694 and confirm that it addresses your needs?

@gregdhill
Copy link
Contributor Author

LGTM, thanks @lexnv @jsdw!

@jsdw jsdw closed this as completed in #694 Oct 20, 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 a pull request may close this issue.

3 participants