remove CallError
#1087
remove CallError
#1087
Conversation
niklasad1
force-pushed
the
na-remove-call-error
branch
from
8f0a165
to
107d867
Compare
types/src/params.rs
Outdated
| @@ -382,6 +384,10 @@ impl<'a> Id<'a> { | |||
| } | |||
| } | |||
|
|
|||
| fn invalid_params(e: impl ToString) -> ErrorObjectOwned { | |||
| ErrorObject::owned(ErrorCode::InvalidParams.code(), e.to_string(), None::<()>) | |||
There was a problem hiding this comment.
strictly from the jsonrpc spec:
code | message | meaning
-32602 | Invalid params | Invalid method parameter(s).
we are using -32602 => message == e: impl ToString
There was a problem hiding this comment.
but it was like that before this PR as well, it's picking hair I'm fine having a slightly better error message than "Invalid Params" though we could modify it in another and add this extra data in the data field instead of the message.
There was a problem hiding this comment.
Doesn't feel like a big deal (I'd expect people to match on the code), but having some more detailed thing inthe data field sounds more "correct" (would it be annoying for us to do this?)
There was a problem hiding this comment.
No, just update some tests I can do it in this PR.
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
| @@ -117,17 +119,17 @@ impl<'a> Params<'a> { | |||
| } | |||
|
|
|||
| /// Attempt to parse all parameters as an array or map into type `T`. | |||
| pub fn parse<T>(&'a self) -> Result<T, CallError> | |||
| pub fn parse<T>(&'a self) -> Result<T, ErrorObjectOwned> | |||
There was a problem hiding this comment.
we could have introduced type InvalidParams here but it is JSON-RPC error object it didn't make sense to have a separate type for that
server/src/tests/helpers.rs
Outdated
| Result::<_, Error>::Ok("ok") | ||
| .register_async_method::<Result<&str, MyAppError>, _, _>("should_ok_async", |_p, ctx| async move { | ||
| ctx.ok()?; | ||
| Ok("ok") |
There was a problem hiding this comment.
Nit: maybe consistify these generic ones with what you did below with §Result::<_, MyAppError>::Ok(..)`
| impl CallError { | ||
| /// Create `CallError` from a generic error. | ||
| pub fn from_std_error<E>(err: E) -> Self | ||
| where | ||
| E: std::error::Error + Send + Sync + 'static, | ||
| { | ||
| CallError::Failed(err.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ah ok, so previously user errors were being turned into thie Failed variant, and now the user describes how to turn them into an ErrorObjectOwned basically? This makes a lot of sense to me; give users the ability to set the error code etc :)
There was a problem hiding this comment.
Yeah, this was a way to easily convert any StdError into a ErrorObject and let jsonrpsee pick the error code.
This was a bad idea it's better that folks take care of the error code etc explicitly.
Close #1085
One downside of this is that
jsonrpsee::core::Errorcan't be used in the proc macro API and I worked around that by changingtype RpcResult<T> = Result<T, ErrorObjectOwned>The reason why ☝️ is because the
IntoResponserequires the error to implementInto<ErrorObjectOwned>which ends up being really weird to implement fromError variantsthat isn't really a JSON-RPC v2 spec error but something else like I/O etc.Thus, it's more explicit for users to implement
Into<ErrorObjectOwned>themselves which is explicit and more understandable.