chore: use jsonrpsee to support JSON-RPC v2 instead of manual implementation#2010
chore: use jsonrpsee to support JSON-RPC v2 instead of manual implementation#2010
jsonrpsee to support JSON-RPC v2 instead of manual implementation#2010Conversation
jsonrpsee to support JSON-RPC v2 instead of manual implementation
netrome
left a comment
There was a problem hiding this comment.
Nice stuff. Interesting mock implementation. I'll play a bit with that one and get back if I have any concrete ideas for how it can be simplified. Anyway not blocking.
There was a problem hiding this comment.
After looking some more I'm skeptical about using jsonrpsee as a dependency. Did you evaluate other alternatives? I'd love to know if this really is the best there is due to how awkward it is to work with it, as evident by the MockJsonRpcClient struct.
I'm very surprised such an established crate doesn't have a cloneable error type. I did look deeper into the result type and they do seem to just have overlooked this. Some result types are Clone, but for example this simple one doesn't implement it:
/// Represent a request that failed because of an invalid request id.
#[derive(Debug, thiserror::Error)]
pub enum InvalidRequestId {
/// The request ID was parsed as valid ID but not a pending request.
#[error("request ID={0} is not a pending call")]
NotPendingRequest(String),
/// The request ID was already assigned to a pending call.
#[error("request ID={0} is already occupied by a pending call")]
Occupied(String),
/// The request ID format was invalid.
#[error("request ID={0} is invalid")]
Invalid(String),
}Naturally this is open source so nothing negative about the authors or the crate itself. I'm happy it exists and that people are sharing these implementations in the public. I just don't think this is an acceptable standard as a dependency I'd like to use in our project.
netrome
left a comment
There was a problem hiding this comment.
Okay they had legit reasons for not exposing Clone on their top-level Error...
gilcu3
left a comment
There was a problem hiding this comment.
LGTM!
Sad to see mockall go, it looked pretty nice :)
netrome
left a comment
There was a problem hiding this comment.
Thanks for incorporating the suggestion 🙏
closes #2009