Implement importmulti method and test #263

GideonBature · 2025-06-21T08:41:09Z

The JSON-RPC method importmulti does return a type. We want to test this to catch any changes in behavior in future Core versions.

This PR adds a client function that errors if the return value is anything other than the type it returns, along with an integration test that calls this function.

Ref: #116

jamillambert · 2025-06-23T17:27:09Z

client/src/client_sync/v17/mod.rs

+/// Args for the `importmulti` method
+#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
+pub struct ImportMultiRequest {


A question for @tcharding what is the reason for these argument structs being in client and not in types?

Because they are used with the client and if one writes their own client (which I hope folk will) and just depend on types we don't want a bunch of 'random' stuff in there. On the other hand wsince we have the client-sync feature we could expose them and then writers of a client could still depend on our client to grab them? I'd be open to that. (By expose I mean put them in lib.rs or someplace so they are not hidden by the feature.)

integration_test/tests/wallet.rs

jamillambert · 2025-06-23T17:47:17Z

types/src/v17/wallet/mod.rs

+    pub warnings: Option<Vec<String>>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    /// The error.
+    pub error: Option<JsonRpcError>,


I don't understand the use of #[serde(default, skip_serializing_if..., this is the result of the RPC call and therefore deserialized from json.

If we want fields that are None in the result types to be omitted from the serialization this should be done crate wide and not have different functionality for different RPC calls. I believe core always omits them?

@tcharding thoughts?

My understanding is, and it may be wrong, that this is a no-op because serde already omits None when serializing options. I'm guessing you got this from an LLM @GideonBature? I've seen LLMs add this when using them.

I don't think we should use this attribute unless there is some explicit reason to, and if so, like you say @jamillambert it would then have to be done crate wide. And we definitely don't want to do that unless there is a very good reason.

my bad, was not supposed to be for result, only for args.

My understanding is, and it may be wrong, that this is a no-op because serde already omits None when serializing options. I'm guessing you got this from an LLM @GideonBature? I've seen LLMs add this when using them.

I don't think we should use this attribute unless there is some explicit reason to, and if so, like you say @jamillambert it would then have to be done crate wide. And we definitely don't want to do that unless there is a very good reason.

yes... you are right.

GideonBature · 2025-06-24T11:57:33Z

Effected suggestions.

Also, introduced another type for v18 upwards, which has the warnings field. The v17 doesn't have the warnings field, had to confirm that with the code in core and found out that every thing remains same except that v17 doesn't return the warnings fields in it's response. Also, updated the test accordingly.

tcharding

Everything looks good except I'd like to see the test improved upon please. Its great that you are testing so thoroughly its just a bit unclear by reading the test exactly what it is testing.

Since we are primarily interested in proving the shape of the returned type perhaps structure the test, and include code comments, around this idea.

E.g, Do some set up and comment what is expected then assert that we get what is expected.

Also please comment if things are version specific, ATM it looks like descriptors are supported all the way back to v17 but the docs don't support that, or is it just undocumented?

Thanks

tcharding · 2025-06-25T01:43:47Z

client/src/client_sync/v17/mod.rs

+    #[serde(rename = "scriptPubKey", skip_serializing_if = "Option::is_none")]
+    pub script_pub_key: Option<ImportMultiScriptPubKey>,


This arg is required by v17 and v29 so shouldn't be Option, right?

yes, it is required by v17 - v29. Why I made it Option is because for v18 - v29 either scriptPubkey or descriptor is required but not both.

ooo, I missed that. Thanks

Needs rename to script_pubkey.

will open a patch PR for the renaming.

client/src/client_sync/v17/mod.rs

GideonBature · 2025-06-27T06:34:46Z

Everything looks good except I'd like to see the test improved upon please. Its great that you are testing so thoroughly its just a bit unclear by reading the test exactly what it is testing.

Since we are primarily interested in proving the shape of the returned type perhaps structure the test, and include code comments, around this idea.

E.g, Do some set up and comment what is expected then assert that we get what is expected.

Also please comment if things are version specific, ATM it looks like descriptors are supported all the way back to v17 but the docs don't support that, or is it just undocumented?

Thanks

Yes, descriptors is not mentioned in v17, yet the test still passes, prolly will exclude the test using descriptors for v17.

GideonBature · 2025-06-27T12:47:04Z

Implemented the changes suggested and also added more cases to the test...

Remove redundant code from types and args Add comment and field checks to test Add appropriate type for v17 and v18 Make test more explicit and robust Resolve Bip125Replaceable import issue Resolve assert checks for v17

Format code changes Format code Format code

tcharding

ACK ec24420

tcharding · 2025-06-28T00:58:07Z

Good effort man, that test is nice.

GideonBature · 2025-06-28T06:28:23Z

Thank you. :).

