feat: impl ToRpcParams for serde_json::Value #1430
Conversation
| serde_json::Value::Array(a) => serde_json::to_string(&a), | ||
| serde_json::Value::Object(o) => serde_json::to_string(&o), | ||
| serde_json::Value::Null => Ok("[null]".to_string()), | ||
| serde_json::Value::Bool(b) => serde_json::to_string(&[b]), | ||
| serde_json::Value::String(s) => serde_json::to_string(&[s]), | ||
| serde_json::Value::Number(n) => serde_json::to_string(&[n]), |
There was a problem hiding this comment.
Something I feel uneasy about here is that it's no longer so clear to me that some values (number, bool, string) are treated as a single param whereas some values (array, object) are spread into multiple params (rather than also being treated as a single param).
I don't think I understood in the first place exactly what motivated the original PR for this, so I guess I'd still ike to understand: why is it useful to be able to pass a Value to an RPC method in this way?
There was a problem hiding this comment.
Yeah, I'm not completely convinced about this one myself but the rationale is easy-of-use for users instead of introducing a new type pattern on top of serde_json::Value to implement ToRpcParams. This should mostly be used by the client and somewhat redundant to our rpc_params![] macro
So the implementation is opinionated and the reason for that is that params must be represent as JSON object or JSON array that is way primitive types are serialized as an array of size 1 and array/object already are valid JSON-RPC params.
I'm also fine skipping the impl completely and let users deal with that.
There was a problem hiding this comment.
Yeah, I guess I'm thinking that our basic impls are pretty foolproof and hard to misuse, and things like this might lead to some more confusion (eg somebody apssing a Value in as a param, expecting it to be treated as a single param, but because it's an array it's treated as multiple params or whatever).
I'd be happiest leaving it to the user; somebody can implement a simple wrapper or function which takes a serde_json::Value and turns it into some MyType which has a custom impl of ToRpcParams to do whatever behaviour they prefer imo :)
but yeah like I said in the other PR, I'd have no objection either to just serde_json::Map implementing ToRpcParams for instance, if that helps anybody!
Superseeds #1426