Address all TODO in v24 #308

jamillambert · 2025-07-22T17:00:21Z

Go through all the TODO in the v24 types table. Add all the missing RPCs.

Add gettxspendingprevout method, model and test
Add sendall method, model and test
Increase client timeout - The migratewallet RPC call takes about 30sec on v24, it's a lot faster on v29. The default timeout was 15sec, increase it to 60 to prevent the timeout during testing. To quote Core "This RPC may take a long time to complete. Increasing the RPC client timeout is recommended."
Add migratewallet method, and test - The help says that passphrase is required in v24, but it isn't. The optional argument wallet_name was required for testing. There is nothing to model so the types table was updated accordingly.
Add simulaterawtransaction method, model and test - The help says that the rawtxs are optional, but they
are required for testing or nothing is simulated.
Run the formatter

tcharding · 2025-07-23T03:17:19Z

client/src/client_sync/v24/mod.rs

+pub struct GetTxSpendingPrevoutInput {
+    /// The transaction id.
+    pub txid: Txid,
+    /// The output number.
+    pub vout: u32,
+}


In 5579b79

This is the same as a bitcoin::OutPoint - we can use that instead.

tcharding · 2025-07-23T03:19:22Z

types/src/model/blockchain.rs

+    /// The transaction id of the checked output.
+    pub txid: Txid,
+    /// The vout value of the checked output.
+    pub vout: u32,


Might be able to use an OutPoint here as well.

tcharding · 2025-07-23T03:22:20Z

types/src/v24/blockchain/into.rs

+
+impl GetTxSpendingPrevoutItem {
+    /// Converts version specific type to a version nonspecific, more strongly typed type.
+    pub fn into_model(self) -> Result<model::GetTxSpendingPrevoutItem, hex::HexToArrayError> {


Whilst this is correct it would be nicer, and more inline with the rest of the crate (I hope) to have a custom error here with two variants, one for each conversion that can fail. If there are other places that do this sort of thing already then feel free to be lazy and leave it as is. If you do chose that can you write an issue explaining the lazy choice please?

Made the custom error with two variants.

tcharding · 2025-07-23T03:23:35Z

types/src/v24/blockchain/into.rs

+        let spending_txid = match self.spending_txid {
+            Some(id) => Some(id.parse::<Txid>()?),
+            None => None,
+        };


Just FYI this can probably be done more cleanly using map, pretty sure linter should fire for this 'manual implementation of map' or some such error. If not we may not have the linter configured as tightly as in rust-bitcoin.

tcharding · 2025-07-23T03:25:33Z

client/src/client_sync/v24/wallet.rs

+macro_rules! impl_client_v24__send_all {
+    () => {
+        impl Client {
+            pub fn send_all(&self, recipients: &[String]) -> Result<SendAll> {


Would be nicer to use Address than String unless there is some specific reason not to.

tcharding · 2025-07-23T03:26:45Z

integration_test/tests/wallet.rs

+    let recipients = vec![address.to_string()];
+
+    let json: SendAll = node.client.send_all(&recipients).expect("sendall");


We can probably just use &[address] inline in send_all.

tcharding · 2025-07-23T03:33:00Z

types/src/v24/wallet/into.rs

+        let hex =
+            self.hex.as_ref().map(|h| encode::deserialize_hex(h)).transpose().map_err(E::Hex)?;


This line is changed in fd06f28 but that patch is unrelated.

Moved to the correct commit

tcharding

Reviewed 1de8ce4

Add the struct for the RPC, the model, client macro, reexports and test.

Add the struct, model, client macro, reexports and test.

New RPC migratewallet takes around 30sec to complete and the client times out first. Increase the timeout from default of 15sec to 60sec.

Add the struct, client macro, reexports and test. No model is required, change the types table to `version`, update verify and remove TODO.

Add the struct, client macro, model, reexports and test.

Reordering of reexports alphabetically, no other changes.

jamillambert · 2025-07-23T09:28:12Z

Suggested changes all made.

tcharding

ACK 78cbbdc

tcharding · 2025-07-24T22:56:06Z

client/src/client_sync/v24/blockchain.rs

+                    serde_json::json!({
+                        "txid": out.txid.to_string(),
+                        "vout": out.vout,
+                    })


Something is odd here, we should be able to use into_json(out).

github-actions bot added C-client C-integration-test C-types C-verify labels Jul 22, 2025

jamillambert force-pushed the 0721-v24-TODO branch from 65da4c9 to 1de8ce4 Compare July 22, 2025 18:09

tcharding reviewed Jul 23, 2025

View reviewed changes

jamillambert added 6 commits July 23, 2025 10:16

Add gettxspendingprevout method, model and test

8610326

Add the struct for the RPC, the model, client macro, reexports and test.

Add sendall method, model and test

5adeeae

Add the struct, model, client macro, reexports and test.

Increase client timeout

976bb9e

New RPC migratewallet takes around 30sec to complete and the client times out first. Increase the timeout from default of 15sec to 60sec.

Add migratewallet method and test

3a9e9be

Add the struct, client macro, reexports and test. No model is required, change the types table to `version`, update verify and remove TODO.

Add simulaterawtransaction method, model and test

2125b02

Add the struct, client macro, model, reexports and test.

Run the formatter

78cbbdc

Reordering of reexports alphabetically, no other changes.

jamillambert force-pushed the 0721-v24-TODO branch from 1de8ce4 to 78cbbdc Compare July 23, 2025 09:26

tcharding approved these changes Jul 24, 2025

View reviewed changes

tcharding merged commit b6d31ec into rust-bitcoin:master Jul 24, 2025
30 checks passed

tcharding reviewed Jul 24, 2025

View reviewed changes

		let recipients = vec![address.to_string()];

		let json: SendAll = node.client.send_all(&recipients).expect("sendall");

		let hex =
		self.hex.as_ref().map(\|h\| encode::deserialize_hex(h)).transpose().map_err(E::Hex)?;

Address all TODO in v24 #308

Address all TODO in v24 #308

Conversation

jamillambert commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

jamillambert commented Jul 23, 2025

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamillambert commented Jul 22, 2025 •

edited

Loading