Do a bunch of minor improvements (#31)

Remove `FIXME`s and things like that, non of this is controversial.

Author: tcharding

client/src/client_sync/v17/blockchain.rs

@@ -33,24 +33,21 @@ macro_rules! impl_client_v17__getblock {
     () => {
         impl Client {
             /// Gets a block by blockhash.
-            pub fn get_block(&self, hash: &BlockHash) -> Result<Block> {
+            pub fn get_block(&self, hash: BlockHash) -> Result<Block> {
                 let json = self.get_block_verbosity_zero(hash)?;
                 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,
+                hash: BlockHash,
             ) -> Result<GetBlockVerbosityZero> {
                 self.call("getblock", &[into_json(hash)?, 0.into()])
             }
 
             pub fn get_block_verbosity_one(
                 &self,
-                hash: &BlockHash,
+                hash: BlockHash,
             ) -> Result<GetBlockVerbosityOne> {
                 self.call("getblock", &[into_json(hash)?, 1.into()])
             }

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();
         }
     };

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.
@@ -42,7 +38,7 @@ pub struct GetNetworkInfo {
     /// List of local addresses.
     pub local_addresses: Vec<GetNetworkInfoAddress>,
     /// 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).

json/src/model/wallet.rs

@@ -83,8 +83,6 @@ pub struct GetNewAddress(pub Address<NetworkUnchecked>);
 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`.

json/src/v17/blockchain.rs

@@ -117,14 +117,12 @@ 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()
             .map(|t| encode::deserialize_hex::<Txid>(t).map_err(E::Tx))
             .collect::<Result<Vec<_>, _>>()?;
 
-        // 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)?;
 
@@ -161,7 +159,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.
@@ -317,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::<BlockHash>().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.
@@ -343,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 {

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,

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<model::SendToAddress, hex::HexToArrayError> {
         let txid = self.0.parse::<Txid>()?;
-        Ok(model::SendToAddress {
-            txid,
-            // FIXME: Is this acceptable?
-            fee_reason: "".to_string(),
-        })
+        Ok(model::SendToAddress { txid })
     }
 
     /// Converts json straight to a `bitcoin::Txid`.
@@ -171,13 +167,8 @@ pub struct GetTransaction {
     pub amount: f64,
     pub fee: Option<f64>,
     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")]

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::<BlockHash>().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.

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<model::GetBalances, ParseAmountError> {
         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 })
     }