Optimize serialization for client parameters #864
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
core/src/client/mod.rs
Outdated
| $( | ||
| __params.push($crate::client::__reexports::to_json_value($param).expect("json serialization is infallible; qed.")); | ||
| __params.insert($param).expect("json serialization is infallible; qed."); |
There was a problem hiding this comment.
Just FYI, Json serialisation isn't infallible?
For example object keys just be strings, so trying to serialize eg a Map<bool, Foo> would fail.
Happy to sort this out in a future PR though to avoid bloating this one; I'd lean towards doing something like I did in subxt and having the macro return a Result, so that the caller can unwrap or "?" It.
Whatcha think @niklasad1 ?
There was a problem hiding this comment.
yepp, you are correct @jsdw
as long as we document that it will panic if serialization fails I'm fine with it and remove the incorrect expect :)
it's intended to work as the serde_json::json! macro but we can do what you did in subxt here as well.
There was a problem hiding this comment.
Yep, it makes sense. For this PR have changed the except message and added documentation. Will come up with a followup PR to return a Result here
There was a problem hiding this comment.
That makes sense @niklasad1! I pondered this and, indeed, it's only programmer error that can led to the panic (ie providing fallible things to the macro), so I'm ok with keeping the panic behaviour and not returning a result!
Maybe subxt's version should be the same..
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
| @@ -225,10 +234,11 @@ impl ClientT for HttpClient { | |||
| .await | |||
| } | |||
|
|
|||
| async fn batch_request<'a, R>(&self, batch: Vec<(&'a str, Option<ParamsSer<'a>>)>) -> Result<Vec<R>, Error> | |||
| async fn batch_request<'a, R>(&self, batch: BatchRequestBuilder<'a>) -> Result<Vec<R>, Error> | |||
There was a problem hiding this comment.
yeah, this API is a bit different than the others i.e, takes the concrete type BatchRequestBuilder instead of ToRpcParams I think that makes sense as it supports both positional params (array) and named params (map) but not possible for folks to use there own impl of this.
Indeed it makes this API easier to understand and cleaner, cool
| let (value, _value_type) = pair.1; | ||
| quote!(#name, #value) | ||
| }); | ||
| quote!({ |
| #[derive(Clone, Debug, Default)] | ||
| pub struct BatchRequestBuilder<'a>(Vec<(&'a str, Option<Box<RawValue>>)>); | ||
|
|
||
| impl<'a> BatchRequestBuilder<'a> { |
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub trait ToRpcParams { |
|
Left a few comments for small tweaks/reminders but overall love it! |
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: James Wilson <james@jsdw.me>
Co-authored-by: James Wilson <james@jsdw.me>
| impl ToRpcParams for ArrayParams { | ||
| fn to_rpc_params(self) -> Result<Option<Box<RawValue>>, Error> { | ||
| if let Some(json) = self.0.build() { | ||
| RawValue::from_string(json).map(Some).map_err(Error::ParseError) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
| } |
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
|
@lexnv anything more to do before merging this one? |
|
Nope, we can go ahead and merge this for now 😄 |
This PR adds a generic way of providing RPC parameters to the clients.
The previous method required the users to serialize their parameters
to a hardcoded
serde_json::Value, which is allocation heavy, especiallywhen the valid parameters are already constructed.
To constrain users to correct parameter types, two alternative methods
are built upon the internal
ParameterBuilder: theUnnamedParamsandNamedParams.The former is the equivalent of the json array, while the latter represents a
json map key-value.
The
ClientTinterface has been updated to receive theToRpcParamstraitas parameters, giving users an escape route that provides a raw constructed
value. This has the benefit of avoiding a
Raw->json::Value->Rawconversion.
To accommodate this change, the batch parameters need to receive
Rawinputs.This is because users wish to provide some methods without parameters
(deduced to the
()empty tuple type), while some methods need to have named orunnamed inputs (
UnnamedParamsandNamedParams).For improving the user's experience, a batch parameter builder is introduced to
abstract the low-level interface required by submitting batch requests.
Closes #862.