Review fixes #1

rpc/src/lib.rs

@@ -12,11 +12,11 @@
 //! | ------- | ----------- |
 //! | `http-client` | Provides [`HttpClient`], which is a basic RPC client that interacts with
 //! remote Tendermint nodes via **JSON-RPC over HTTP**. This client does not provide [`Event`]
-//! subscription functionality. See the [Tendermint RPC] for more details. | | `websocket-client` |
-//! Provides [`WebSocketClient`], which currently only provides [`Event`] subscription functionality
-//! over a WebSocket connection. See the [`/subscribe` endpoint] in the Tendermint RPC for more
-//! details. This client does not yet provide access to the RPC methods provided by the [`Client`]
-//! trait (this is planned for a future release). |
+//! subscription functionality. See the [Tendermint RPC] for more details. |
+//! | `websocket-client` | Provides [`WebSocketClient`], which currently only provides [`Event`]
+//! subscription functionality over a WebSocket connection. See the [`/subscribe` endpoint] in the
+//! Tendermint RPC for more details. This client does not yet provide access to the RPC methods
+//! provided by the [`Client`] trait (this is planned for a future release). |
 //!
 //! ### Mock Clients
 //!

tendermint/src/account.rs

@@ -36,7 +36,7 @@ impl TryFrom<Vec<u8>> for Id {
 
     fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> {
         if value.len() != LENGTH {
-            return Err(Kind::InvalidAccountIDLength.into());
+            return Err(Kind::InvalidAccountIdLength.into());
         }
         let mut slice: [u8; LENGTH] = [0; LENGTH];
         slice.copy_from_slice(&value[..]);

tendermint/src/block/id.rs

@@ -1,7 +1,7 @@
 use crate::{
     block::parts::Header as PartSetHeader,
-    error::Error,
-    hash::{Algorithm, Hash, SHA256_HASH_SIZE},
+    error::{Error, Kind},
+    hash::{Algorithm, Hash},
 };
 use serde::{Deserialize, Serialize};
 use std::convert::{TryFrom, TryInto};
@@ -22,7 +22,12 @@ pub const PREFIX_LENGTH: usize = 10;
 /// as well as the number of parts in the block.
 ///
 /// <https://github.com/tendermint/spec/blob/d46cd7f573a2c6a2399fcab2cde981330aa63f37/spec/core/data_structures.md#blockid>
-#[derive(Serialize, Deserialize, Copy, Clone, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
+///
+/// Default implementation is an empty Id as defined by the Go implementation in
+/// https://github.com/tendermint/tendermint/blob/1635d1339c73ae6a82e062cd2dc7191b029efa14/types/block.go#L1204 .
+#[derive(
+    Serialize, Deserialize, Copy, Clone, Debug, Default, Hash, Eq, PartialEq, PartialOrd, Ord,
+)]
 pub struct Id {
     /// The block's main hash is the Merkle root of all the fields in the
     /// block header.
@@ -40,7 +45,11 @@ pub struct Id {
     /// way to propagate a large file over a gossip network.
     ///
     /// <https://github.com/tendermint/tendermint/wiki/Block-Structure#partset>
-    pub parts: Option<PartSetHeader>,
+    ///
+    /// PartSetHeader in protobuf is defined as never nil using the gogoproto
+    /// annotations. This does not translate to Rust, but we can indicate this
+    /// in the DomainType.
+    pub parts: PartSetHeader,
 }
 
 impl DomainType<RawBlockId> for Id {}
@@ -49,12 +58,12 @@ impl TryFrom<RawBlockId> for Id {
     type Error = Error;
 
     fn try_from(value: RawBlockId) -> Result<Self, Self::Error> {
+        if value.part_set_header.is_none() {
+            return Err(Kind::InvalidPartSetHeader.into());
+        }
         Ok(Self {
             hash: value.hash.try_into()?,
-            parts: match value.part_set_header {
-                None => None,
-                Some(p) => Some(p.try_into()?),
-            },
+            parts: value.part_set_header.unwrap().try_into()?,
         })
     }
 }
@@ -76,7 +85,7 @@ impl From<Id> for RawBlockId {
         } else {
             RawBlockId {
                 hash: value.hash.into(),
-                part_set_header: value.parts.map(|p| p.into()),
+                part_set_header: Some(value.parts.into()),
             }
         }
     }
