change(rpc): Simplify RPC types and add documentation (#4218)

* Simplify RPC types and add documentation

* Derive serde traits in production code

zebra-chain/Cargo.toml

@@ -9,8 +9,7 @@ edition = "2021"
 
 [features]
 default = []
-proptest-impl = ["proptest", "proptest-derive", "zebra-test", "rand", "rand_chacha", "tokio",
+proptest-impl = ["proptest", "proptest-derive", "zebra-test", "rand", "rand_chacha", "tokio", "hex/serde"]
-"hex/serde", "serde_with"]
 bench = ["zebra-test"]
 
 [dependencies]
@@ -39,7 +38,7 @@ rand_core = "0.6.3"
 ripemd = "0.1.1"
 
 serde = { version = "1.0.136", features = ["serde_derive", "rc"] }
-serde_with = { version = "1.13.0", optional = true }
+serde_with = "1.13.0"
 serde-big-array = "0.4.1"
 # Matches version used by hdwallet
 secp256k1 = { version = "0.21.3", features = ["serde"] }
@@ -79,7 +78,6 @@ spandoc = "0.2.2"
 tracing = "0.1.31"
 
 hex = { version = "0.4.3", features = ["serde"] }
-serde_with = "1.13.0"
 
 proptest = "0.10.1"
 proptest-derive = "0.3.0"

zebra-chain/src/transparent/address.rs

@@ -41,14 +41,12 @@ mod magics {
 /// to a Bitcoin address just by removing the "t".)
 ///
 /// https://zips.z.cash/protocol/protocol.pdf#transparentaddrencoding
-#[derive(Copy, Clone, Eq, PartialEq, Hash)]
+#[derive(
+    Copy, Clone, Eq, PartialEq, Hash, serde_with::SerializeDisplay, serde_with::DeserializeFromStr,
+)]
 #[cfg_attr(
     any(test, feature = "proptest-impl"),
-    derive(
+    derive(proptest_derive::Arbitrary)
-        proptest_derive::Arbitrary,
-        serde_with::SerializeDisplay,
-        serde_with::DeserializeFromStr
-    )
 )]
 pub enum Address {
     /// P2SH (Pay to Script Hash) addresses

zebra-chain/src/transparent/script.rs

@@ -9,17 +9,17 @@ use crate::serialization::{
 };
 
 /// An encoding of a Bitcoin script.
-#[derive(Clone, Eq, PartialEq, Hash)]
+#[derive(Clone, Eq, PartialEq, Hash, Serialize, Deserialize)]
 #[cfg_attr(
     any(test, feature = "proptest-impl"),
-    derive(proptest_derive::Arbitrary, Serialize, Deserialize)
+    derive(proptest_derive::Arbitrary)
 )]
 pub struct Script(
     /// # Correctness
     ///
     /// Consensus-critical serialization uses [`ZcashSerialize`].
-    /// [`serde`]-based hex serialization must only be used for testing.
+    /// [`serde`]-based hex serialization must only be used for RPCs and testing.
-    #[cfg_attr(any(test, feature = "proptest-impl"), serde(with = "hex"))]
+    #[serde(with = "hex")]
     Vec<u8>,
 );
 

zebra-rpc/src/methods.rs

@@ -24,10 +24,11 @@ use zebra_chain::{
     parameters::{ConsensusBranchId, Network, NetworkUpgrade},
     serialization::{SerializationError, ZcashDeserialize},
     transaction::{self, SerializedTransaction, Transaction, UnminedTx},
-    transparent::Address,
+    transparent::{self, Address},
 };
 use zebra_network::constants::USER_AGENT;
 use zebra_node_services::{mempool, BoxError};
+use zebra_state::OutputIndex;
 
 use crate::queue::Queue;
 
@@ -410,9 +411,9 @@ where
 
         let response = GetBlockChainInfo {
             chain,
-            blocks: tip_height.0,
+            blocks: tip_height,
-            best_block_hash: GetBestBlockHash(tip_hash),
+            best_block_hash: tip_hash,
-            estimated_height: estimated_height.0,
+            estimated_height,
             upgrades,
             consensus,
         };
@@ -737,20 +738,20 @@ where
             };
 
             for utxo_data in utxos.utxos() {
-                let address = utxo_data.0.to_string();
+                let address = utxo_data.0;
-                let txid = utxo_data.1.to_string();
+                let txid = *utxo_data.1;
-                let height = utxo_data.2.height().0;
+                let height = utxo_data.2.height();
-                let output_index = utxo_data.2.output_index().as_usize();
+                let output_index = utxo_data.2.output_index();
-                let script = utxo_data.3.lock_script.to_string();
+                let script = utxo_data.3.lock_script.clone();
                 let satoshis = u64::from(utxo_data.3.value);
 
                 let entry = GetAddressUtxos {
                     address,
                     txid,
-                    height,
                     output_index,
                     script,
                     satoshis,
+                    height,
                 };
                 response_utxos.push(entry);
             }
