From 70814bf1992fcfb09dd00c9316a396138912019c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:32:58 +1000 Subject: [PATCH 01/11] Pass BlockHash by value The `BlockHash` type is 32 bytes and implements `Copy`, we should pass it by value not by reference. --- client/src/client_sync/v17/blockchain.rs | 6 +++--- integration_test/src/v17/blockchain.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/client/src/client_sync/v17/blockchain.rs b/client/src/client_sync/v17/blockchain.rs index 0d3de1d..a3bc475 100644 --- a/client/src/client_sync/v17/blockchain.rs +++ b/client/src/client_sync/v17/blockchain.rs @@ -33,7 +33,7 @@ macro_rules! impl_client_v17__getblock { () => { impl Client { /// Gets a block by blockhash. - pub fn get_block(&self, hash: &BlockHash) -> Result { + pub fn get_block(&self, hash: BlockHash) -> Result { let json = self.get_block_verbosity_zero(hash)?; Ok(json.block()?) } @@ -43,14 +43,14 @@ macro_rules! impl_client_v17__getblock { pub fn get_block_verbosity_zero( &self, - hash: &BlockHash, + hash: BlockHash, ) -> Result { self.call("getblock", &[into_json(hash)?, 0.into()]) } pub fn get_block_verbosity_one( &self, - hash: &BlockHash, + hash: BlockHash, ) -> Result { self.call("getblock", &[into_json(hash)?, 1.into()]) } diff --git a/integration_test/src/v17/blockchain.rs b/integration_test/src/v17/blockchain.rs index 4a8c56a..1c0080e 100644 --- a/integration_test/src/v17/blockchain.rs +++ b/integration_test/src/v17/blockchain.rs @@ -32,7 +32,7 @@ macro_rules! impl_test_v17__getblock_verbosity_0 { let bitcoind = $crate::bitcoind_no_wallet(); let block_hash = best_block_hash(); - let json = bitcoind.client.get_block_verbosity_zero(&block_hash).expect("getblock 0"); + let json = bitcoind.client.get_block_verbosity_zero(block_hash).expect("getblock 0"); json.into_model().unwrap(); } }; @@ -47,7 +47,7 @@ macro_rules! impl_test_v17__getblock_verbosity_1 { let bitcoind = $crate::bitcoind_no_wallet(); let block_hash = best_block_hash(); - let json = bitcoind.client.get_block_verbosity_one(&block_hash).expect("getblock 1"); + let json = bitcoind.client.get_block_verbosity_one(block_hash).expect("getblock 1"); json.into_model().unwrap(); } }; @@ -62,7 +62,7 @@ macro_rules! impl_test_v17__getblock_verbosity_2 { let bitcoind = $crate::bitcoind_no_wallet(); let block_hash = best_block_hash(); - let json = client.get_block_verbosity_two(&block_hash).expect("getblock 2"); + let json = client.get_block_verbosity_two(block_hash).expect("getblock 2"); json.into_model().unwrap(); } }; From 13543c48287d1b0783ea78ab4aca1090dfc91aba Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:52:48 +1000 Subject: [PATCH 02/11] Use transpose combinator Convert `Option>` to ``Result, E>` by using the `transpose` combinator - TIL. --- json/src/v19/wallet.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/json/src/v19/wallet.rs b/json/src/v19/wallet.rs index f1e35d7..308e106 100644 --- a/json/src/v19/wallet.rs +++ b/json/src/v19/wallet.rs @@ -53,11 +53,7 @@ impl GetBalances { /// Converts version specific type to a version in-specific, more strongly typed type. pub fn into_model(self) -> Result { let mine = self.mine.into_model()?; - // FIXME: Use combinators instead of matching like a noob. - let watch_only = match self.watch_only { - Some(watch_only) => Some(watch_only.into_model()?), - None => None, - }; + let watch_only = self.watch_only.map(|watch_only| watch_only.into_model()).transpose()?; Ok(model::GetBalances { mine, watch_only }) } @@ -69,11 +65,7 @@ impl GetBalancesMine { let trusted = Amount::from_btc(self.trusted)?; let untrusted_pending = Amount::from_btc(self.untrusted_pending)?; let immature = Amount::from_btc(self.immature)?; - // FIXME: Use combinators instead of matching like a noob. - let used = match self.used { - Some(used) => Some(Amount::from_btc(used)?), - None => None, - }; + let used = self.used.map(Amount::from_btc).transpose()?; Ok(model::GetBalancesMine { trusted, untrusted_pending, immature, used }) } From 0dba9ed991c6b08b8873dd841cbd386e6137286f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:58:45 +1000 Subject: [PATCH 03/11] model: Remove connections in/out The `v17::GetNetworkInfo` does not have connections in and out, only total. We should not have these fields until we support a version of Core that does have these fields. --- json/src/model/network.rs | 4 ---- json/src/v17/network.rs | 2 -- 2 files changed, 6 deletions(-) diff --git a/json/src/model/network.rs b/json/src/model/network.rs index a6775b0..c204e09 100644 --- a/json/src/model/network.rs +++ b/json/src/model/network.rs @@ -27,10 +27,6 @@ pub struct GetNetworkInfo { pub time_offset: isize, /// The total number of connections. pub connections: usize, - /// The number of inbound connections. - pub connections_in: usize, - /// The number of outbound connections. - pub connections_out: usize, /// Whether p2p networking is enabled. pub network_active: bool, /// Information per network. diff --git a/json/src/v17/network.rs b/json/src/v17/network.rs index e06145e..b99652a 100644 --- a/json/src/v17/network.rs +++ b/json/src/v17/network.rs @@ -99,8 +99,6 @@ impl GetNetworkInfo { local_relay: self.local_relay, time_offset: self.time_offset, connections: self.connections, - connections_in: 0, // FIXME: Can we do better than this? - connections_out: 0, // FIXME: Can we do better than this? network_active: self.network_active, networks: self.networks.into_iter().map(|j| j.into_model()).collect(), relay_fee, From 69a0d9225c9f5f2d55db3f0543b2dfe7344468e4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 13:06:27 +1000 Subject: [PATCH 04/11] Remove fee_reason from SendToAddress This is kruft from an earlier design; currently we only support v17 `sendtoaddress` which only responds with the txid. --- json/src/model/wallet.rs | 2 -- json/src/v17/wallet.rs | 6 +----- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/json/src/model/wallet.rs b/json/src/model/wallet.rs index d0ab943..8f17656 100644 --- a/json/src/model/wallet.rs +++ b/json/src/model/wallet.rs @@ -83,8 +83,6 @@ pub struct GetNewAddress(pub Address); pub struct SendToAddress { /// The transaction id. pub txid: Txid, - /// The transaction fee reason. - pub fee_reason: String, } /// Models the result of JSON-RPC method `gettransaction`. diff --git a/json/src/v17/wallet.rs b/json/src/v17/wallet.rs index ebf144d..c1fdee7 100644 --- a/json/src/v17/wallet.rs +++ b/json/src/v17/wallet.rs @@ -146,11 +146,7 @@ impl SendToAddress { /// Converts version specific type to a version in-specific, more strongly typed type. pub fn into_model(self) -> Result { let txid = self.0.parse::()?; - Ok(model::SendToAddress { - txid, - // FIXME: Is this acceptable? - fee_reason: "".to_string(), - }) + Ok(model::SendToAddress { txid }) } /// Converts json straight to a `bitcoin::Txid`. From 27a7dc3405fd8825bc7f195fe8f0c3ab603078f5 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:30:39 +1000 Subject: [PATCH 05/11] docs: Fix typo in GetBlockVerbasityOne --- json/src/v17/blockchain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/json/src/v17/blockchain.rs b/json/src/v17/blockchain.rs index 46ee53e..62648e0 100644 --- a/json/src/v17/blockchain.rs +++ b/json/src/v17/blockchain.rs @@ -161,7 +161,7 @@ impl GetBlockVerbosityOne { } } -/// Error when converting a `GetBlockVerbasityOne` type into the model type. +/// Error when converting a `GetBlockVerbosityOne` type into the model type. #[derive(Debug)] pub enum GetBlockVerbosityOneError { /// Conversion of the transaction `hash` field failed. From 4ef249b8c029971ec29ca7ed17552ab80a302c41 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:40:10 +1000 Subject: [PATCH 06/11] Remove FIXME on call to collect This is indeed the correct way to get `collect` to transform a list of `Result` to a `Result, E>`. --- json/src/v17/blockchain.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/json/src/v17/blockchain.rs b/json/src/v17/blockchain.rs index 62648e0..25f8d01 100644 --- a/json/src/v17/blockchain.rs +++ b/json/src/v17/blockchain.rs @@ -117,7 +117,6 @@ impl GetBlockVerbosityOne { let weight = Weight::from_wu(self.weight); // TODO: Confirm this uses weight units. let version = block::Version::from_consensus(self.version); - // FIXME: Is there a better way to handle the error without type annotations on `collect`? let tx = self .tx .iter() From 86c95de8e73f925d9b5c7c6c6e7aa692a57e5832 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 12:55:07 +1000 Subject: [PATCH 07/11] Remove FIXME about unperfixed hex The integration tests will catch this if it is wrong. --- json/src/v17/blockchain.rs | 2 -- json/src/v19/blockchain.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/json/src/v17/blockchain.rs b/json/src/v17/blockchain.rs index 25f8d01..67f04cc 100644 --- a/json/src/v17/blockchain.rs +++ b/json/src/v17/blockchain.rs @@ -123,7 +123,6 @@ impl GetBlockVerbosityOne { .map(|t| encode::deserialize_hex::(t).map_err(E::Tx)) .collect::, _>>()?; - // FIXME: Is unprefixed correct? let bits = CompactTarget::from_unprefixed_hex(&self.bits).map_err(E::Bits)?; let chain_work = Work::from_unprefixed_hex(&self.chain_work).map_err(E::ChainWork)?; @@ -316,7 +315,6 @@ impl GetBlockchainInfo { let chain = Network::from_core_arg(&self.chain).map_err(E::Chain)?; let best_block_hash = self.best_block_hash.parse::().map_err(E::BestBlockHash)?; - // FIXME: Is unprefixed correct? let chain_work = Work::from_unprefixed_hex(&self.chain_work).map_err(E::ChainWork)?; let softforks = BTreeMap::new(); // TODO: Handle softforks stuff. diff --git a/json/src/v19/blockchain.rs b/json/src/v19/blockchain.rs index ea23bd8..6a23561 100644 --- a/json/src/v19/blockchain.rs +++ b/json/src/v19/blockchain.rs @@ -148,7 +148,6 @@ impl GetBlockchainInfo { let chain = Network::from_core_arg(&self.chain).map_err(E::Chain)?; let best_block_hash = self.best_block_hash.parse::().map_err(E::BestBlockHash)?; - // FIXME: Is unprefixed correct? let chain_work = Work::from_unprefixed_hex(&self.chain_work).map_err(E::ChainWork)?; let softforks = BTreeMap::new(); // TODO: Handle softforks stuff. From 5d9ba83de981b0d5ac655783dfef16e5b89a0be3 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 13:00:33 +1000 Subject: [PATCH 08/11] Remove FIXME question from getblock We want to provide all shapes of JSON data so having two explicit functions is a reasonable way, I can't think of a better one right now. --- client/src/client_sync/v17/blockchain.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/client/src/client_sync/v17/blockchain.rs b/client/src/client_sync/v17/blockchain.rs index a3bc475..f45e341 100644 --- a/client/src/client_sync/v17/blockchain.rs +++ b/client/src/client_sync/v17/blockchain.rs @@ -38,9 +38,6 @@ macro_rules! impl_client_v17__getblock { Ok(json.block()?) } - // FIXME(getblock): This handling of optional args is ugly as hell but because the returned json - // is different for each verbosity these are functionally different methods. Is there a better way? - pub fn get_block_verbosity_zero( &self, hash: BlockHash, From 586f19cb98f1785e25e3de8812611abaf4c9a129 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 13:03:33 +1000 Subject: [PATCH 09/11] Remove FIXME on warnings I may have got mixed up with `getblockchaininfo` when I wrote this FIXME, we only support v17 `getnetworkinfo` and the `warnings` field is correctly named (and documented). --- json/src/model/network.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/json/src/model/network.rs b/json/src/model/network.rs index c204e09..ec3fdf9 100644 --- a/json/src/model/network.rs +++ b/json/src/model/network.rs @@ -38,7 +38,7 @@ pub struct GetNetworkInfo { /// List of local addresses. pub local_addresses: Vec, /// Any network and blockchain warnings. - pub warnings: String, // FIXME: I rekon this is wrong. + pub warnings: String, } /// Part of the result of the JSON-RPC method `getnetworkinfo` (information per network). From ee33ee4b4ad742a28586ae3f55248357c112fd86 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 13:17:58 +1000 Subject: [PATCH 10/11] Remove FIXME for missing fields Investigate and remove the fixme, leave a more terse comment incase someone stumbles upon this later. --- json/src/v17/wallet.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/json/src/v17/wallet.rs b/json/src/v17/wallet.rs index c1fdee7..b8f2370 100644 --- a/json/src/v17/wallet.rs +++ b/json/src/v17/wallet.rs @@ -167,13 +167,8 @@ pub struct GetTransaction { pub amount: f64, pub fee: Option, pub confirmations: u32, - // FIXME: The docs say these two fields should be here but it is not returned. - // Is it worth patching Core for a version this old? - // - // #[serde(rename = "blockhash")] - // pub block_hash: String, - // #[serde(rename = "blockindex")] - // pub block_index: u64, + // The docs say there should be two more fields: `blockhash` and `blockindex` but integration + // test fails if we add them i.e., they are not returned by `v0.17.1`. pub txid: String, pub time: u64, #[serde(rename = "timereceived")] From e14093ae1ffce7ca16f8e9965fb5a3bf14a7b4d4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 6 Sep 2024 13:19:39 +1000 Subject: [PATCH 11/11] Remove FIXME on enum type conversion This code is verbose but trivial, lets leave it in. --- json/src/v17/blockchain.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/json/src/v17/blockchain.rs b/json/src/v17/blockchain.rs index 67f04cc..7bf648c 100644 --- a/json/src/v17/blockchain.rs +++ b/json/src/v17/blockchain.rs @@ -340,7 +340,6 @@ impl GetBlockchainInfo { } } -// FIXME: Me mightn't need this. impl Bip9SoftforkStatus { /// Converts version specific type to a version in-specific, more strongly typed type. pub fn into_model(self) -> model::Bip9SoftforkStatus {