@@ -86,49 +95,26 @@ impl TryFrom<RawCanonicalBlockId> for Id {
     type Error = Error;
 
     fn try_from(value: RawCanonicalBlockId) -> Result<Self, Self::Error> {
+        if value.part_set_header.is_none() {
+            return Err(Kind::InvalidPartSetHeader.into());
+        }
         Ok(Self {
             hash: value.hash.try_into()?,
-            parts: match value.part_set_header {
-                None => None,
-                Some(p) => Some(p.try_into()?),
-            },
+            parts: value.part_set_header.unwrap().try_into()?,
         })
     }
 }
 
 impl From<Id> for RawCanonicalBlockId {
-    // Todo: it's possible that an empty CanonicalBlockId is encoded differently. Test this.
     fn from(value: Id) -> Self {
         RawCanonicalBlockId {
             hash: value.hash.as_bytes().to_vec(),
-            part_set_header: match value.parts {
-                None => None,
-                Some(h) => Some(h.into()),
-            },
-        }
-    }
-}
-
-/// Default implementation is an empty Id as defined by the Go implementation in
-/// https://github.com/tendermint/tendermint/blob/1635d1339c73ae6a82e062cd2dc7191b029efa14/types/block.go#L1204 .
-impl Default for Id {
-    fn default() -> Self {
-        Id {
-            hash: Hash::new(Algorithm::Sha256, &[0; SHA256_HASH_SIZE]).unwrap(),
-            parts: Some(PartSetHeader {
-                total: 0,
-                hash: Hash::new(Algorithm::Sha256, &[0; SHA256_HASH_SIZE]).unwrap(),
-            }),
+            part_set_header: Some(value.parts.into()),
         }
     }
 }
 
 impl Id {
-    /// Create a new `Id` from a hash byte slice
-    pub fn new(hash: Hash, parts: Option<PartSetHeader>) -> Self {
-        Self { hash, parts }
-    }
-
     /// Get a shortened 12-character prefix of a block ID (ala git)
     pub fn prefix(&self) -> String {
         let mut result = self.to_string();
@@ -149,7 +135,10 @@ impl FromStr for Id {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Error> {
-        Ok(Self::new(Hash::from_hex_upper(Algorithm::Sha256, s)?, None))
+        Ok(Self {
+            hash: Hash::from_hex_upper(Algorithm::Sha256, s)?,
+            parts: PartSetHeader::default(),
+        })
     }
 }
 

tendermint/src/block/parts.rs

@@ -12,7 +12,9 @@ use tendermint_proto::types::{
 use tendermint_proto::DomainType;
 
 /// Block parts header
-#[derive(Serialize, Deserialize, Clone, Copy, Debug, Hash, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(
+    Serialize, Deserialize, Clone, Copy, Debug, Default, Hash, Eq, PartialEq, PartialOrd, Ord,
+)]
 pub struct Header {
     /// Number of parts in this block
     pub total: u32,
@@ -33,7 +35,7 @@ impl TryFrom<RawPartSetHeader> for Header {
         }
         Ok(Self {
             total: value.total,
-            hash: Hash::new(Algorithm::Sha256, &value.hash)?,
+            hash: Hash::from_bytes(Algorithm::Sha256, &value.hash)?,
         })
     }
 }
@@ -56,7 +58,7 @@ impl TryFrom<RawCanonicalPartSetHeader> for Header {
         }
         Ok(Self {
             total: value.total,
-            hash: Hash::new(Algorithm::Sha256, &value.hash)?,
+            hash: Hash::from_bytes(Algorithm::Sha256, &value.hash)?,
         })
     }
 }

tendermint/src/error.rs

