From 06670107b936558a4d2d8203cef935b5d5cd9e32 Mon Sep 17 00:00:00 2001 From: Aleksandr Petrosyan Date: Wed, 22 Mar 2023 10:35:27 +0400 Subject: [PATCH] [fix] #3130: Fix the UI/UX of missing configuration parameters Signed-off-by: Aleksandr Petrosyan --- Cargo.lock | 1 + cli/src/lib.rs | 150 +++++++++++++-------------- cli/src/main.rs | 77 +++++++++++++- config/base/Cargo.toml | 1 + config/base/derive/src/documented.rs | 8 +- config/base/derive/src/proxy.rs | 13 +-- config/base/derive/src/view.rs | 2 +- config/base/src/lib.rs | 110 +++++++++++++------- config/src/client.rs | 64 ++++++++---- config/src/iroha.rs | 48 +++++---- config/src/lib.rs | 2 +- config/src/path.rs | 18 +++- core/src/smartcontracts/wasm.rs | 2 +- crypto/src/lib.rs | 2 +- data_model/derive/src/api.rs | 1 + logger/src/lib.rs | 1 + 16 files changed, 321 insertions(+), 179 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index da45af262d4..2ee89f9b958 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2072,6 +2072,7 @@ version = "2.0.0-pre-rc.13" dependencies = [ "crossbeam", "iroha_config_derive", + "iroha_crypto", "json5", "serde", "serde_json", diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 67fe8d99edb..e266723e179 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -12,8 +12,10 @@ use core::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use color_eyre::eyre::{eyre, Result, WrapErr}; -use eyre::ContextCompat as _; +use color_eyre::{ + eyre::{eyre, Result, WrapErr}, + Section, +}; use iroha_actor::{broker::*, prelude::*}; use iroha_config::{ base::proxy::{LoadFromDisk, LoadFromEnv, Override}, @@ -31,8 +33,9 @@ use iroha_core::{ IrohaNetwork, }; use iroha_data_model::prelude::*; -use iroha_genesis::{GenesisNetwork, GenesisNetworkTrait, RawGenesisBlock}; +use iroha_genesis::GenesisNetwork; use iroha_p2p::network::NetworkBaseRelayOnlinePeers; +use owo_colors::OwoColorize as _; use tokio::{ signal, sync::{broadcast, Notify}, @@ -154,77 +157,11 @@ impl Handler for FromNetworkBaseRelay { } impl Iroha { - /// To make `Iroha` peer work all actors should be started first. - /// After that moment it you can start it with listening to torii events. - /// - /// # Side effect - /// - Prints welcome message in the log - /// - /// # Errors - /// - Reading genesis from disk - /// - Reading telemetry configs - /// - telemetry setup - /// - Initialization of [`Sumeragi`] - #[allow(clippy::non_ascii_literal)] - pub async fn new(args: &Arguments) -> Result { - let mut config = args - .config_path - .first_existing_path() - .map_or_else( - || { - eprintln!( - "Configuration file not found. Using environment variables as fallback." - ); - ConfigurationProxy::default() - }, - |path| ConfigurationProxy::from_path(&path.as_path()), - ) - .override_with(ConfigurationProxy::from_env()) - .build()?; - - if style::should_disable_color() { - config.disable_panic_terminal_colors = true; - // Remove terminal colors to comply with XDG - // specifications, Rust's conventions as well as remove - // escape codes from logs redirected from STDOUT. If you - // need syntax highlighting, use JSON logging instead. - config.logger.terminal_colors = false; - } - - let telemetry = iroha_logger::init(&config.logger)?; - iroha_logger::info!( - git_commit_sha = env!("VERGEN_GIT_SHA"), - "Hyperledgerいろは2にようこそ！(translation) Welcome to Hyperledger Iroha {}!", - env!("CARGO_PKG_VERSION") - ); - - let genesis = if let Some(genesis_path) = &args.genesis_path { - GenesisNetwork::from_configuration( - args.submit_genesis, - RawGenesisBlock::from_path( - genesis_path - .first_existing_path() - .wrap_err_with(|| { - format!("Genesis block file {genesis_path:?} doesn't exist") - })? - .as_ref(), - )?, - Some(&config.genesis), - &config.sumeragi.transaction_limits, - ) - .wrap_err("Failed to initialize genesis.")? - } else { - None - }; - - Self::with_genesis(genesis, config, Broker::new(), telemetry).await - } - fn prepare_panic_hook(notify_shutdown: Arc) { #[cfg(not(feature = "test-network"))] use std::panic::set_hook; - // This is a hot-fix for tests + // Hotfix for tests. // // # Problem // @@ -232,15 +169,13 @@ impl Iroha { // the same for all threads. That means, that panic in one test can // cause another test shutdown, which we don't want. // - // # Downside + // # Solution drawbacks // // A downside of this approach is that this panic hook will not work for // threads created by Iroha itself (e.g. Sumeragi thread). // - // # TODO - // - // Remove this when all Rust integrations tests will be converted to a - // separate Python tests. + // # TODO: Remove this when all Rust integrations tests will + // be converted to a separate Python tests. #[cfg(feature = "test-network")] use thread_local_panic_hook::set_hook; @@ -497,6 +432,71 @@ impl Iroha { } } +/// Combine configs from as many sources as are available. +/// +/// # Errors +/// If a necessary field isn't specified or specified with an error in +/// the overriding structure, report an error +pub fn combine_configs(args: &Arguments) -> Result { + let config = { + let colors = style::Styling::new(); + let config = args + .config_path + .first_existing_path() + .map_or_else( + || { + eprintln!( + "Configuration file not found at {}.\n{}\n", + args.config_path.style(colors.highlight), + "Falling back to environment variables".style(colors.highlight), + ); + ConfigurationProxy::default() + }, + |path| ConfigurationProxy::from_path(&path.as_path()), + ) + .override_with(ConfigurationProxy::from_env()) + .build(); + + match config { + Ok(mut config) => { + if style::should_disable_color() { + config.disable_panic_terminal_colors = true; + // Remove terminal colors to comply with XDG + // specifications, Rust's conventions as well as remove + // escape codes from logs redirected from STDOUT. If you + // need syntax highlighting, use JSON logging instead. + config.logger.terminal_colors = false; + } + config + } + Err(e) => { + use iroha_config::base::derive::Error::*; + + color_eyre::install()?; + + return Err(match e { + UnknownField(_) => eyre!(e) + .wrap_err("Failed to combine configurations.") + .suggestion("Double check the spelling of fields"), + MissingField { message, .. } | ProvidedInferredField { message, .. } => { + eyre!(e) + .wrap_err("Failed to combine configurations.") + .suggestion(message) + } + InsaneValue { ref message, .. } => { + let msg = message.clone(); + eyre!(e) + .wrap_err("Failed to combine configurations") + .suggestion(msg) + } + _ => eyre!(e).wrap_err("Failed to combine configurations."), + }); + } + } + }; + Ok(config) +} + fn genesis_account(public_key: iroha_crypto::PublicKey) -> Account { Account::new(AccountId::genesis(), [public_key]).build() } diff --git a/cli/src/main.rs b/cli/src/main.rs index 35445603f23..7bebfca033a 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -2,8 +2,10 @@ #![allow(clippy::print_stdout)] use std::env; +use color_eyre::eyre::WrapErr as _; use iroha::style::Styling; use iroha_config::path::Path as ConfigPath; +use iroha_genesis::{GenesisNetwork, GenesisNetworkTrait as _, RawGenesisBlock}; use owo_colors::OwoColorize as _; const HELP_ARG: [&str; 2] = ["--help", "-h"]; @@ -27,6 +29,17 @@ const REQUIRED_ENV_VARS: [(&str, &str); 7] = [ ]; #[tokio::main] +/// To make `Iroha` peer work all actors should be started first. +/// After that moment it you can start it with listening to torii events. +/// +/// # Side effect +/// - Prints welcome message in the log +/// +/// # Errors +/// - Reading genesis from disk +/// - Reading telemetry configs +/// - telemetry setup +/// - Initialization of [`Sumeragi`] async fn main() -> Result<(), color_eyre::Report> { let styling = Styling::new(); let mut args = iroha::Arguments::default(); @@ -43,7 +56,12 @@ async fn main() -> Result<(), color_eyre::Report> { if env::args().any(|a| SUBMIT_ARG.contains(&a.as_str())) { args.submit_genesis = true; if let Ok(genesis_path) = env::var("IROHA2_GENESIS_PATH") { - args.genesis_path = Some(ConfigPath::user_provided(&genesis_path)?); + args.genesis_path = + Some(ConfigPath::user_provided(&genesis_path) + .map_err(|e| {color_eyre::install().expect("CRITICAL"); e}) + .wrap_err_with( + || + format!("Could not read `{genesis_path}`, which is required, given you requested `--submit-genesis` on the command line."))?); } } else { args.genesis_path = None; @@ -52,10 +70,19 @@ async fn main() -> Result<(), color_eyre::Report> { for arg in env::args().skip(1) { if !arg.is_empty() && !([HELP_ARG, SUBMIT_ARG] - .iter() - .any(|group| group.contains(&arg.as_str()))) + .iter() + .any(|group| group.contains(&arg.as_str()))) { print_help(&styling)?; + // NOTE: because of how `color_eyre` works, we need to + // install the hook before creating any instance of + // `eyre::Report`, otherwise the `eyre` default reporting + // hook is going to be installed automatically. + + // This results in a nasty repetition of the + // `color_eyre::install().unwrap()` pattern, which is the + // lesser of two evils + color_eyre::install().expect("CRITICAL"); eyre::bail!( "Unrecognised command-line flag `{}`", arg.style(styling.negative) @@ -64,7 +91,12 @@ async fn main() -> Result<(), color_eyre::Report> { } if let Ok(config_path) = env::var("IROHA2_CONFIG_PATH") { - args.config_path = ConfigPath::user_provided(&config_path)?; + args.config_path = ConfigPath::user_provided(&config_path) + .map_err(|e| { + color_eyre::install().expect("CRITICAL"); + e + }) + .wrap_err_with(|| format!("Failed to parse `{config_path}` as configuration path"))?; } if !args.config_path.exists() { // Require all the fields defined in default `config.json` @@ -84,7 +116,42 @@ async fn main() -> Result<(), color_eyre::Report> { } } - ::new(&args).await?.start().await?; + // ::new(&args).await?.start().await?; + let config = iroha::combine_configs(&args)?; + let telemetry = iroha_logger::init(&config.logger)?; + iroha_logger::info!( + git_commit_sha = env!("VERGEN_GIT_SHA"), + "Hyperledgerいろは2にようこそ！(translation) Welcome to Hyperledger Iroha {}!", + env!("CARGO_PKG_VERSION") + ); + + let genesis = if let Some(genesis_path) = &args.genesis_path { + GenesisNetwork::from_configuration( + args.submit_genesis, + RawGenesisBlock::from_path( + genesis_path + .first_existing_path() + .ok_or({ + color_eyre::install().expect("CRITICAL"); + color_eyre::eyre::eyre!("Genesis block file {genesis_path:?} doesn't exist") + })? + .as_ref(), + )?, + Some(&config.genesis), + &config.sumeragi.transaction_limits, + ) + .wrap_err("Failed to initialize genesis.")? + } else { + None + }; + + iroha::Iroha::with_genesis( + genesis, + config, + iroha_actor::broker::Broker::new(), + telemetry, + ) + .await?; Ok(()) } diff --git a/config/base/Cargo.toml b/config/base/Cargo.toml index a7d0ef961ef..de645864c81 100644 --- a/config/base/Cargo.toml +++ b/config/base/Cargo.toml @@ -9,6 +9,7 @@ license.workspace = true [dependencies] iroha_config_derive = { path = "derive" } +iroha_crypto = { version = "=2.0.0-pre-rc.13", path = "../../crypto" } serde = { version = "1.0.151", default-features = false, features = ["derive"] } serde_json = "1.0.91" diff --git a/config/base/derive/src/documented.rs b/config/base/derive/src/documented.rs index 91e8d144c3d..3720229cfad 100644 --- a/config/base/derive/src/documented.rs +++ b/config/base/derive/src/documented.rs @@ -193,7 +193,13 @@ fn impl_get_recursive(ast: &StructWithFields) -> proc_macro2::TokenStream { quote! { [stringify!(#ident)] => { serde_json::to_value(&#l_value) - .map_err(|e| ::iroha_config_base::derive::Error::field_error(stringify!(#ident), e))? + .map_err( + |error| + ::iroha_config_base::derive::Error::FieldDeserialization { + field: stringify!(#ident), + error + } + )? } #inner_thing2 } diff --git a/config/base/derive/src/proxy.rs b/config/base/derive/src/proxy.rs index 9b710e25a68..16459d3da0b 100644 --- a/config/base/derive/src/proxy.rs +++ b/config/base/derive/src/proxy.rs @@ -92,7 +92,7 @@ pub fn impl_load_from_env(ast: &StructWithFields) -> TokenStream { false }; let err_ty = quote! { ::iroha_config_base::derive::Error }; - let err_variant = quote! { ::iroha_config_base::derive::Error::SerdeError }; + let err_variant = quote! { ::iroha_config_base::derive::Error::Json5 }; let inner = if is_string { quote! { Ok(var) } } else if as_str_attr { @@ -152,8 +152,8 @@ pub fn impl_load_from_disk(ast: &StructWithFields) -> TokenStream { let proxy_name = &ast.ident; let disk_trait = quote! { ::iroha_config_base::proxy::LoadFromDisk }; let error_ty = quote! { ::iroha_config_base::derive::Error }; - let disk_err_variant = quote! { ::iroha_config_base::derive::Error::DiskError }; - let serde_err_variant = quote! { ::iroha_config_base::derive::Error::SerdeError }; + let disk_err_variant = quote! { ::iroha_config_base::derive::Error::Disk }; + let serde_err_variant = quote! { ::iroha_config_base::derive::Error::Json5 }; let none_proxy = gen_none_fields_proxy(ast); quote! { impl #disk_trait for #proxy_name { @@ -243,21 +243,22 @@ pub fn impl_build(ast: &StructWithFields) -> TokenStream { fn gen_none_fields_check(ast: &StructWithFields) -> proc_macro2::TokenStream { let checked_fields = ast.fields.iter().map(|field| { let ident = &field.ident; - let err_variant = quote! { ::iroha_config_base::derive::Error::ProxyBuildError }; + let missing_field = quote! { ::iroha_config_base::derive::Error::MissingField }; if field.has_inner { let inner_ty = get_inner_type("Option", &field.ty); let builder_trait = quote! { ::iroha_config_base::proxy::Builder }; quote! { #ident: <#inner_ty as #builder_trait>::build( self.#ident.ok_or( - #err_variant(stringify!(#ident).to_owned()) + #missing_field{field: stringify!(#ident), message: ""} )? )? } } else { quote! { #ident: self.#ident.ok_or( - #err_variant(stringify!(#ident).to_owned()))? + #missing_field{field: stringify!(#ident), message: ""} + )? } } }); diff --git a/config/base/derive/src/view.rs b/config/base/derive/src/view.rs index 674ef3f8503..05d954c12eb 100644 --- a/config/base/derive/src/view.rs +++ b/config/base/derive/src/view.rs @@ -123,7 +123,7 @@ mod gen { const _: () = { use iroha_config_base::view::NoView; #( - const _: () = assert!(!iroha_config_base::view::IsHasView::<#field_types>::IS_HAS_VIEW, #messages); + const _: () = assert!(!iroha_config_base::view::IsInstanceHasView::<#field_types>::IS_HAS_VIEW, #messages); )* }; } diff --git a/config/base/src/lib.rs b/config/base/src/lib.rs index f1649a61fa1..28be6cef8e8 100644 --- a/config/base/src/lib.rs +++ b/config/base/src/lib.rs @@ -1,5 +1,4 @@ //! Package for managing iroha configuration -#![allow(clippy::std_instead_of_core)] use std::{fmt::Debug, path::Path}; use serde::{de::DeserializeOwned, Deserialize, Deserializer, Serialize}; @@ -300,16 +299,10 @@ pub mod derive { use serde::Deserialize; use thiserror::Error; - /// Error related to deserializing specific field - #[derive(Debug, Error)] - #[error("Name of the field: {}", .field)] - pub struct FieldError { - /// Field name (known at compile time) - pub field: &'static str, - /// Serde json error - #[source] - pub error: serde_json::Error, - } + // TODO: use VERGEN to point to LTS reference on LTS branch + /// Reference to the current Dev branch configuration + pub static CONFIG_REFERENCE: &str = + "https://github.com/hyperledger/iroha/blob/iroha2-dev/docs/source/references/config.md"; // TODO: deal with `#[serde(skip)]` /// Derive `Configurable` and `Proxy` error @@ -317,37 +310,71 @@ pub mod derive { #[allow(clippy::enum_variant_names)] pub enum Error { /// Used in [`Documented`] trait for wrong query errors - #[error("Got unknown field: `{}`", Self::concat_error_string(.0))] + #[error("Got unknown field: `{}`", .0.join("."))] UnknownField(Vec), + /// Used in [`Documented`] trait for deserialization errors /// while retaining field info - #[error("Failed to (de)serialize the field: {}", .0.field)] + #[error("Failed to (de)serialize the field: {}", .field)] #[serde(skip)] - FieldError(#[from] FieldError), - /// Used in [`Builder`] trait for build errors - #[error("Proxy failed at build stage due to: {0}")] - ProxyBuildError(String), - /// Used in the [`LoadFromDisk`](`crate::proxy::LoadFromDisk`) trait for file read errors - #[error("Reading file from disk failed: {0}")] + FieldDeserialization { + /// Field name (known at compile time) + field: &'static str, + /// Serde json error + #[source] + error: serde_json::Error, + }, + + /// When a field is missing. + #[error("Please add `{}` to the configuration.", .field)] #[serde(skip)] - DiskError(#[from] std::io::Error), - /// Used in [`LoadFromDisk`](`crate::proxy::LoadFromDisk`) trait for deserialization errors - #[error("Deserializing JSON failed: {0}")] + MissingField { + /// Field name + field: &'static str, + /// Additional message to be added as `color_eyre::suggestion` + message: &'static str, + }, + + /// Key pair creation failed, most likely because the keys don't form a pair + #[error("Key pair creation failed")] + Crypto(#[from] iroha_crypto::Error), + + // IMO this variant should not exist. If the value is inferred, we should only warn people if the inferred value is different from the provided one. + /// Inferred field was provided by accident and we don't want it to be provided, because the value is inferred from other fields + #[error("You should remove the field `{}` as its value is determined by other configuration parameters.", .field)] #[serde(skip)] - SerdeError(#[from] json5::Error), - } + ProvidedInferredField { + /// Field name + field: &'static str, + /// Additional message to be added as `color_eyre::suggestion` + message: &'static str, + }, - impl Error { - /// Construct a field error - pub const fn field_error(field: &'static str, error: serde_json::Error) -> Self { - Self::FieldError(FieldError { field, error }) - } + /// Value that is unacceptable to Iroha was encountered when deserializing the config + #[error("The value {} of {} is wrong. \nPlease change the value.", .value, .field)] + #[serde(skip)] + InsaneValue { + /// The value of the field that's incorrect + value: String, + /// Field name that contains invalid value + field: &'static str, + /// Additional message to be added as `color_eyre::suggestion` + message: String, + // docstring: &'static str, // TODO: Inline the docstring for easy access + }, - /// To be used for [`Self::UnknownField`] variant construction. - #[inline] - pub fn concat_error_string(field: &[String]) -> String { - field.join(".") - } + // /// Used in [`Builder`] trait for build errors + // #[error("Proxy failed at build stage due to: {0}")] + // ProxyBuildError(String), + /// Used in the [`LoadFromDisk`](`crate::proxy::LoadFromDisk`) trait for file read errors + #[error("Reading file from disk failed.")] + #[serde(skip)] + Disk(#[from] std::io::Error), + + /// Used in [`LoadFromDisk`](`crate::proxy::LoadFromDisk`) trait for deserialization errors + #[error("Deserializing JSON failed")] + #[serde(skip)] + Json5(#[from] json5::Error), } } @@ -356,24 +383,27 @@ pub mod runtime_upgrades; pub mod view { //! Module for view related traits and structs - /// Marker trait to set default value `IS_HAS_VIEW` to `false` + /// Marker trait to set default value [`IsInstanceHasView::IS_INSTANCE_HAS_VIEW`] to `false` pub trait NoView { /// [`Self`] doesn't implement [`HasView`] const IS_HAS_VIEW: bool = false; } + impl NoView for T {} /// Marker traits for types for which views are implemented pub trait HasView {} /// Wrapper structure used to check if type implements `[HasView]` - /// If `T` doesn't implement [`HasView`] then `NoView::IS_HAS_VIEW` (`false`) will be used - /// Otherwise `IsHasView::IS_HAS_VIEW` (`true`) from `impl` block will shadow `NoView::IS_HAS_VIEW` - pub struct IsHasView(core::marker::PhantomData); + /// If `T` doesn't implement [`HasView`] then + /// [`NoView::IS_INSTANCE_HAS_VIEW`] (`false`) will be used Otherwise + /// [`IsInstanceHasView::IS_INSTANCE_HAS_VIEW`] (`true`) from `impl` block + /// will shadow `NoView::IS_INSTANCE_HAS_VIEW` + pub struct IsInstanceHasView(core::marker::PhantomData); - impl IsHasView { + impl IsInstanceHasView { /// `T` implements trait [`HasView`] - pub const IS_HAS_VIEW: bool = true; + pub const IS_INSTANCE_HAS_VIEW: bool = true; } } diff --git a/config/src/client.rs b/config/src/client.rs index 470c00d36d8..5e222ee8b30 100644 --- a/config/src/client.rs +++ b/config/src/client.rs @@ -124,20 +124,30 @@ impl ConfigurationProxy { if let Some(tx_ttl) = self.transaction_time_to_live_ms { // Really small TTL would be detrimental to performance if tx_ttl < TTL_TOO_SMALL_THRESHOLD { - eyre::bail!( - ConfigError::ProxyBuildError("`TRANSACTION_TIME_TO_LIVE_MS`, network throughput may be compromised for values less than {TTL_TOO_SMALL_THRESHOLD}".to_owned()) - ); + eyre::bail!(ConfigError::InsaneValue { + field: "TRANSACTION_TIME_TO_LIVE_MS", + value: format!("{tx_ttl}"), + message: format!(", because if it's smaller than {TTL_TOO_SMALL_THRESHOLD}, Iroha wouldn't be able to produce blocks on time.") + }); } // Timeouts bigger than transaction TTL don't make sense as then transaction would be discarded before this timeout if let Some(timeout) = self.transaction_status_timeout_ms { if timeout > tx_ttl { - eyre::bail!(ConfigError::ProxyBuildError("`TRANSACTION_STATUS_TIMEOUT_MS`: {timeout} bigger than `TRANSACTION_TIME_TO_LIVE_MS`: {self.transaction_status_timeout_ms}. Consider making it smaller".to_owned())); + eyre::bail!(ConfigError::InsaneValue { + field: "TRANSACTION_STATUS_TIMEOUT_MS", + value: format!("{timeout}"), + message: format!(", because it should be smaller than `TRANSACTION_TIME_TO_LIVE_MS`, which is {tx_ttl}") + }) } } } if let Some(tx_limits) = self.transaction_limits { if *tx_limits.max_wasm_size_bytes() < WASM_SIZE_TOO_SMALL_THRESHOLD { - eyre::bail!(ConfigError::ProxyBuildError("`TRANSACTION_LIMITS` parameter's `max_wasm_size` field too small at {tx_limits.max_wasm_size_bytes}. Consider making it bigger than {WASM_SIZE_TOO_SMALL_THRESHOLD}".to_owned())); + eyre::bail!(ConfigError::InsaneValue { + field: "TRANSACTION_LIMITS", + value: format!("{}", tx_limits.max_wasm_size_bytes), + message: String::new() + }); } } if let Some(api_url) = &self.torii_api_url { @@ -147,16 +157,20 @@ impl ConfigurationProxy { Some((protocol, _)) => { // TODO: this is neither robust, nor useful. This should be enforced as a `FromStr` implementation. if protocol != "http" { - eyre::bail!(ConfigError::ProxyBuildError( - "`TORII_API_URL` string: `{api_url}` only supports the `HTTP` protocol currently".to_owned() - )); + eyre::bail!(ConfigError::InsaneValue { + field: "TORII_API_URL", + value: api_url.to_string(), + message: ", because we only support HTTP".to_owned(), + }); } } _ => { - eyre::bail!(ConfigError::ProxyBuildError( - "`TORII_API_URL` string: `{api_url}` should provide a connection protocol" - .to_owned() - )); + eyre::bail!(ConfigError::InsaneValue { + field: "TORII_API_URL", + value: api_url.to_string(), + message: ", because it's missing the connection protocol (e.g. `http://`)" + .to_owned(), + }); } } } @@ -166,21 +180,27 @@ impl ConfigurationProxy { match split { Some((protocol, endpoint)) => { if protocol != "http" { - eyre::bail!(ConfigError::ProxyBuildError( - "`TORII_TELEMETRY_URL` string: `{telemetry_url}` only supports HTTP" - .to_owned() - )); + eyre::bail!(ConfigError::InsaneValue { + field: "TORII_TELEMETRY_URL", + value: telemetry_url.to_string(), + message: ", because we only support HTTP".to_owned(), + }); } if endpoint.split(':').count() != 2 { - eyre::bail!(ConfigError::ProxyBuildError( - "`TORII_TELEMETRY_URL` string: `{telemetry_url}` should provide a connection port, e.g. `http://127.0.0.1:8180`".to_owned() - )); + eyre::bail!(ConfigError::InsaneValue{ + value: telemetry_url.to_string(), + field: "TORII_TELEMETRY_URL", + message: ". You haven't provided a connection port, e.g. `8180` in `http://127.0.0.1:8180`".to_owned(), + }); } } _ => { - eyre::bail!(ConfigError::ProxyBuildError( - "`TORII_TELEMETRY_URL` string: `{telemetry_url}` should provide a connection protocol".to_owned() - )); + eyre::bail!(ConfigError::InsaneValue { + value: telemetry_url.to_string(), + field: "TORII_TELEMETRY_URL", + message: ", because it's missing the connection protocol (e.g. `http://`)" + .to_owned() + }); } } } diff --git a/config/src/iroha.rs b/config/src/iroha.rs index dd5968306ac..55236157253 100644 --- a/config/src/iroha.rs +++ b/config/src/iroha.rs @@ -2,7 +2,6 @@ #![allow(clippy::std_instead_of_core)] use std::fmt::Debug; -use eyre::{Result, WrapErr}; use iroha_config_base::derive::{view, Documented, Error as ConfigError, Proxy}; use iroha_crypto::prelude::*; use serde::{Deserialize, Serialize}; @@ -88,48 +87,56 @@ impl ConfigurationProxy { /// # Errors /// - If the relevant uppermost Iroha config fields were not provided. #[allow(clippy::expect_used, clippy::unwrap_in_result)] - pub fn finish(&mut self) -> Result<()> { + pub fn finish(&mut self) -> Result<(), ConfigError> { if let Some(sumeragi_proxy) = &mut self.sumeragi { // First, iroha public/private key and sumeragi keypair are interchangeable, but // the user is allowed to provide only the former, and keypair is generated automatically, // bailing out if key_pair provided in sumeragi no matter its value if sumeragi_proxy.key_pair.is_some() { - eyre::bail!(ConfigError::ProxyBuildError( - "Sumeragi should not be provided with `key_pair` directly as it is instantiated via Iroha config. Please set the `KEY_PAIR` to `null` or omit them entirely." - .to_owned())) + return Err(ConfigError::ProvidedInferredField{ + field: "key_pair", + message: "Sumeragi should not be provided with `KEY_PAIR` directly. That value is computed from the other config parameters. Please set the `KEY_PAIR` to `null` or omit entirely." + }); } if let (Some(public_key), Some(private_key)) = (&self.public_key, &self.private_key) { sumeragi_proxy.key_pair = Some(KeyPair::new(public_key.clone(), private_key.clone())?); } else { - eyre::bail!(ConfigError::ProxyBuildError( - "Iroha public and private key not supplied, instantiating `sumeragi` keypair is impossible. Please provide `PRIVATE_KEY` and `PUBLIC_KEY` variables." - .to_owned() - )) + return Err(ConfigError::MissingField { + field: "PUBLIC_KEY and PRIVATE_KEY", + message: "The sumeragi keypair is not provided in the example configuration. It's done this way to ensure you don't re-use the example keys in production, and know how to generate new keys. Please have a look at \n\nhttps://hyperledger.github.io/iroha-2-docs/guide/configure/keys.html\n\nto learn more.\n\n-----", + }); } // Second, torii gateway and sumeragi peer id are interchangeable too; the latter is derived from the // former and overwritten silently in case of difference if let Some(torii_proxy) = &mut self.torii { if sumeragi_proxy.peer_id.is_none() { sumeragi_proxy.peer_id = Some(iroha_data_model::peer::Id::new( - &torii_proxy.p2p_addr.clone().ok_or_else(|| { - eyre::eyre!("Torii `p2p_addr` field has `None` value") - })?, + &torii_proxy + .p2p_addr + .clone() + .ok_or(ConfigError::MissingField { + field: "p2p_addr", + message: + "`p2p_addr` should not be set to `null` or `None` explicitly.", + })?, &self.public_key.clone().expect( "Iroha `public_key` should have been initialized above at the latest", ), )); } else { // TODO: should we just warn the user that this value will be ignored? - eyre::bail!(ConfigError::ProxyBuildError( - "Sumeragi should not be provided with `peer_id` directly. It is computed from the other provided values.".to_owned() - )) + // TODO: Consider eliminating this value from the public API. + return Err(ConfigError::ProvidedInferredField { + field: "PEER_ID", + message: "The `peer_id` is computed from the key and address. You should remove it from the config.", + }); } } else { - eyre::bail!(ConfigError::ProxyBuildError( - "Torii config should have at least `p2p_addr` provided for sumeragi finalisation" - .to_owned() - )) + return Err(ConfigError::MissingField{ + field: "p2p_addr", + message: "Torii config should have at least `p2p_addr` provided for sumeragi finalisation", + }); } // Finally, if trusted peers were not supplied, we can fall back to inserting itself as // the only trusted one @@ -151,10 +158,9 @@ impl ConfigurationProxy { /// - Finalisation fails /// - Building fails, e.g. any of the inner fields had a `None` value when that /// is not allowed by the defaults. - pub fn build(mut self) -> Result { + pub fn build(mut self) -> Result { self.finish()?; ::build(self) - .wrap_err("Failed to build `Configuration` from `ConfigurationProxy`") } } diff --git a/config/src/lib.rs b/config/src/lib.rs index 99b164bb7b4..73e0aaaf37f 100644 --- a/config/src/lib.rs +++ b/config/src/lib.rs @@ -1,5 +1,5 @@ //! Aggregate configuration for different Iroha modules. - +#![feature(absolute_path)] pub use iroha_config_base as base; use serde::{Deserialize, Serialize}; diff --git a/config/src/path.rs b/config/src/path.rs index 0963b260de4..68e6743cf5c 100644 --- a/config/src/path.rs +++ b/config/src/path.rs @@ -15,13 +15,13 @@ pub const ALLOWED_CONFIG_EXTENSIONS: [&str; 2] = ["json", "json5"]; pub enum ExtensionError { /// User provided config file without extension. #[error( - "Provided config file has no extension, allowed extensions are: {:?}.", + "No valid file extension found. Allowed file extensions are: {:?}.", ALLOWED_CONFIG_EXTENSIONS )] Missing, /// User provided config file with unsupported extension. #[error( - "Provided config file has invalid extension `{0}`, \ + "Provided config file has an unsupported file extension `{0}`, \ allowed extensions are: {:?}.", ALLOWED_CONFIG_EXTENSIONS )] @@ -40,7 +40,7 @@ enum InnerPath { UserProvided(PathBuf), } -/// Wrapper around path to config file (i.e. config.json, genesis.json). +/// Wrapper around path to config file (e.g. `config.json`). /// /// Provides abstraction above user-provided config and default ones. #[derive(Debug, Clone)] @@ -49,8 +49,16 @@ pub struct Path(InnerPath); impl core::fmt::Display for Path { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.0 { - Default(pth) => write!(f, "{pth:?} (default)"), - UserProvided(pth) => write!(f, "{pth:?}"), + Default(pth) => write!( + f, + "{:?} (default)", + std::path::absolute(pth.with_extension("json")).expect("Malformed path provided") + ), + UserProvided(pth) => write!( + f, + "{:?} (user-provided)", + std::path::absolute(pth.with_extension("json")).expect("Malformed path") + ), } } } diff --git a/core/src/smartcontracts/wasm.rs b/core/src/smartcontracts/wasm.rs index 96a42be2c2e..b7db537eb0e 100644 --- a/core/src/smartcontracts/wasm.rs +++ b/core/src/smartcontracts/wasm.rs @@ -438,7 +438,7 @@ impl<'wrld> Runtime<'wrld> { ) -> Result<(), Trap> { const TARGET: &str = "WASM"; - let error_msg = || Trap::new(format!("{}: not a valid log level", log_level)); + let error_msg = || Trap::new(format!("{log_level}: not a valid log level")); let Ok(log_level) = log_level.try_into() else { return Err(error_msg()); }; diff --git a/crypto/src/lib.rs b/crypto/src/lib.rs index 6178254d4b2..62e70610947 100755 --- a/crypto/src/lib.rs +++ b/crypto/src/lib.rs @@ -180,7 +180,7 @@ ffi::ffi_item! { } /// Error when dealing with cryptographic functions -#[derive(Debug, Display)] +#[derive(Debug, Display, Deserialize)] pub enum Error { /// Returned when trying to create an algorithm which does not exist #[display(fmt = "Algorithm doesn't exist")] // TODO: which algorithm diff --git a/data_model/derive/src/api.rs b/data_model/derive/src/api.rs index bab205cb8c4..57c11b84ace 100644 --- a/data_model/derive/src/api.rs +++ b/data_model/derive/src/api.rs @@ -129,6 +129,7 @@ fn process_pub_item(input: syn::DeriveInput) -> TokenStream { } }); + #[allow(clippy::arithmetic_side_effects)] let item = quote! { pub union #ident #impl_generics #where_clause { #(#fields),* diff --git a/logger/src/lib.rs b/logger/src/lib.rs index fad0bf80c12..01070ff3087 100644 --- a/logger/src/lib.rs +++ b/logger/src/lib.rs @@ -283,6 +283,7 @@ pub fn install_panic_hook() -> Result<(), Report> { .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) .is_ok() { + println!("Installing eyre hook"); color_eyre::install() } else { Ok(())