@@ -766,7 +767,10 @@ where
 /// See the notes for the [`Rpc::get_info` method].
 #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
 pub struct GetInfo {
+    /// The node version build number
     build: String,
+
+    /// The server sub-version identifier, used as the network protocol user-agent
     subversion: String,
 }
 
@@ -775,31 +779,46 @@ pub struct GetInfo {
 /// See the notes for the [`Rpc::get_blockchain_info` method].
 #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
 pub struct GetBlockChainInfo {
+    /// Current network name as defined in BIP70 (main, test, regtest)
     chain: String,
-    blocks: u32,
+
-    #[serde(rename = "bestblockhash")]
+    /// The current number of blocks processed in the server, numeric
-    best_block_hash: GetBestBlockHash,
+    blocks: Height,
+
+    /// The hash of the currently best block, in big-endian order, hex-encoded
+    #[serde(rename = "bestblockhash", with = "hex")]
+    best_block_hash: block::Hash,
+
+    /// If syncing, the estimated height of the chain, else the current best height, numeric.
+    ///
+    /// In Zebra, this is always the height estimate, so it might be a little inaccurate.
     #[serde(rename = "estimatedheight")]
-    estimated_height: u32,
+    estimated_height: Height,
+
+    /// Status of network upgrades
     upgrades: IndexMap<ConsensusBranchIdHex, NetworkUpgradeInfo>,
+
+    /// Branch IDs of the current and upcoming consensus rules
     consensus: TipConsensusBranch,
 }
 
-/// A wrapper type with a list of strings of addresses.
+/// A wrapper type with a list of transparent address strings.
 ///
 /// This is used for the input parameter of [`Rpc::get_address_balance`],
 /// [`Rpc::get_address_tx_ids`] and [`Rpc::get_address_utxos`].
 #[derive(Clone, Debug, Eq, PartialEq, Hash, serde::Deserialize)]
 pub struct AddressStrings {
+    /// A list of transparent address strings.
     addresses: Vec<String>,
 }
 
 impl AddressStrings {
-    // Creates a new `AddressStrings` given a vector.
+    /// Creates a new `AddressStrings` given a vector.
     #[cfg(test)]
     pub fn new(addresses: Vec<String>) -> AddressStrings {
         AddressStrings { addresses }
     }
+
     /// Given a list of addresses as strings:
     /// - check if provided list have all valid transparent addresses.
     /// - return valid addresses as a set of `Address`.
@@ -821,6 +840,7 @@ impl AddressStrings {
 /// The transparent balance of a set of addresses.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, serde::Serialize)]
 pub struct AddressBalance {
+    /// The total transparent balance.
     balance: u64,
 }
 
@@ -831,19 +851,34 @@ struct ConsensusBranchIdHex(#[serde(with = "hex")] ConsensusBranchId);
 /// Information about [`NetworkUpgrade`] activation.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
 struct NetworkUpgradeInfo {
+    /// Name of upgrade, string.
+    ///
+    /// Ignored by lightwalletd, but useful for debugging.
     name: NetworkUpgrade,
+
+    /// Block height of activation, numeric.
     #[serde(rename = "activationheight")]
     activation_height: Height,
+
+    /// Status of upgrade, string.
     status: NetworkUpgradeStatus,
 }
 
 /// The activation status of a [`NetworkUpgrade`].
 #[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
 enum NetworkUpgradeStatus {
+    /// The network upgrade is currently active.
+    ///
+    /// Includes all network upgrades that have previously activated,
+    /// even if they are not the most recent network upgrade.
     #[serde(rename = "active")]
     Active,
+
+    /// The network upgrade does not have an activation height.
     #[serde(rename = "disabled")]
     Disabled,
+
+    /// The network upgrade has an activation height, but we haven't reached it yet.
     #[serde(rename = "pending")]
     Pending,
 }
@@ -853,8 +888,11 @@ enum NetworkUpgradeStatus {
 /// These branch IDs are different when the next block is a network upgrade activation block.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
 struct TipConsensusBranch {
+    /// Branch ID used to validate the current chain tip, big-endian, hex-encoded.
     #[serde(rename = "chaintip")]
     chain_tip: ConsensusBranchIdHex,
+
+    /// Branch ID used to validate the next block, big-endian, hex-encoded.
     #[serde(rename = "nextblock")]
     next_block: ConsensusBranchIdHex,
 }
@@ -903,18 +941,34 @@ pub enum GetRawTransaction {
 /// Response to a `getaddressutxos` RPC request.
 ///
 /// See the notes for the [`Rpc::get_address_utxos` method].
-#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)]
+#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)]
 pub struct GetAddressUtxos {
-    address: String,
+    /// The transparent address, base58check encoded
-    txid: String,
+    address: transparent::Address,
-    height: u32,
+
+    /// The output txid, in big-endian order, hex-encoded
+    #[serde(with = "hex")]
+    txid: transaction::Hash,
+
+    /// The transparent output index, numeric
     #[serde(rename = "outputIndex")]
-    output_index: usize,
+    output_index: OutputIndex,
-    script: String,
+
+    /// The transparent output script, hex encoded
+    #[serde(with = "hex")]
+    script: transparent::Script,
+
+    /// The amount of zatoshis in the transparent output
     satoshis: u64,
+
+    /// The block height, numeric.
+    ///
+    /// We put this field last, to match the zcashd order.
+    height: Height,
 }
 
 impl GetRawTransaction {
+    /// Converts `tx` and `height` into a new `GetRawTransaction` in the `verbose` format.
     fn from_transaction(
         tx: Arc<Transaction>,
         height: Option<block::Height>,
@@ -937,7 +991,7 @@ impl GetRawTransaction {
     }
 }
 
-/// Check if provided height range is valid
+/// Check if provided height range is valid for address indexes.
 fn check_height_range(start: Height, end: Height, chain_height: Height) -> Result<()> {
     if start == Height(0) || end == Height(0) {
         return Err(Error::invalid_params(

zebra-rpc/src/methods/tests/prop.rs

@@ -536,9 +536,9 @@ proptest! {
         match response {
             Ok(info) => {
                 prop_assert_eq!(info.chain, network.bip70_network_name());
-                prop_assert_eq!(info.blocks, block_height.0);
+                prop_assert_eq!(info.blocks, block_height);
-                prop_assert_eq!(info.best_block_hash.0, block_hash);
+                prop_assert_eq!(info.best_block_hash, block_hash);
-                prop_assert!(info.estimated_height < Height::MAX.0);
+                prop_assert!(info.estimated_height < Height::MAX);
 
                 prop_assert_eq!(
                     info.consensus.chain_tip.0,

zebra-state/src/lib.rs

@@ -35,7 +35,7 @@ pub use request::{FinalizedBlock, HashOrHeight, PreparedBlock, ReadRequest, Requ
 pub use response::{ReadResponse, Response};
 pub use service::{
     chain_tip::{ChainTipChange, LatestChainTip, TipAction},
-    init, OutputLocation, TransactionLocation,
+    init, OutputIndex, OutputLocation, TransactionLocation,
 };
 
 #[cfg(any(test, feature = "proptest-impl"))]

zebra-state/src/service.rs

@@ -66,7 +66,7 @@ pub mod arbitrary;
 #[cfg(test)]
 mod tests;
 
-pub use finalized_state::{OutputLocation, TransactionLocation};
+pub use finalized_state::{OutputIndex, OutputLocation, TransactionLocation};
 
 pub type QueuedBlock = (
     PreparedBlock,

zebra-state/src/service/finalized_state.rs

@@ -37,7 +37,7 @@ mod arbitrary;
 #[cfg(test)]
 mod tests;
 
-pub use disk_format::{OutputLocation, TransactionLocation};
+pub use disk_format::{OutputIndex, OutputLocation, TransactionLocation};
 
 pub(super) use zebra_db::ZebraDb;
 

zebra-state/src/service/finalized_state/disk_format.rs

@@ -16,7 +16,7 @@ pub mod transparent;
 mod tests;
 
 pub use block::{TransactionIndex, TransactionLocation};
-pub use transparent::OutputLocation;
+pub use transparent::{OutputIndex, OutputLocation};
 
 /// Helper type for writing types to disk as raw bytes.
 /// Also used to convert key types to raw bytes for disk lookups.

zebra-state/src/service/finalized_state/disk_format/transparent.rs

@@ -7,6 +7,8 @@
 
 use std::{cmp::max, fmt::Debug};
 
+use serde::{Deserialize, Serialize};
+
 use zebra_chain::{
     amount::{self, Amount, NonNegative},
     block::Height,
@@ -22,8 +24,6 @@ use crate::service::finalized_state::disk_format::{
 
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
-#[cfg(any(test, feature = "proptest-impl"))]
-use serde::{Deserialize, Serialize};
 
 #[cfg(any(test, feature = "proptest-impl"))]
 mod arbitrary;
@@ -46,8 +46,7 @@ pub const OUTPUT_LOCATION_DISK_BYTES: usize =
 // Transparent types
 
 /// A transparent output's index in its transaction.
-#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash, Serialize, Deserialize)]
-#[cfg_attr(any(test, feature = "proptest-impl"), derive(Serialize, Deserialize))]
 pub struct OutputIndex(u32);
 
 impl OutputIndex {