@@ -41,8 +41,8 @@ pub enum Kind {
     #[error("bad signature")]
     SignatureInvalid,
 
-    /// invalid Vote type
-    #[error("invalid Type")]
+    /// invalid message type
+    #[error("invalid message type")]
     InvalidMessageType,
 
     /// Negative block height
@@ -54,15 +54,15 @@ pub enum Kind {
     NegativeRound,
 
     /// Negative POL round
-    #[error("negative pol round")]
-    NegativePOLRound,
+    #[error("negative POL round")]
+    NegativePolRound,
 
     /// Negative validator index in vote
-    #[error("negative ValidatorIndex")]
+    #[error("negative validator index")]
     NegativeValidatorIndex,
 
-    /// Wrong hash size in part_set_header
-    #[error("Wrong hash: expected Hash size to be 32 bytes")]
+    /// Invalid hash size in part_set_header
+    #[error("invalid hash: expected hash size to be 32 bytes")]
     InvalidHashSize,
 
     /// No timestamp in vote
@@ -71,27 +71,31 @@ pub enum Kind {
 
     /// Invalid account ID length
     #[error("invalid account ID length")]
-    InvalidAccountIDLength,
+    InvalidAccountIdLength,
 
     /// Invalid signature ID length
     #[error("invalid signature ID length")]
-    InvalidSignatureIDLength,
+    InvalidSignatureIdLength,
 
     /// Overflow during conversion
-    #[error("Integer overflow")]
+    #[error("integer overflow")]
     IntegerOverflow,
 
     /// No Vote found during conversion
-    #[error("No vote found")]
+    #[error("no vote found")]
     NoVoteFound,
 
     /// No Proposal found during conversion
-    #[error("No proposal found")]
+    #[error("no proposal found")]
     NoProposalFound,
 
     /// Invalid AppHash length found during conversion
-    #[error("Invalid AppHash Length")]
+    #[error("invalid app hash Length")]
     InvalidAppHashLength,
+
+    /// Invalid PartSetHeader
+    #[error("invalid part set header")]
+    InvalidPartSetHeader,
 }
 
 impl Kind {

tendermint/src/hash.rs

@@ -34,7 +34,7 @@ impl TryFrom<Vec<u8>> for Hash {
     type Error = Error;
 
     fn try_from(value: Vec<u8>) -> Result<Self, Self::Error> {
-        Hash::new(Algorithm::Sha256, &value)
+        Hash::from_bytes(Algorithm::Sha256, &value)
     }
 }
 
@@ -50,9 +50,8 @@ impl From<Hash> for Vec<u8> {
 }
 
 impl Hash {
-    #[allow(clippy::new_ret_no_self)]
     /// Create a new `Hash` with the given algorithm type
-    pub fn new(alg: Algorithm, bytes: &[u8]) -> Result<Hash, Error> {
+    pub fn from_bytes(alg: Algorithm, bytes: &[u8]) -> Result<Hash, Error> {
         match alg {
             Algorithm::Sha256 => {
                 if bytes.len() == SHA256_HASH_SIZE {
@@ -102,7 +101,7 @@ impl Debug for Hash {
 
 impl Default for Hash {
     fn default() -> Self {
-        Hash::new(Algorithm::Sha256, &[0; SHA256_HASH_SIZE]).unwrap()
+        Hash::from_bytes(Algorithm::Sha256, &[0; SHA256_HASH_SIZE]).unwrap()
     }
 }
 

tendermint/src/proposal.rs

@@ -45,7 +45,7 @@ impl TryFrom<RawProposal> for Proposal {
 
     fn try_from(value: RawProposal) -> Result<Self, Self::Error> {
         if value.pol_round < -1 {
-            return Err(Kind::NegativePOLRound.into());
+            return Err(Kind::NegativePolRound.into());
         }
         let pol_round = match value.pol_round {
             -1 => None,
@@ -58,7 +58,7 @@ impl TryFrom<RawProposal> for Proposal {
             pol_round,
             block_id: match value.block_id {
                 None => None,
-                Some(raw_block_id) => Some(BlockId::try_from(raw_block_id).unwrap()),
+                Some(raw_block_id) => Some(BlockId::try_from(raw_block_id)?),
             },
             timestamp: match value.timestamp {
                 None => None,
@@ -146,14 +146,14 @@ mod tests {
                     "DEADBEEFDEADBEEFBAFBAFBAFBAFBAFADEADBEEFDEADBEEFBAFBAFBAFBAFBAFA",
                 )
                 .unwrap(),
-                parts: Some(Header {
+                parts: Header {
                     total: 65535,
                     hash: Hash::from_hex_upper(
                         Algorithm::Sha256,
                         "0022446688AACCEE1133557799BBDDFF0022446688AACCEE1133557799BBDDFF",
                     )
                     .unwrap(),
-                }),
+                },
             }),
             timestamp: Some(dt.into()),
             signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
@@ -232,14 +232,14 @@ mod tests {
                     "DEADBEEFDEADBEEFBAFBAFBAFBAFBAFADEADBEEFDEADBEEFBAFBAFBAFBAFBAFA",
                 )
                 .unwrap(),
-                parts: Some(Header {
+                parts: Header {
                     total: 65535,
                     hash: Hash::from_hex_upper(
                         Algorithm::Sha256,
                         "0022446688AACCEE1133557799BBDDFF0022446688AACCEE1133557799BBDDFF",
                     )
                     .unwrap(),
-                }),
+                },
             }),
             signature: Signature::Ed25519(Ed25519Signature::new([0; ED25519_SIGNATURE_SIZE])),
         };

tendermint/src/proposal/canonical_proposal.rs

@@ -35,7 +35,7 @@ impl TryFrom<RawCanonicalProposal> for CanonicalProposal {
 
     fn try_from(value: RawCanonicalProposal) -> Result<Self, Self::Error> {
         if value.pol_round < -1 {
-            return Err(Kind::NegativePOLRound.into());
+            return Err(Kind::NegativePolRound.into());
         }
         let round = Round::try_from(
             i32::try_from(value.round).map_err(|e| Kind::IntegerOverflow.context(e))?,

tendermint/src/proposal/sign_proposal.rs

@@ -86,10 +86,7 @@ impl TryFrom<RawSignedProposalResponse> for SignedProposalResponse {
 impl From<SignedProposalResponse> for RawSignedProposalResponse {
     fn from(value: SignedProposalResponse) -> Self {
         RawSignedProposalResponse {
-            proposal: match value.proposal {
-                None => None,
-                Some(proposal) => Some(proposal.into()),
-            },
+            proposal: value.proposal.map(|p| p.into()),
             error: value.error,
         }
     }

tendermint/src/public_key.rs

@@ -55,19 +55,15 @@ impl DomainType<RawPubKeyResponse> for PublicKey {}
 impl TryFrom<RawPubKeyResponse> for PublicKey {
     type Error = Error;
 
-    // This does not check if the underlying pub_key_ed25519 has the right size.
-    // The caller needs to make sure that this is actually the case.
     fn try_from(value: RawPubKeyResponse) -> Result<Self, Self::Error> {
-        match &value
+        let Sum::Ed25519(b) = &value
             .pub_key
             .ok_or_else(|| format_err!(error::Kind::InvalidKey, "empty pubkey"))?
             .sum
-            .ok_or_else(|| format_err!(error::Kind::InvalidKey, "empty sum"))?
-        {
-            Sum::Ed25519(b) => Ed25519::from_bytes(b),
-        }
-        .map(Into::into)
-        .map_err(|_| format_err!(error::Kind::InvalidKey, "malformed key").into())
+            .ok_or_else(|| format_err!(error::Kind::InvalidKey, "empty sum"))?;
+        Ed25519::from_bytes(b)
+            .map(Into::into)
+            .map_err(|_| format_err!(error::Kind::InvalidKey, "malformed key").into())
     }
 }