303d8e4 Rename script_pub_key to script_pubkey (GideonBature) Pull request description: This is a follow up PR, that implements the suggestion given in this [comment](#263 (comment)) for PR #263 to rename `script_pub_key` to `script_pubkey` for readability purpose. ACKs for top commit: tcharding: ACK 303d8e4 Tree-SHA512: b6f5f0def9947f8dcfbe2464ffdff295a084dfe634d5e081276ca79b84c31a7569a45db5ba6c8386372ab35ae33cf6154439832fa809b31ee0ba4d7c1aad2568

github-actions bot added C-client C-integration-test C-types C-verify labels Jun 21, 2025

jamillambert reviewed Jun 23, 2025

View reviewed changes

GideonBature force-pushed the importmulti branch 3 times, most recently from afcf58d to b05f196 Compare June 24, 2025 10:08

GideonBature force-pushed the importmulti branch from 01eeced to 9d0995a Compare June 24, 2025 12:07

tcharding reviewed Jun 25, 2025

View reviewed changes

GideonBature force-pushed the importmulti branch 6 times, most recently from 5b1f2ad to cc79a67 Compare June 27, 2025 12:29

GideonBature marked this pull request as draft June 27, 2025 12:37

GideonBature force-pushed the importmulti branch from 2096697 to 1c1bb5d Compare June 27, 2025 18:26

GideonBature marked this pull request as ready for review June 27, 2025 18:40

GideonBature added 2 commits June 27, 2025 19:41

Implement importmulti method and test

4667b26

Remove redundant code from types and args Add comment and field checks to test Add appropriate type for v17 and v18 Make test more explicit and robust Resolve Bip125Replaceable import issue Resolve assert checks for v17

Format code

ec24420

Format code changes Format code Format code

GideonBature force-pushed the importmulti branch from d6de385 to ec24420 Compare June 27, 2025 18:41

tcharding approved these changes Jun 28, 2025

View reviewed changes

tcharding merged commit 2aede02 into rust-bitcoin:master Jun 28, 2025
29 checks passed

GideonBature mentioned this pull request Jun 28, 2025

Rename script_pub_key to script_pubkey #275

Merged

		#[serde(rename = "scriptPubKey", skip_serializing_if = "Option::is_none")]
		pub script_pub_key: Option<ImportMultiScriptPubKey>,

Implement importmulti method and test #263

Implement importmulti method and test #263

Uh oh!

Conversation

GideonBature commented Jun 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GideonBature commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GideonBature Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GideonBature commented Jun 27, 2025

Uh oh!

GideonBature commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Jun 28, 2025

Uh oh!

GideonBature commented Jun 28, 2025

Uh oh!

Uh oh!

tcharding Jun 24, 2025 •

edited

Loading

GideonBature commented Jun 24, 2025 •

edited

Loading

GideonBature Jun 27, 2025 •

edited

Loading

GideonBature commented Jun 27, 2025 •

edited

Loading

tcharding left a comment •

edited

Loading