Refactor error handling (#336)

* Refactor and clean up error handling

* Add docs and cleanup

blockchain/blocks/Cargo.toml

@@ -14,6 +14,7 @@ derive_builder = "0.9"
 serde = { version = "1.0", features = ["derive"] }
 encoding = { package = "forest_encoding", path = "../../encoding" }
 num-bigint = { path = "../../utils/bigint", package = "forest_bigint" }
+thiserror = "1.0"
 
 [dev-dependencies]
 base64 = "0.12.0"

blockchain/blocks/src/errors.rs

@@ -1,31 +1,26 @@
 // Copyright 2020 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
-use std::{fmt, time::SystemTimeError as TimeErr};
+use std::time::SystemTimeError as TimeErr;
+use thiserror::Error;
 
-#[derive(Debug, PartialEq)]
+/// Blockchain blocks error
+#[derive(Debug, PartialEq, Error)]
 pub enum Error {
     /// Tipset contains invalid data, as described by the string parameter.
+    #[error("Invalid tipset: {0}")]
     InvalidTipSet(String),
     /// The given tipset has no blocks
+    #[error("No blocks for tipset")]
     NoBlocks,
     /// Invalid signature
+    #[error("Invalid signature: {0}")]
     InvalidSignature(String),
     /// Error in validating arbitrary data
+    #[error("Error validating data: {0}")]
     Validation(String),
 }
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Error::InvalidTipSet(msg) => write!(f, "Invalid tipset: {}", msg),
-            Error::NoBlocks => write!(f, "No blocks for tipset"),
-            Error::InvalidSignature(msg) => write!(f, "Invalid signature: {}", msg),
-            Error::Validation(msg) => write!(f, "Error validating data: {}", msg),
-        }
-    }
-}
-
 impl From<TimeErr> for Error {
     fn from(e: TimeErr) -> Error {
         Error::Validation(e.to_string())

blockchain/chain/Cargo.toml

@@ -15,9 +15,12 @@ num-bigint = { path = "../../utils/bigint", package = "forest_bigint" }
 message = { package = "forest_message", path = "../../vm/message" }
 ipld_blockstore = { path = "../../ipld/blockstore" }
 ipld_amt = { path = "../../ipld/amt/" }
+thiserror = "1.0"
 
 [dev-dependencies]
 address = { package = "forest_address", path = "../../vm/address" }
 crypto = { path = "../../crypto" }
 multihash = "0.10.0"
-test_utils = { version = "0.1.0", path = "../../utils/test_utils/", features = ["test_constructors"] }
+test_utils = { version = "0.1.0", path = "../../utils/test_utils/", features = [
+    "test_constructors"
+] }

blockchain/chain/src/store/chain_store.rs

@@ -92,7 +92,7 @@ where
             let key = m.cid()?.key();
             let value = &m.marshal_cbor()?;
             if self.db.exists(&key)? {
-                return Err(Error::KeyValueStore("Keys exist".to_string()));
+                return Ok(());
             }
             self.db.write(&key, value)?
         }
@@ -127,9 +127,7 @@ where
                 let bh = BlockHeader::unmarshal_cbor(&x)?;
                 block_headers.push(bh);
             } else {
-                return Err(Error::KeyValueStore(
-                    "Key for header does not exist".to_string(),
-                ));
+                return Err(Error::NotFound("Key for header"));
             }
         }
         // construct new Tipset to return
@@ -187,7 +185,8 @@ where
                 let bytes = value.ok_or_else(|| Error::UndefinedKey(k.to_string()))?;
 
                 // Decode bytes into type T
-                from_slice(&bytes)?
+                let t = from_slice(&bytes)?;
+                Ok(t)
             })
             .collect()
     }

blockchain/chain/src/store/errors.rs

@@ -6,57 +6,35 @@ use cid::Error as CidErr;
 use db::Error as DbErr;
 use encoding::{error::Error as SerdeErr, Error as EncErr};
 use ipld_amt::Error as AmtErr;
-use serde::Deserialize;
-use std::fmt;
+use thiserror::Error;
 
-#[derive(Debug, PartialEq, Deserialize)]
+/// Chain error
+#[derive(Debug, Error, PartialEq)]
 pub enum Error {
     /// Key was not found
+    #[error("Invalid tipset: {0}")]
     UndefinedKey(String),
     /// Tipset contains no blocks
+    #[error("No blocks for tipset")]
     NoBlocks,
+    /// Key not found in database
+    #[error("{0} not found")]
+    NotFound(&'static str),
     /// Error originating from key-value store
-    KeyValueStore(String),
+    #[error(transparent)]
+    DB(#[from] DbErr),
     /// Error originating constructing blockchain structures
-    Blockchain(String),
+    #[error(transparent)]
+    Blockchain(#[from] BlkErr),
     /// Error originating from encoding arbitrary data
+    #[error("{0}")]
     Encoding(String),
     /// Error originating from Cid creation
-    Cid(String),
+    #[error(transparent)]
+    Cid(#[from] CidErr),
     /// Amt error
-    Amt(String),
-}
-
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Error::UndefinedKey(msg) => write!(f, "Invalid key: {}", msg),
-            Error::NoBlocks => write!(f, "No blocks for tipset"),
-            Error::KeyValueStore(msg) => {
-                write!(f, "Error originating from Key-Value store: {}", msg)
-            }
-            Error::Blockchain(msg) => write!(
-                f,
-                "Error originating from construction of blockchain structures: {}",
-                msg
-            ),
-            Error::Amt(msg) => write!(f, "Error originating from AMT: {}", msg),
-            Error::Encoding(msg) => write!(f, "Error originating from Encoding type: {}", msg),
-            Error::Cid(msg) => write!(f, "Error originating from from Cid creation: {}", msg),
-        }
-    }
-}
-
-impl From<DbErr> for Error {
-    fn from(e: DbErr) -> Error {
-        Error::KeyValueStore(e.to_string())
-    }
-}
-
-impl From<BlkErr> for Error {
-    fn from(e: BlkErr) -> Error {
-        Error::Blockchain(e.to_string())
-    }
+    #[error(transparent)]
+    Amt(#[from] AmtErr),
 }
 
 impl From<EncErr> for Error {
@@ -70,15 +48,3 @@ impl From<SerdeErr> for Error {
         Error::Encoding(e.to_string())
     }
 }
-
-impl From<CidErr> for Error {
-    fn from(e: CidErr) -> Error {
-        Error::Cid(e.to_string())
-    }
-}
-
-impl From<AmtErr> for Error {
-    fn from(e: AmtErr) -> Error {
-        Error::Amt(e.to_string())
-    }
-}

blockchain/chain_sync/Cargo.toml

@@ -25,6 +25,7 @@ async-std = { version = "1.4.0", features = ["unstable"] }
 forest_libp2p = { path = "../../node/forest_libp2p" }
 futures = "0.3.2"
 lru = "0.4.3"
+thiserror = "1.0"
 
 [dev-dependencies]
 test_utils = { version = "0.1.0", path = "../../utils/test_utils/", features = ["test_constructors"] }

blockchain/chain_sync/src/errors.rs

@@ -8,57 +8,42 @@ use cid::Error as CidErr;
 use db::Error as DbErr;
 use encoding::{error::Error as SerdeErr, Error as EncErr};
 use state_manager::Error as StErr;
-use std::fmt;
+use thiserror::Error;
 
-#[derive(Debug, PartialEq)]
+/// ChainSync error
+#[derive(Debug, PartialEq, Error)]
 pub enum Error {
+    #[error("No blocks for tipset")]
     NoBlocks,
     /// Error originating constructing blockchain structures
-    Blockchain(String),
+    #[error(transparent)]
+    Blockchain(#[from] BlkErr),
     /// Error originating from encoding arbitrary data
+    #[error("{0}")]
     Encoding(String),
     /// Error originating from CID construction
-    InvalidCid(String),
+    #[error(transparent)]
+    InvalidCid(#[from] CidErr),
     /// Error indicating an invalid root
+    #[error("Invalid root detected")]
     InvalidRoots,
     /// Error indicating a chain store error
-    Store(String),
-    /// Error originating from key-value store
-    KeyValueStore(String),
+    #[error(transparent)]
+    Store(#[from] StoreErr),
     /// Error originating from state
-    State(String),
+    #[error(transparent)]
+    State(#[from] StErr),
     /// Error in validating arbitrary data
-    Validation(String),
+    #[error("{0}")]
+    Validation(&'static str),
     /// Any other error that does not need to be specifically handled
+    #[error("{0}")]
     Other(String),
 }
 
-impl fmt::Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            Error::NoBlocks => write!(f, "No blocks for tipset"),
-            Error::InvalidRoots => write!(f, "Invalid root detected"),
-            Error::Blockchain(msg) => write!(f, "{}", msg),
-            Error::KeyValueStore(msg) => write!(f, "{}", msg),
-            Error::Encoding(msg) => write!(f, "{}", msg),
-            Error::InvalidCid(msg) => write!(f, "{}", msg),
-            Error::Store(msg) => write!(f, "{}", msg),
-            Error::State(msg) => write!(f, "{}", msg),
-            Error::Validation(msg) => write!(f, "{}", msg),
-            Error::Other(msg) => write!(f, "chain_sync error: {}", msg),
-        }
-    }
-}
-
-impl From<BlkErr> for Error {
-    fn from(e: BlkErr) -> Error {
-        Error::Blockchain(e.to_string())
-    }
-}
-
 impl From<DbErr> for Error {
     fn from(e: DbErr) -> Error {
-        Error::KeyValueStore(e.to_string())
+        Error::Store(e.into())
     }
 }
 
@@ -74,30 +59,12 @@ impl From<SerdeErr> for Error {
     }
 }
 
-impl From<CidErr> for Error {
-    fn from(e: CidErr) -> Error {
-        Error::InvalidCid(e.to_string())
-    }
-}
-
-impl From<StoreErr> for Error {
-    fn from(e: StoreErr) -> Error {
-        Error::Store(e.to_string())
-    }
-}
-
 impl From<AmtErr> for Error {
     fn from(e: AmtErr) -> Error {
         Error::Other(e.to_string())
     }
 }
 
-impl From<StErr> for Error {
-    fn from(e: StErr) -> Error {
-        Error::State(e.to_string())
-    }
-}
-
 impl From<&str> for Error {
     fn from(e: &str) -> Error {
         Error::Other(e.to_string())

blockchain/chain_sync/src/sync.rs

@@ -504,13 +504,13 @@ where
                 Some(MsgMetaData { sequence, balance }) => {
                     // sequence equality check
                     if *sequence != msg.sequence() {
-                        return Err(Error::Validation("Sequences are not equal".to_string()));
+                        return Err(Error::Validation("Sequences are not equal"));
                     }
 
                     // sufficient funds check
                     if *balance < msg.required_funds() {
                         return Err(Error::Validation(
-                            "Insufficient funds for message execution".to_string(),
+                            "Insufficient funds for message execution",
                         ));
                     }
                     // update balance and increment sequence by 1
@@ -523,9 +523,9 @@ where
                 None => {
                     let actor = tree
                         .get_actor(msg.from())
-                        .map_err(Error::Blockchain)?
+                        .map_err(Error::Other)?
                         .ok_or_else(|| {
-                            Error::State("Could not retrieve actor from state tree".to_owned())
+                            Error::Other("Could not retrieve actor from state tree".to_owned())
                         })?;
 
                     MsgMetaData {
@@ -551,9 +551,7 @@ where
             check_msg(m, &mut msg_meta_data, &tree)?;
             // signature validation
             if !is_valid_signature(&m.cid()?.to_bytes(), m.from(), m.signature()) {
-                return Err(Error::Validation(
-                    "Message signature is not valid".to_string(),
-                ));
+                return Err(Error::Validation("Message signature is not valid"));
             }
         }
         // validate message root from header matches message root
@@ -572,7 +570,7 @@ where
 
         // check if block has been signed
         if header.signature().bytes().is_empty() {
-            return Err(Error::Validation("Signature is nil in header".to_string()));
+            return Err(Error::Validation("Signature is nil in header"));
         }
 
         let base_tipset = self.load_fts(&TipSetKeys::new(header.parents().cids.clone()))?;
@@ -634,11 +632,7 @@ where
 
         let mut return_set = vec![head];
 
-        let to_epoch = to
-            .blocks()
-            .get(0)
-            .ok_or_else(|| Error::Blockchain("Tipset must not be empty".to_owned()))?
-            .epoch();
+        let to_epoch = to.blocks().get(0).expect("Tipset cannot be empty").epoch();
 
         // Loop until most recent tipset height is less than to tipset height
         'sync: while let Some(cur_ts) = return_set.last() {

blockchain/state_manager/Cargo.toml

@@ -13,3 +13,4 @@ state_tree = { path = "../../vm/state_tree/" }
 vm = { path = "../../vm" }
 blockstore = { package = "ipld_blockstore", path = "../../ipld/blockstore/" }
 forest_blocks = { path = "../../blockchain/blocks" }
+thiserror = "1.0"