From 0b6d5b3929c3f578edf8d9e9efb38776efd30eb3 Mon Sep 17 00:00:00 2001 From: Christian Gorenflo Date: Fri, 1 Sep 2023 15:40:59 -0400 Subject: [PATCH] refactor: clean up error definitions --- ampd/src/lib.rs | 23 ++++++++------ ampd/src/report.rs | 18 ++++------- ampd/src/tofnd/error.rs | 2 +- ampd/tests/report.rs | 21 ++++++++----- contracts/aggregate-verifier/src/error.rs | 4 +-- contracts/batching/src/error.rs | 2 +- contracts/connection-router/src/error.rs | 18 +++++------ contracts/connection-router/tests/test.rs | 38 +++++++++++------------ contracts/gateway/src/error.rs | 8 ++--- contracts/gateway/tests/tests.rs | 4 +-- contracts/multisig-prover/src/error.rs | 2 +- contracts/multisig/src/error.rs | 2 +- contracts/service-registry/src/error.rs | 16 +++++----- contracts/service-registry/src/state.rs | 2 +- contracts/service-registry/tests/tests.rs | 6 ++-- contracts/voting-verifier/src/error.rs | 10 +++--- contracts/voting-verifier/tests/tests.rs | 2 +- 17 files changed, 91 insertions(+), 87 deletions(-) diff --git a/ampd/src/lib.rs b/ampd/src/lib.rs index f8cb6e9e6..cccd82099 100644 --- a/ampd/src/lib.rs +++ b/ampd/src/lib.rs @@ -4,7 +4,7 @@ use std::pin::Pin; use cosmos_sdk_proto::cosmos::{ auth::v1beta1::query_client::QueryClient, tx::v1beta1::service_client::ServiceClient, }; -use error_stack::{FutureExt, Report, Result, ResultExt}; +use error_stack::{FutureExt, Result, ResultExt}; use tokio::signal::unix::{signal, SignalKind}; use tokio::task::JoinSet; use tokio_stream::Stream; @@ -52,20 +52,20 @@ pub async fn run(cfg: Config, state_path: PathBuf) -> Result<(), Error> { multisig, } = cfg; - let tm_client = - tendermint_rpc::HttpClient::new(tm_jsonrpc.to_string().as_str()).map_err(Error::new)?; + let tm_client = tendermint_rpc::HttpClient::new(tm_jsonrpc.to_string().as_str()) + .change_context(Error::Connection)?; let service_client = ServiceClient::connect(tm_grpc.to_string()) .await - .map_err(Error::new)?; + .change_context(Error::Connection)?; let query_client = QueryClient::connect(tm_grpc.to_string()) .await - .map_err(Error::new)?; + .change_context(Error::Connection)?; let multisig_client = MultisigClient::connect(tofnd_config.party_uid, tofnd_config.url) .await - .map_err(Error::new)?; + .change_context(Error::Tofnd)?; let ecdsa_client = SharableEcdsaClient::new(multisig_client); - let mut state_updater = StateUpdater::new(state_path).map_err(Error::new)?; + let mut state_updater = StateUpdater::new(state_path).change_context(Error::StateUpdater)?; let pub_key = match state_updater.state().pub_key { Some(pub_key) => pub_key, None => { @@ -83,7 +83,9 @@ pub async fn run(cfg: Config, state_path: PathBuf) -> Result<(), Error> { .account_id(PREFIX) .expect("failed to convert to account identifier") .into(); - let account = account(query_client, &worker).await.map_err(Error::new)?; + let account = account(query_client, &worker) + .await + .change_context(Error::Broadcaster)?; let broadcaster = broadcaster::BroadcastClientBuilder::default() .client(service_client) @@ -176,7 +178,8 @@ where worker.clone(), config.voting_verifier, config.name, - evm::json_rpc::Client::new_http(&config.rpc_url).map_err(Error::new)?, + evm::json_rpc::Client::new_http(&config.rpc_url) + .change_context(Error::Connection)?, self.broadcaster.client(), ); @@ -258,7 +261,7 @@ where let res = match (set.join_next().await, token.is_cancelled()) { (Some(result), false) => { token.cancel(); - result.map_err(Error::new).map_err(Report::from)? + result.change_context(Error::Threading)? } (Some(_), true) => Ok(()), (None, _) => panic!("all tasks exited unexpectedly"), diff --git a/ampd/src/report.rs b/ampd/src/report.rs index 00c524c7d..8fa2a2bf3 100644 --- a/ampd/src/report.rs +++ b/ampd/src/report.rs @@ -1,10 +1,11 @@ +use std::backtrace::Backtrace; use std::collections::VecDeque; use std::iter::FromIterator; use std::panic::Location; -use std::{backtrace::Backtrace, fmt::Display}; use error_stack::{AttachmentKind, Context, Frame, FrameKind, Report}; use thiserror::Error; +use tracing::error; use valuable::Valuable; #[derive(Error, Debug)] @@ -19,17 +20,10 @@ pub enum Error { StateUpdater, #[error("tofnd failed")] Tofnd, - #[error("{0}")] - Error(String), -} - -impl Error { - pub fn new(msg: T) -> Error - where - T: Display, - { - Error::Error(msg.to_string()) - } + #[error("connection failed")] + Connection, + #[error("thread execution failed")] + Threading, } #[derive(Valuable, PartialEq, Debug, Default)] diff --git a/ampd/src/tofnd/error.rs b/ampd/src/tofnd/error.rs index 54f8c7911..705d3926d 100644 --- a/ampd/src/tofnd/error.rs +++ b/ampd/src/tofnd/error.rs @@ -8,7 +8,7 @@ pub enum Error { KeygenFailed, #[error("sign failed")] SignFailed, - #[error("{0}")] + #[error(transparent)] FromHex(#[from] hex::FromHexError), #[error("parsing failed")] ParsingFailed, diff --git a/ampd/tests/report.rs b/ampd/tests/report.rs index dd2e22dfc..8899001ac 100644 --- a/ampd/tests/report.rs +++ b/ampd/tests/report.rs @@ -1,16 +1,23 @@ use error_stack::Report; +use thiserror::Error; -use ampd::report::{Error, LoggableError}; +use ampd::report::LoggableError; + +#[derive(Error, Debug)] +enum Error { + #[error("{0}")] + FromString(String), +} // Do not move this test or the location field checks break #[test] fn correct_error_log() { - let report = Report::new(Error::new("error1".to_string())) + let report = Report::new(Error::FromString("error1".to_string())) .attach_printable("foo1") - .change_context(Error::new("error2".to_string())) + .change_context(Error::FromString("error2".to_string())) .attach_printable("test1") .attach_printable("test2") - .change_context(Error::new("error3".to_string())) + .change_context(Error::FromString("error3".to_string())) .attach(5); let mut err = LoggableError::from(&report); @@ -25,15 +32,15 @@ fn correct_error_log() { let expected_err = LoggableError { msg: "error3".to_string(), attachments: vec!["opaque attachment".to_string()], - location: "ampd/tests/report.rs:13:10".to_string(), + location: "ampd/tests/report.rs:20:10".to_string(), cause: Some(Box::new(LoggableError { msg: "error2".to_string(), attachments: vec!["test1".to_string(), "test2".to_string()], - location: "ampd/tests/report.rs:10:10".to_string(), + location: "ampd/tests/report.rs:17:10".to_string(), cause: Some(Box::new(LoggableError { msg: "error1".to_string(), attachments: vec!["foo1".to_string()], - location: "ampd/tests/report.rs:8:18".to_string(), + location: "ampd/tests/report.rs:15:18".to_string(), cause: None, backtrace: None, })), diff --git a/contracts/aggregate-verifier/src/error.rs b/contracts/aggregate-verifier/src/error.rs index c3370f2ce..06e4a804d 100644 --- a/contracts/aggregate-verifier/src/error.rs +++ b/contracts/aggregate-verifier/src/error.rs @@ -3,10 +3,10 @@ use thiserror::Error; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), - #[error("{0}")] + #[error(transparent)] RouterError(#[from] connection_router::ContractError), #[error("received invalid verifier reply: {0}")] diff --git a/contracts/batching/src/error.rs b/contracts/batching/src/error.rs index 40dab9ff3..6f4e9e04e 100644 --- a/contracts/batching/src/error.rs +++ b/contracts/batching/src/error.rs @@ -3,6 +3,6 @@ use thiserror::Error; #[derive(Error, Debug)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), } diff --git a/contracts/connection-router/src/error.rs b/contracts/connection-router/src/error.rs index 4164b1655..abbe41e2b 100644 --- a/contracts/connection-router/src/error.rs +++ b/contracts/connection-router/src/error.rs @@ -5,29 +5,29 @@ use crate::types::ChainName; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), #[error("caller is not authorized")] - Unauthorized {}, + Unauthorized, #[error("chain already exists")] - ChainAlreadyExists {}, + ChainAlreadyExists, #[error("chain name is invalid")] - InvalidChainName {}, + InvalidChainName, #[error("message ID is invalid")] - InvalidMessageID {}, + InvalidMessageID, #[error("chain is not found")] - ChainNotFound {}, + ChainNotFound, #[error("gateway is not registered")] - GatewayNotRegistered {}, + GatewayNotRegistered, #[error("gateway is already registered")] - GatewayAlreadyRegistered {}, + GatewayAlreadyRegistered, #[error("chain is frozen")] ChainFrozen { chain: ChainName }, @@ -36,7 +36,7 @@ pub enum ContractError { InvalidAddress(String), #[error("source chain does not match registered gateway")] - WrongSourceChain {}, + WrongSourceChain, } impl From for StdError { diff --git a/contracts/connection-router/tests/test.rs b/contracts/connection-router/tests/test.rs index d042a0a8b..54dfb7906 100644 --- a/contracts/connection-router/tests/test.rs +++ b/contracts/connection-router/tests/test.rs @@ -143,7 +143,7 @@ fn route_non_existing_chain() { &[], ) .unwrap_err(); - assert_eq!(ContractError::ChainNotFound {}, res.downcast().unwrap()); + assert_eq!(ContractError::ChainNotFound , res.downcast().unwrap()); } #[test] @@ -188,7 +188,7 @@ fn message_id() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidMessageID {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidMessageID , res.downcast().unwrap()); // don't prepend source chain let bad_id = msg.id.split(&msg.source_chain).collect::>()[0]; @@ -204,7 +204,7 @@ fn message_id() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidMessageID {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidMessageID , res.downcast().unwrap()); let res = config .app @@ -218,7 +218,7 @@ fn message_id() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidMessageID {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidMessageID , res.downcast().unwrap()); let res = config .app @@ -232,7 +232,7 @@ fn message_id() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidMessageID {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidMessageID , res.downcast().unwrap()); } #[test] @@ -301,7 +301,7 @@ fn wrong_source_chain() { &[], ) .unwrap_err(); - assert_eq!(ContractError::WrongSourceChain {}, res.downcast().unwrap()); + assert_eq!(ContractError::WrongSourceChain , res.downcast().unwrap()); } #[test] @@ -379,7 +379,7 @@ fn authorization() { ) .unwrap_err(); - assert_eq!(ContractError::Unauthorized {}, res.downcast().unwrap()); + assert_eq!(ContractError::Unauthorized , res.downcast().unwrap()); let res = config.app.execute_contract( config.admin_address.clone(), @@ -405,7 +405,7 @@ fn authorization() { ) .unwrap_err(); - assert_eq!(ContractError::Unauthorized {}, res.downcast().unwrap()); + assert_eq!(ContractError::Unauthorized , res.downcast().unwrap()); let res = config.app.execute_contract( config.admin_address.clone(), @@ -431,7 +431,7 @@ fn authorization() { ) .unwrap_err(); - assert_eq!(ContractError::Unauthorized {}, res.downcast().unwrap()); + assert_eq!(ContractError::Unauthorized , res.downcast().unwrap()); let res = config .app @@ -446,7 +446,7 @@ fn authorization() { ) .unwrap_err(); - assert_eq!(ContractError::Unauthorized {}, res.downcast().unwrap()); + assert_eq!(ContractError::Unauthorized, res.downcast().unwrap()); let res = config.app.execute_contract( config.admin_address.clone(), @@ -536,7 +536,7 @@ fn upgrade_gateway_incoming() { ) .unwrap_err(); assert_eq!( - ContractError::GatewayNotRegistered {}, + ContractError::GatewayNotRegistered , res.downcast().unwrap() ); @@ -569,7 +569,7 @@ fn register_chain_test() { ) .unwrap_err(); assert_eq!( - ContractError::GatewayNotRegistered {}, + ContractError::GatewayNotRegistered , res.downcast().unwrap() ); @@ -583,7 +583,7 @@ fn register_chain_test() { &[], ) .unwrap_err(); - assert_eq!(ContractError::ChainNotFound {}, res.downcast().unwrap()); + assert_eq!(ContractError::ChainNotFound , res.downcast().unwrap()); register_chain(&mut config, &polygon); let res = config.app.execute_contract( @@ -614,7 +614,7 @@ fn chain_already_registered() { ) .unwrap_err(); assert_eq!( - ContractError::ChainAlreadyExists {}, + ContractError::ChainAlreadyExists , res.downcast().unwrap() ); @@ -632,7 +632,7 @@ fn chain_already_registered() { ) .unwrap_err(); assert_eq!( - ContractError::ChainAlreadyExists {}, + ContractError::ChainAlreadyExists , res.downcast().unwrap() ); } @@ -652,7 +652,7 @@ fn invalid_chain_name() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidChainName {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidChainName , res.downcast().unwrap()); let res = config .app @@ -666,7 +666,7 @@ fn invalid_chain_name() { &[], ) .unwrap_err(); - assert_eq!(ContractError::InvalidChainName {}, res.downcast().unwrap()); + assert_eq!(ContractError::InvalidChainName , res.downcast().unwrap()); } #[test] @@ -688,7 +688,7 @@ fn gateway_already_registered() { ) .unwrap_err(); assert_eq!( - ContractError::GatewayAlreadyRegistered {}, + ContractError::GatewayAlreadyRegistered , res.downcast().unwrap() ); @@ -706,7 +706,7 @@ fn gateway_already_registered() { ) .unwrap_err(); assert_eq!( - ContractError::GatewayAlreadyRegistered {}, + ContractError::GatewayAlreadyRegistered , res.downcast().unwrap() ); } diff --git a/contracts/gateway/src/error.rs b/contracts/gateway/src/error.rs index 85e39a5ca..9af76ddc1 100644 --- a/contracts/gateway/src/error.rs +++ b/contracts/gateway/src/error.rs @@ -3,15 +3,15 @@ use thiserror::Error; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), - #[error("{0}")] + #[error(transparent)] RouterError(#[from] connection_router::ContractError), #[error("sender is not router")] - SenderNotRouter {}, + SenderNotRouter, #[error("batch contains duplicate message ids")] - DuplicateMessageID {}, + DuplicateMessageID, } diff --git a/contracts/gateway/tests/tests.rs b/contracts/gateway/tests/tests.rs index 5bc46a750..264632e36 100644 --- a/contracts/gateway/tests/tests.rs +++ b/contracts/gateway/tests/tests.rs @@ -909,7 +909,7 @@ fn duplicate_message_id() { ) .unwrap_err(); assert_eq!( - ContractError::DuplicateMessageID {}, + ContractError::DuplicateMessageID , err.downcast().unwrap() ); @@ -922,7 +922,7 @@ fn duplicate_message_id() { ) .unwrap_err(); assert_eq!( - ContractError::DuplicateMessageID {}, + ContractError::DuplicateMessageID , err.downcast().unwrap() ); diff --git a/contracts/multisig-prover/src/error.rs b/contracts/multisig-prover/src/error.rs index c53a5961d..40e31ca55 100644 --- a/contracts/multisig-prover/src/error.rs +++ b/contracts/multisig-prover/src/error.rs @@ -3,7 +3,7 @@ use thiserror::Error; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), #[error("caller is not authorized")] diff --git a/contracts/multisig/src/error.rs b/contracts/multisig/src/error.rs index d3cf35d8b..8b9a439bd 100644 --- a/contracts/multisig/src/error.rs +++ b/contracts/multisig/src/error.rs @@ -3,7 +3,7 @@ use thiserror::Error; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), #[error("No active key found for {key_id:?}")] diff --git a/contracts/service-registry/src/error.rs b/contracts/service-registry/src/error.rs index 6c5c60348..fcde8f979 100644 --- a/contracts/service-registry/src/error.rs +++ b/contracts/service-registry/src/error.rs @@ -6,24 +6,24 @@ use crate::state::BondingState; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), - #[error("{0}")] + #[error(transparent)] NonEmpty(#[from] nonempty::Error), #[error("unauthorized")] - Unauthorized {}, + Unauthorized, #[error("service name already exists")] - ServiceAlreadyExists {}, + ServiceAlreadyExists, #[error("service not found")] - ServiceNotFound {}, + ServiceNotFound, #[error("worker already authorized")] - WorkerAlreadyAuthorized {}, + WorkerAlreadyAuthorized, #[error("funds are in the wrong denomination")] - WrongDenom {}, + WrongDenom, #[error("worker not found")] - WorkerNotFound {}, + WorkerNotFound, #[error("invalid bonding state `{0:?}` for this operation")] InvalidBondingState(BondingState), } diff --git a/contracts/service-registry/src/state.rs b/contracts/service-registry/src/state.rs index c48841dda..dc731aa2e 100644 --- a/contracts/service-registry/src/state.rs +++ b/contracts/service-registry/src/state.rs @@ -358,7 +358,7 @@ mod tests { assert!(res.is_ok()); assert_eq!( res.unwrap(), - (BondingState::Unbonded {}, Uint128::from(100u32)) + (BondingState::Unbonded, Uint128::from(100u32)) ); } diff --git a/contracts/service-registry/tests/tests.rs b/contracts/service-registry/tests/tests.rs index 83f0442bb..1549267b7 100644 --- a/contracts/service-registry/tests/tests.rs +++ b/contracts/service-registry/tests/tests.rs @@ -63,7 +63,7 @@ fn register_service() { ); assert!(!res.is_ok()); assert_eq!( - ContractError::Unauthorized {}, + ContractError::Unauthorized, res.unwrap_err().downcast().unwrap() ); } @@ -125,7 +125,7 @@ fn authorize_worker() { &[], ); assert_eq!( - ContractError::Unauthorized {}, + ContractError::Unauthorized, res.unwrap_err().downcast().unwrap() ); } @@ -460,7 +460,7 @@ fn bond_wrong_denom() { ); assert!(res.is_err()); assert_eq!( - ContractError::WrongDenom {}, + ContractError::WrongDenom, res.unwrap_err().downcast().unwrap() ); } diff --git a/contracts/voting-verifier/src/error.rs b/contracts/voting-verifier/src/error.rs index 7e75d5230..7eac941bd 100644 --- a/contracts/voting-verifier/src/error.rs +++ b/contracts/voting-verifier/src/error.rs @@ -8,16 +8,16 @@ use service_registry; #[derive(Error, Debug, PartialEq)] pub enum ContractError { - #[error("{0}")] + #[error(transparent)] Std(#[from] StdError), - #[error("{0}")] + #[error(transparent)] RouterError(#[from] connection_router::ContractError), - #[error("{0}")] + #[error(transparent)] NonEmptyError(#[from] nonempty::Error), - #[error("{0}")] + #[error(transparent)] ServiceRegistryError(#[from] service_registry::ContractError), #[error("empty batch of messages")] @@ -35,7 +35,7 @@ pub enum ContractError { #[error("poll not found")] PollNotFound, - #[error("{0}")] + #[error(transparent)] VoteError(#[from] voting::Error), #[error("worker set already confirmed")] diff --git a/contracts/voting-verifier/tests/tests.rs b/contracts/voting-verifier/tests/tests.rs index e7de405bb..097195baa 100644 --- a/contracts/voting-verifier/tests/tests.rs +++ b/contracts/voting-verifier/tests/tests.rs @@ -505,7 +505,7 @@ fn should_not_confirm_twice() { let res = app.execute_contract(Addr::unchecked(SENDER), contract_address.clone(), &msg, &[]); assert!(res.is_err()); assert_eq!( - ContractError::WorkerSetAlreadyConfirmed {}, + ContractError::WorkerSetAlreadyConfirmed, res.unwrap_err().downcast().unwrap() ); }