rust-bitcoin / rust-bitcoincore-rpc

Rust RPC client library for the Bitcoin Core JSON-RPC API.
343 stars 256 forks source link

Use `serde_json::value::to_raw_value` in Client::call() #324

Closed romanz closed 6 months ago

romanz commented 11 months ago

Since MSRV is 1.56.1: https://github.com/rust-bitcoin/rust-bitcoincore-rpc#minimum-supported-rust-version-msrv

apoelstra commented 11 months ago

For context: this comment appears in https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/180 and the MSRV incompatibility seems to come from us pinning a version of serde_json somewhere in our dep tree.

This function seems to be introduced in serde_json 1.0.52. We should update the dependency in Cargo.toml from "1" to this value. (Furthermore, we should bisect backward to find out what our actual minimum dependency versions are. I think this crate is riding on rust-bitcoin's value for serde and probably others, but it shouldn't be.)

As a second thought, in rust-jsonrpc we have some hacky stuff related to raw values and maybe we should be using this new method as well.

tcharding commented 7 months ago

Is this live still @romanz? What problem is it trying to fix, does the problem still exist? Please note we are on MSRV of 1.56.1 now and also have MSRV dependency pinning fixes in #338. Thanks.

romanz commented 6 months ago

What problem is it trying to fix, does the problem still exist?

IIRC, it shouldn't change the functionality - only simplify the existing code a bit. https://docs.rs/serde_json/1.0.108/src/serde_json/raw.rs.html#284-290

I have thought to suggest this change when reviewing the code at: https://github.com/rust-bitcoin/rust-bitcoincore-rpc/blob/33293a57b3411765b654e8ad06081f9159cd1af9/client/src/client.rs#L1319

If it's not relevant, I can close this PR.

tcharding commented 6 months ago

Ah yeah, I've spent more time in the RawValue docs since I asked that question. I believe you are correct and this is refactor only.

tcharding commented 6 months ago

I took the liberty of editing the PR description for you @romanz because the MSRV has bumped since this PR was opened. The commit message doesn't include the msrv so we are good to go. Thanks man!

romanz commented 6 months ago

LGTM, thanks @tcharding!

tcharding commented 6 months ago

I just noticed that there is another instance of this in log_response(), I think we need

diff --git a/client/src/client.rs b/client/src/client.rs
index 745b863..80fd717 100644
--- a/client/src/client.rs
+++ b/client/src/client.rs
@@ -1342,11 +1342,7 @@ fn log_response(cmd: &str, resp: &Result<jsonrpc::Response>) {
                         debug!(target: "bitcoincore_rpc", "JSON-RPC error for {}: {:?}", cmd, e);
                     }
                 } else if log_enabled!(Trace) {
-                    // we can't use to_raw_value here due to compat with Rust 1.29
-                    let def = serde_json::value::RawValue::from_string(
-                        serde_json::Value::Null.to_string(),
-                    )
-                    .unwrap();
+                    let def = serde_json::value::to_raw_value(&serde_json::value::Value::Null);
                     let result = resp.result.as_ref().unwrap_or(&def);
                     trace!(target: "bitcoincore_rpc", "JSON-RPC response for {}: {}", cmd, result);
apoelstra commented 6 months ago

Good catch. And yep, I think your solution is right ... I don't see a way to directly create a raw null.

romanz commented 6 months ago

Thanks - fixed in https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/324/commits/308448d7b7c92190e5e9e8596315ba8e7840438a.

tcharding commented 6 months ago

CI fail is unrelated to this PR, fixed in #351