-
Notifications
You must be signed in to change notification settings - Fork 19
Better way of JsonRpseeError creation
#539
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
Conversation
|
The problem with error creation is not that it's too verbose, but rather that we don't have a proper contract between our UI and the RPC server to be able to display the errors with a sensible custom logic. |
|
Let's discuss with @sesh92 |
|
@sesh92, @dmitrylavrenov let's spend some time on this |
ae89750 to
d5213a6
Compare
a79d633 to
b5e21be
Compare
b5e21be to
427e927
Compare
eec29a9 to
956b488
Compare
956b488 to
ebbe219
Compare
|
Please make sure we don't lose existing error message string customizations when we migrate to the new errors. We still need them. |
c01ff6c to
634610f
Compare
634610f to
e0d27c4
Compare
|
I think this needs more work but we need other things done. Let's put this on hold and move on. I also need some time to capture our requirements here more precisely, so we probably should not waste our time bashing this over and over. Alternatively, we can merge as-is, but it might be worse than where we started... In a sense that what we had before was clearly a mess, but it was compact and could be helped relatively simply with helpers. This has some logic in it, but no explicit constraints, and is much more verbose. Yet it still feels like the logic is not clear on how we would implement this, to me it feels like reading the code from the position of the frontend engineer has become even more difficult. To sum up what I've figured here so far:
Anyhow, this one task has taken us too much effort already, so let's put a pin in it. We might want to do a prototype with (5), but we have better ways to spend our time now, as this is not a blocker for anyone. |
|
@dmitrylavrenov I managed to fix most of the issues I observed while writing #539 (comment), please check out my changes and adjust the rest of the codebase accordingly. |
b85d61e to
d7376b7
Compare
|
How come we have merge conflicts? Let's fix them! |
MOZGIII
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
bd3ebc4 to
6bc2148
Compare
Closes #332
The details of
JsonRpseeError- https://github.com/paritytech/jsonrpsee/blob/f83c6c55a8e2c36172cea8a5c1145e9a3b9ab377/core/src/error.rs#L57Found 2 options to create
JsonRpseeError:At the current moment I've implemented an example with first option as it's not required a lot of additional codes with our current implementation.
@MOZGIII What do you think? Can I continue with the first one?