-
Notifications
You must be signed in to change notification settings - Fork 195
unify usage of JSON via Box<RawValue> #1545
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
Mostly LGTM! I wonder if we can avoid all of the double quoted strings by using Also a small Q about the subscriptionerror! |
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.
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
core/src/server/subscription.rs:504
- Consider using structured JSON construction (e.g., the serde_json::json! macro) instead of string formatting to build JSON messages; this can help reduce potential formatting issues.
let json_str = format!("{\"jsonrpc\":\"2.0\",\"method\":\"{method}\",\"params\":{\"subscription\":{sub_id},\"result\":{result}}}");
It was much faster
r#"{{"jsonrpc":"2.0","method":"{method}","params":{{"subscription":{sub_id},"{result_or_err}":{result}}}}}"#, | ||
) | ||
} | ||
SubscriptionMessageInner::NeedsData(result) => serde_json::value::to_raw_value(&SubscriptionResponse::new( |
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.
This was actually faster than the manual stuff above
jsonrpsee_types_serialize_sub_manual
time: [177.53 ns 177.85 ns 178.23 ns]
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
jsonrpsee_types_serialize_sub_serde
time: [90.732 ns 91.579 ns 92.648 ns]
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe
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.
Nice! Liking the to_raw_values and such :D
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.
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
core/src/server/method_response.rs:264
- [nitpick] Using RawValue::NULL directly can reduce code clarity. Consider defining a named constant for an empty JSON message to enhance maintainability and readability.
json: RawValue::NULL.to_owned(),
All of the examples compile and run. A few lines of code had to be changed to account for upstream changes in `jsonrpsee`: paritytech/jsonrpsee#1545
Builds on-top of #1521
This PR unifies that Box is used everywhere instead of having untypes JSON Strings with a the native Rust String type.
Some additional changes that I got annoyed/needed for this work properly: