Replace snafu with this_error and remove global error enum (#1823)

This PR: - Removes global error enum by moving the variants to a BlockBuildingError enum - removes snafu dependency from everywhere except from the availability endpoint, as hotshot query service still uses it - moves error types into their relevant modules - use parse_duration and parse_size from espresso_types::utils This PR does not remove snafu dependency completely, as we need to replace snafu with this_error in hotshot query service
EspressoSystems · Aug 7, 2024 · e52425a · e52425a
2 parents d342aff + d3e7e25
commit e52425a
Show file tree

Hide file tree

Showing 32 changed files with 133 additions and 276 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -107,7 +107,6 @@ jf-relation = { git = "https://github.com/EspressoSystems/jellyfish", tag = "0.4
 jf-utils = { git = "https://github.com/EspressoSystems/jellyfish", tag = "0.4.5" }
 libp2p = { version = "0.53", default-features = false }
 log-panics = { version = "2.0", features = ["with-backtrace"] }
-snafu = "0.8"
 strum = { version = "0.26", features = ["derive"] }
 surf-disco = "0.9"
 tagged-base64 = "0.4"

diff --git a/builder/Cargo.toml b/builder/Cargo.toml
@@ -40,7 +40,6 @@ rand = "0.8.5"
 sequencer = { path = "../sequencer", features = ["testing"] }
 sequencer-utils = { path = "../utils" }
 serde = { workspace = true }
-snafu = { workspace = true }
 surf = "2.3.1"
 surf-disco = { workspace = true }
 tide-disco = { workspace = true }

diff --git a/builder/src/bin/permissioned-builder.rs b/builder/src/bin/permissioned-builder.rs
@@ -5,7 +5,7 @@ use std::{
 use anyhow::{bail, Context};
 use builder::permissioned::init_node;
 use clap::Parser;
-use espresso_types::eth_signature_key::EthKeyPair;
+use espresso_types::{eth_signature_key::EthKeyPair, parse_duration};
 use ethers::types::Address;
 use hotshot_types::{
     data::ViewNumber,
@@ -14,9 +14,7 @@ use hotshot_types::{
     traits::{metrics::NoMetrics, node_implementation::ConsensusTime},
 };
 use libp2p::Multiaddr;
-use sequencer::{
-    options::parse_duration, persistence::no_storage::NoStorage, Genesis, L1Params, NetworkParams,
-};
+use sequencer::{persistence::no_storage::NoStorage, Genesis, L1Params, NetworkParams};
 use sequencer_utils::logging;
 use url::Url;
 use vbs::version::{StaticVersion, StaticVersionType};

diff --git a/builder/src/bin/permissionless-builder.rs b/builder/src/bin/permissionless-builder.rs
@@ -1,14 +1,12 @@
-use std::{num::NonZeroUsize, path::PathBuf, str::FromStr, time::Duration};
+use std::{num::NonZeroUsize, path::PathBuf, time::Duration};
 
 use builder::non_permissioned::{build_instance_state, BuilderConfig};
 use clap::Parser;
-use cld::ClDuration;
-use espresso_types::eth_signature_key::EthKeyPair;
+use espresso_types::{eth_signature_key::EthKeyPair, parse_duration};
 use hotshot::traits::ValidatedState;
 use hotshot_types::{data::ViewNumber, traits::node_implementation::ConsensusTime};
 use sequencer::{Genesis, L1Params};
 use sequencer_utils::logging;
-use snafu::Snafu;
 use url::Url;
 use vbs::version::{StaticVersion, StaticVersionType};
 
@@ -88,19 +86,6 @@ struct NonPermissionedBuilderOptions {
     logging: logging::Config,
 }
 
-#[derive(Clone, Debug, Snafu)]
-struct ParseDurationError {
-    reason: String,
-}
-
-fn parse_duration(s: &str) -> Result<Duration, ParseDurationError> {
-    ClDuration::from_str(s)
-        .map(Duration::from)
-        .map_err(|err| ParseDurationError {
-            reason: err.to_string(),
-        })
-}
-
 #[async_std::main]
 async fn main() -> anyhow::Result<()> {
     let opt = NonPermissionedBuilderOptions::parse();

diff --git a/builder/src/lib.rs b/builder/src/lib.rs
@@ -165,7 +165,6 @@ pub mod testing {
     use portpicker::pick_unused_port;
     use sequencer::state_signature::StateSignatureMemStorage;
     use serde::{Deserialize, Serialize};
-    use snafu::{guide::feature_flags, *};
     use vbs::version::StaticVersion;
 
     use super::*;

diff --git a/hotshot-state-prover/Cargo.toml b/hotshot-state-prover/Cargo.toml
@@ -36,7 +36,6 @@ jf-utils = { workspace = true }
 reqwest = { workspace = true }
 sequencer-utils = { path = "../utils" }
 serde = { workspace = true }
-snafu = { workspace = true }
 surf-disco = { workspace = true }
 tide-disco = { workspace = true }
 time = { workspace = true }

diff --git a/hotshot-state-prover/src/bin/state-prover.rs b/hotshot-state-prover/src/bin/state-prover.rs
@@ -1,8 +1,7 @@
-use std::{str::FromStr as _, time::Duration};
+use std::time::Duration;
 
 use clap::Parser;
-use cld::ClDuration;
-use espresso_types::SeqTypes;
+use espresso_types::{parse_duration, SeqTypes};
 use ethers::{
     providers::{Http, Middleware, Provider},
     signers::{coins_bip39::English, MnemonicBuilder, Signer},
@@ -12,7 +11,6 @@ use hotshot_stake_table::config::STAKE_TABLE_CAPACITY;
 use hotshot_state_prover::service::{run_prover_once, run_prover_service, StateProverConfig};
 use hotshot_types::traits::node_implementation::NodeType;
 use sequencer_utils::logging;
-use snafu::Snafu;
 use url::Url;
 use vbs::version::StaticVersionType;
 
@@ -85,19 +83,6 @@ struct Args {
     logging: logging::Config,
 }
 
-#[derive(Clone, Debug, Snafu)]
-pub struct ParseDurationError {
-    reason: String,
-}
-
-fn parse_duration(s: &str) -> Result<Duration, ParseDurationError> {
-    ClDuration::from_str(s)
-        .map(Duration::from)
-        .map_err(|err| ParseDurationError {
-            reason: err.to_string(),
-        })
-}
-
 #[async_std::main]
 async fn main() {
     let args = Args::parse();

diff --git a/marketplace-builder/Cargo.toml b/marketplace-builder/Cargo.toml
@@ -40,7 +40,6 @@ portpicker = { workspace = true }
 rand = "0.8.5"
 sequencer = { path = "../sequencer", features = ["testing"] }
 serde = { workspace = true }
-snafu = { workspace = true }
 surf = "2.3.1"
 surf-disco = { workspace = true }
 tagged-base64 = { workspace = true }

diff --git a/marketplace-builder/src/bin/marketplace-builder.rs b/marketplace-builder/src/bin/marketplace-builder.rs
@@ -1,9 +1,10 @@
-use std::{num::NonZeroUsize, path::PathBuf, str::FromStr, time::Duration};
+use std::{num::NonZeroUsize, path::PathBuf, time::Duration};
 
 use async_compatibility_layer::logging::{setup_backtrace, setup_logging};
 use clap::Parser;
-use cld::ClDuration;
-use espresso_types::{eth_signature_key::EthKeyPair, FeeAmount, NamespaceId, SeqTypes};
+use espresso_types::{
+    eth_signature_key::EthKeyPair, parse_duration, FeeAmount, NamespaceId, SeqTypes,
+};
 use hotshot::traits::ValidatedState;
 use hotshot_types::{data::ViewNumber, traits::node_implementation::ConsensusTime};
 use marketplace_builder::{
@@ -12,7 +13,6 @@ use marketplace_builder::{
 };
 use marketplace_builder_core::testing::basic_test::NodeType;
 use sequencer::{Genesis, L1Params};
-use snafu::Snafu;
 use url::Url;
 use vbs::version::StaticVersionType;
 
@@ -118,19 +118,6 @@ struct NonPermissionedBuilderOptions {
     bid_amount: FeeAmount,
 }
 
-#[derive(Clone, Debug, Snafu)]
-struct ParseDurationError {
-    reason: String,
-}
-
-fn parse_duration(s: &str) -> Result<Duration, ParseDurationError> {
-    ClDuration::from_str(s)
-        .map(Duration::from)
-        .map_err(|err| ParseDurationError {
-            reason: err.to_string(),
-        })
-}
-
 #[async_std::main]
 async fn main() -> anyhow::Result<()> {
     setup_logging();

diff --git a/marketplace-solver/src/options.rs b/marketplace-solver/src/options.rs
@@ -1,7 +1,7 @@
-use std::{str::FromStr, time::Duration};
+use std::time::Duration;
 
 use clap::Parser;
-use thiserror::Error;
+use espresso_types::parse_duration;
 use tide_disco::Url;
 
 use crate::database::PostgresClient;
@@ -68,13 +68,3 @@ impl DatabaseOptions {
         PostgresClient::connect(self).await
     }
 }
-
-#[derive(Clone, Debug, Error)]
-#[error("failed to parse `{0}`")]
-pub struct ParseDurationError(String);
-
-pub fn parse_duration(s: &str) -> Result<Duration, ParseDurationError> {
-    cld::ClDuration::from_str(s)
-        .map(Duration::from)
-        .map_err(|err| ParseDurationError(err.to_string()))
-}
diff --git a/node-metrics/Cargo.toml b/node-metrics/Cargo.toml
@@ -21,6 +21,9 @@ futures = { workspace = true }
 hotshot = { workspace = true }
 hotshot-query-service = { workspace = true }
 hotshot-stake-table = { workspace = true }
+
+# Dependencies for feature `testing`
+hotshot-testing = { workspace = true, optional = true }
 hotshot-types = { workspace = true }
 prometheus-parse = { version = "^0.2.5" }
 reqwest = { workspace = true }
@@ -34,6 +37,3 @@ toml = { workspace = true }
 tracing = { workspace = true }
 url = { workspace = true }
 vbs = { workspace = true }
-
-# Dependencies for feature `testing`
-hotshot-testing = { workspace = true, optional = true }
diff --git a/sequencer/Cargo.toml b/sequencer/Cargo.toml
@@ -88,7 +88,7 @@ sequencer-utils = { path = "../utils" }
 serde = { workspace = true }
 serde_json = { workspace = true }
 sha2 = "0.10"                                                      # TODO temporary, used only for VID, should be set in hotshot
-snafu = { workspace = true }
+snafu = "0.8"
 static_assertions = "1"
 strum = { workspace = true }
 surf-disco = { workspace = true }
@@ -101,7 +101,6 @@ tracing-subscriber = "0.3.18"
 url = { workspace = true }
 vbs = { workspace = true }
 vec1 = { workspace = true }
-
 [package.metadata.cargo-udeps.ignore]
 normal = ["hotshot-testing"]
 

diff --git a/sequencer/src/api/endpoints.rs b/sequencer/src/api/endpoints.rs
@@ -51,6 +51,9 @@ pub(super) type AvailState<N, P, D, Ver> = Arc<RwLock<StorageState<N, P, D, Ver>
 
 type AvailabilityApi<N, P, D, Ver> = Api<AvailState<N, P, D, Ver>, availability::Error, Ver>;
 
+// TODO (abdul): replace snafu with `this_error` in  hotshot query service
+// Snafu has been replaced by `this_error` everywhere.
+// However, the query service still uses snafu
 pub(super) fn availability<N, P, D, Ver: StaticVersionType + 'static>(
     bind_version: Ver,
 ) -> Result<AvailabilityApi<N, P, D, Ver>>

diff --git a/sequencer/src/bin/cdn-broker.rs b/sequencer/src/bin/cdn-broker.rs
@@ -3,12 +3,9 @@
 use anyhow::{Context, Result};
 use cdn_broker::{reexports::crypto::signature::KeyPair, Broker, Config};
 use clap::Parser;
-use espresso_types::SeqTypes;
+use espresso_types::{parse_size, SeqTypes};
 use hotshot_types::traits::{node_implementation::NodeType, signature_key::SignatureKey};
-use sequencer::{
-    network::cdn::{ProductionDef, WrappedSignatureKey},
-    options::parse_size,
-};
+use sequencer::network::cdn::{ProductionDef, WrappedSignatureKey};
 use sha2::Digest;
 use tracing_subscriber::EnvFilter;
 

diff --git a/sequencer/src/bin/cdn-marshal.rs b/sequencer/src/bin/cdn-marshal.rs
@@ -4,8 +4,8 @@
 use anyhow::{Context, Result};
 use cdn_marshal::{Config, Marshal};
 use clap::Parser;
-use espresso_types::SeqTypes;
-use sequencer::{network::cdn::ProductionDef, options::parse_size};
+use espresso_types::{parse_size, SeqTypes};
+use sequencer::network::cdn::ProductionDef;
 use tracing_subscriber::EnvFilter;
 
 #[derive(Parser, Debug)]

diff --git a/sequencer/src/bin/commitment-task.rs b/sequencer/src/bin/commitment-task.rs
@@ -2,14 +2,11 @@ use std::{io, time::Duration};
 
 use async_std::task::spawn;
 use clap::Parser;
-use espresso_types::SeqTypes;
+use espresso_types::{parse_duration, SeqTypes};
 use ethers::prelude::*;
 use futures::FutureExt;
 use hotshot_types::traits::node_implementation::NodeType;
-use sequencer::{
-    hotshot_commitment::{run_hotshot_commitment_task, CommitmentTaskOptions},
-    options::parse_duration,
-};
+use sequencer::hotshot_commitment::{run_hotshot_commitment_task, CommitmentTaskOptions};
 use sequencer_utils::logging;
 use tide_disco::{error::ServerError, Api};
 use url::Url;

diff --git a/sequencer/src/bin/nasty-client.rs b/sequencer/src/bin/nasty-client.rs
@@ -20,7 +20,9 @@ use async_std::{
 use clap::Parser;
 use committable::Committable;
 use derivative::Derivative;
-use espresso_types::{v0_3::IterableFeeInfo, BlockMerkleTree, FeeMerkleTree, Header, SeqTypes};
+use espresso_types::{
+    parse_duration, v0_3::IterableFeeInfo, BlockMerkleTree, FeeMerkleTree, Header, SeqTypes,
+};
 use futures::{
     future::{FutureExt, TryFuture, TryFutureExt},
     stream::{Peekable, StreamExt},
@@ -38,7 +40,7 @@ use jf_merkle_tree::{
     ForgetableMerkleTreeScheme, MerkleCommitment, MerkleTreeScheme, UniversalMerkleTreeScheme,
 };
 use rand::{seq::SliceRandom, RngCore};
-use sequencer::{api::endpoints::NamespaceProofQueryData, options::parse_duration};
+use sequencer::api::endpoints::NamespaceProofQueryData;
 use sequencer_utils::logging;
 use serde::de::DeserializeOwned;
 use std::{

diff --git a/sequencer/src/bin/orchestrator.rs b/sequencer/src/bin/orchestrator.rs
@@ -2,13 +2,12 @@ use std::{num::NonZeroUsize, time::Duration};
 
 use clap::Parser;
 use derive_more::From;
-use espresso_types::PubKey;
+use espresso_types::{parse_duration, PubKey, Ratio};
 use ethers::utils::hex::{self, FromHexError};
 use hotshot_orchestrator::{
     config::{Libp2pConfig, NetworkConfig},
     run_orchestrator,
 };
-use sequencer::options::{parse_duration, Ratio};
 use sequencer_utils::logging;
 use snafu::Snafu;
 use url::Url;

diff --git a/sequencer/src/bin/submit-transactions.rs b/sequencer/src/bin/submit-transactions.rs
@@ -8,7 +8,7 @@ use std::{
 use async_std::task::{sleep, spawn};
 use clap::Parser;
 use committable::{Commitment, Committable};
-use espresso_types::{SeqTypes, Transaction};
+use espresso_types::{parse_duration, parse_size, SeqTypes, Transaction};
 use futures::{
     channel::mpsc::{self, Sender},
     sink::SinkExt,
@@ -19,7 +19,6 @@ use hotshot_types::traits::node_implementation::NodeType;
 use rand::{Rng, RngCore, SeedableRng};
 use rand_chacha::ChaChaRng;
 use rand_distr::Distribution;
-use sequencer::options::{parse_duration, parse_size};
 use sequencer_utils::logging;
 use surf_disco::{Client, Url};
 use tide_disco::{error::ServerError, App};