From ffc96c863a385bc0b36e957ea288cdf8c64faba7 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 13:14:36 +0530 Subject: [PATCH 1/8] GH-672: add more errors --- .../db_access_objects/failed_payable_dao.rs | 118 +++++++++++++----- 1 file changed, 86 insertions(+), 32 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index 78554557d..f541d0d35 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -7,9 +7,11 @@ use crate::accountant::{checked_conversion, comma_joined_stringifiable}; use crate::database::rusqlite_wrappers::ConnectionWrapper; use itertools::Itertools; use masq_lib::utils::ExpectValue; +use serde_derive::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; use std::fmt::{Display, Formatter}; use std::str::FromStr; +use web3::error::Error as Web3Error; use web3::types::Address; #[derive(Debug, PartialEq, Eq)] @@ -21,11 +23,72 @@ pub enum FailedPayableDaoError { SqlExecutionFailed(String), } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum FailureReason { + Local(LocalError), + Api(ApiError), // It includes the RPC + Pool(PoolError), PendingTooLong, - NonceIssue, - General, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum LocalError { + InsufficientFunds, + InvalidNonce, + SigningError, + SerializationError, + GasPriceTooLow, + ExceedsGasLimit, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum ApiError { + ConnectionFailed, + Timeout, + InvalidResponse(String), + UnsupportedMethod, + LimitExceeded, + Web3RpcError { code: i64, message: String }, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum PoolError { + Underpriced, + OverCapacity, + AlreadyKnown, + Replaced, + Dropped, +} + +impl From for FailureReason { + fn from(error: Web3Error) -> Self { + match error { + Web3Error::Rpc(web3_rpc_error) => FailureReason::Api(ApiError::Web3RpcError { + code: web3_rpc_error.code.code(), + message: web3_rpc_error.message, + }), + Web3Error::Io(_) => FailureReason::Api(ApiError::ConnectionFailed), + Web3Error::Decoder(_) => FailureReason::Local(LocalError::SerializationError), + _ => FailureReason::Api(ApiError::InvalidResponse(error.to_string())), + } + } +} + +impl Display for FailureReason { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match serde_json::to_string(self) { + Ok(json) => write!(f, "{}", json), + Err(_) => write!(f, ""), + } + } +} + +impl FromStr for FailureReason { + type Err = serde_json::Error; + + fn from_str(s: &str) -> Result { + serde_json::from_str(s) + } } #[derive(Clone, Debug, PartialEq, Eq)] @@ -47,19 +110,6 @@ impl FromStr for FailureStatus { } } -impl FromStr for FailureReason { - type Err = String; - - fn from_str(s: &str) -> Result { - match s { - "PendingTooLong" => Ok(FailureReason::PendingTooLong), - "NonceIssue" => Ok(FailureReason::NonceIssue), - "General" => Ok(FailureReason::General), - _ => Err(format!("Invalid FailureReason: {}", s)), - } - } -} - #[derive(Clone, Debug, PartialEq, Eq)] pub struct FailedTx { pub hash: TxHash, @@ -349,15 +399,16 @@ impl FailedPayableDaoFactory for DaoFactoryReal { #[cfg(test)] mod tests { + use crate::accountant::db_access_objects::failed_payable_dao::ApiError::LimitExceeded; use crate::accountant::db_access_objects::failed_payable_dao::FailureReason::{ - General, NonceIssue, PendingTooLong, + Api, PendingTooLong, }; use crate::accountant::db_access_objects::failed_payable_dao::FailureStatus::{ Concluded, RecheckRequired, RetryRequired, }; use crate::accountant::db_access_objects::failed_payable_dao::{ - FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureReason, - FailureRetrieveCondition, FailureStatus, + FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureRetrieveCondition, + FailureStatus, }; use crate::accountant::db_access_objects::test_utils::{ make_read_only_db_connection, FailedTxBuilder, @@ -382,7 +433,7 @@ mod tests { .unwrap(); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(NonceIssue) + .reason(Api(LimitExceeded)) .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) @@ -563,16 +614,19 @@ mod tests { #[test] fn failure_reason_from_str_works() { - assert_eq!( - FailureReason::from_str("PendingTooLong"), - Ok(PendingTooLong) - ); - assert_eq!(FailureReason::from_str("NonceIssue"), Ok(NonceIssue)); - assert_eq!(FailureReason::from_str("General"), Ok(General)); - assert_eq!( - FailureReason::from_str("InvalidReason"), - Err("Invalid FailureReason: InvalidReason".to_string()) - ); + todo!("test me"); + // assert_eq!( + // FailureReason::from_str("PendingTooLong"), + // Ok(PendingTooLong) + // ); + // let expected = FailureReason::from_str("NonceIssue"); + + // assert_eq!(FailureReason::from_str("NonceIssue"), Ok(NonceIssue)); + // assert_eq!(FailureReason::from_str("General"), Ok(General)); + // assert_eq!( + // FailureReason::from_str("InvalidReason"), + // Err("Invalid FailureReason: InvalidReason".to_string()) + // ); } #[test] @@ -640,7 +694,7 @@ mod tests { .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) - .reason(NonceIssue) + .reason(Api(LimitExceeded)) .timestamp(now - 3600) .status(RetryRequired) .build(); @@ -674,7 +728,7 @@ mod tests { let subject = FailedPayableDaoReal::new(wrapped_conn); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(NonceIssue) + .reason(Api(LimitExceeded)) .status(RetryRequired) .build(); let tx2 = FailedTxBuilder::default() From 6b9cddc95f9df78a07ae1084c2f635d97432f7eb Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 14:06:20 +0530 Subject: [PATCH 2/8] GH-672: improve errors; compiling --- .../db_access_objects/failed_payable_dao.rs | 51 ++++++++++--------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index f541d0d35..927f03ea5 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -33,43 +33,48 @@ pub enum FailureReason { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum LocalError { - InsufficientFunds, - InvalidNonce, - SigningError, - SerializationError, - GasPriceTooLow, - ExceedsGasLimit, + Decoder(String), + Internal, + Io(String), + Signing(String), + Transport(String), } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum ApiError { - ConnectionFailed, - Timeout, - InvalidResponse(String), - UnsupportedMethod, - LimitExceeded, + Unreachable, Web3RpcError { code: i64, message: String }, + InvalidResponse(String), } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum PoolError { - Underpriced, - OverCapacity, - AlreadyKnown, - Replaced, - Dropped, + NonceTooHigh, // GH-672: We can also store the used vs expected nonce + NonceTooLow, + OrphanedTransaction, // GH-672: We can also store the successful transaction and block number } impl From for FailureReason { fn from(error: Web3Error) -> Self { match error { + // Local Errors + Web3Error::Decoder(error) => FailureReason::Local(LocalError::Decoder(error)), + Web3Error::Transport(error) => FailureReason::Local(LocalError::Transport(error)), + + Web3Error::Io(error) => FailureReason::Local(LocalError::Io(error.to_string())), + Web3Error::Signing(error) => { + FailureReason::Local(LocalError::Signing(error.to_string())) + } + Web3Error::Internal => FailureReason::Local(LocalError::Internal), + // Api Errors + Web3Error::Unreachable => FailureReason::Api(ApiError::Unreachable), + Web3Error::InvalidResponse(response) => { + FailureReason::Api(ApiError::InvalidResponse(response)) + } Web3Error::Rpc(web3_rpc_error) => FailureReason::Api(ApiError::Web3RpcError { code: web3_rpc_error.code.code(), message: web3_rpc_error.message, }), - Web3Error::Io(_) => FailureReason::Api(ApiError::ConnectionFailed), - Web3Error::Decoder(_) => FailureReason::Local(LocalError::SerializationError), - _ => FailureReason::Api(ApiError::InvalidResponse(error.to_string())), } } } @@ -399,7 +404,7 @@ impl FailedPayableDaoFactory for DaoFactoryReal { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::failed_payable_dao::ApiError::LimitExceeded; + use crate::accountant::db_access_objects::failed_payable_dao::ApiError::Unreachable; use crate::accountant::db_access_objects::failed_payable_dao::FailureReason::{ Api, PendingTooLong, }; @@ -433,7 +438,7 @@ mod tests { .unwrap(); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(Api(LimitExceeded)) + .reason(Api(Unreachable)) .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) @@ -694,7 +699,7 @@ mod tests { .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) - .reason(Api(LimitExceeded)) + .reason(Api(Unreachable)) .timestamp(now - 3600) .status(RetryRequired) .build(); @@ -728,7 +733,7 @@ mod tests { let subject = FailedPayableDaoReal::new(wrapped_conn); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(Api(LimitExceeded)) + .reason(Api(Unreachable)) .status(RetryRequired) .build(); let tx2 = FailedTxBuilder::default() From 41d598b06818de847b889f63050339213429b020 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 14:39:47 +0530 Subject: [PATCH 3/8] GH-672: introduce the conversion of FailureReason --- .../db_access_objects/failed_payable_dao.rs | 102 +++++++++++++++--- 1 file changed, 86 insertions(+), 16 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index 927f03ea5..c281be4da 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -42,9 +42,9 @@ pub enum LocalError { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum ApiError { + InvalidResponse(String), Unreachable, Web3RpcError { code: i64, message: String }, - InvalidResponse(String), } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -412,8 +412,8 @@ mod tests { Concluded, RecheckRequired, RetryRequired, }; use crate::accountant::db_access_objects::failed_payable_dao::{ - FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureRetrieveCondition, - FailureStatus, + ApiError, FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureReason, + FailureRetrieveCondition, FailureStatus, LocalError, PoolError, }; use crate::accountant::db_access_objects::test_utils::{ make_read_only_db_connection, FailedTxBuilder, @@ -619,19 +619,89 @@ mod tests { #[test] fn failure_reason_from_str_works() { - todo!("test me"); - // assert_eq!( - // FailureReason::from_str("PendingTooLong"), - // Ok(PendingTooLong) - // ); - // let expected = FailureReason::from_str("NonceIssue"); - - // assert_eq!(FailureReason::from_str("NonceIssue"), Ok(NonceIssue)); - // assert_eq!(FailureReason::from_str("General"), Ok(General)); - // assert_eq!( - // FailureReason::from_str("InvalidReason"), - // Err("Invalid FailureReason: InvalidReason".to_string()) - // ); + // Local errors (lexicographically sorted) + assert_eq!( + FailureReason::from_str(r#"{"Local":{"Decoder":"Test decoder error"}}"#).unwrap(), + FailureReason::Local(LocalError::Decoder("Test decoder error".to_string())) + ); + assert_eq!( + FailureReason::from_str(r#"{"Local":{"Internal":null}}"#).unwrap(), + FailureReason::Local(LocalError::Internal) + ); + assert_eq!( + FailureReason::from_str(r#"{"Local":{"Io":"Test IO error"}}"#).unwrap(), + FailureReason::Local(LocalError::Io("Test IO error".to_string())) + ); + assert_eq!( + FailureReason::from_str(r#"{"Local":{"Signing":"Test signing error"}}"#).unwrap(), + FailureReason::Local(LocalError::Signing("Test signing error".to_string())) + ); + assert_eq!( + FailureReason::from_str(r#"{"Local":{"Transport":"Test transport error"}}"#).unwrap(), + FailureReason::Local(LocalError::Transport("Test transport error".to_string())) + ); + + // Api errors (lexicographically sorted) + assert_eq!( + FailureReason::from_str(r#"{"Api":{"InvalidResponse":"Test invalid response"}}"#) + .unwrap(), + FailureReason::Api(ApiError::InvalidResponse( + "Test invalid response".to_string() + )) + ); + assert_eq!( + FailureReason::from_str(r#"{"Api":{"Unreachable":null}}"#).unwrap(), + FailureReason::Api(ApiError::Unreachable) + ); + assert_eq!( + FailureReason::from_str( + r#"{"Api":{"Web3RpcError":{"code":123,"message":"Test RPC error"}}}"# + ) + .unwrap(), + FailureReason::Api(ApiError::Web3RpcError { + code: 123, + message: "Test RPC error".to_string() + }) + ); + + // Pool errors (lexicographically sorted) + assert_eq!( + FailureReason::from_str(r#"{"Pool":{"NonceTooHigh":null}}"#).unwrap(), + FailureReason::Pool(PoolError::NonceTooHigh) + ); + assert_eq!( + FailureReason::from_str(r#"{"Pool":{"NonceTooLow":null}}"#).unwrap(), + FailureReason::Pool(PoolError::NonceTooLow) + ); + assert_eq!( + FailureReason::from_str(r#"{"Pool":{"OrphanedTransaction":null}}"#).unwrap(), + FailureReason::Pool(PoolError::OrphanedTransaction) + ); + + // PendingTooLong + assert_eq!( + FailureReason::from_str(r#"{"PendingTooLong":null}"#).unwrap(), + FailureReason::PendingTooLong + ); + + // Invalid Variant + assert_eq!( + FailureReason::from_str(r#"{"UnknownReason":null}"#) + .unwrap_err() + .to_string(), + "unknown variant `UnknownReason`, \ + expected one of `Local`, `Api`, `Pool`, `PendingTooLong` \ + at line 1 column 16" + .to_string() + ); + + // Invalid Input + assert_eq!( + FailureReason::from_str("random string") + .unwrap_err() + .to_string(), + "expected value at line 1 column 1".to_string() + ); } #[test] From 43249977f0ae5a58cf4c924d41e75fd38a0d0833 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 14:44:04 +0530 Subject: [PATCH 4/8] GH-672: all tests are passing :) --- node/src/accountant/db_access_objects/failed_payable_dao.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index c281be4da..719f3c462 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -229,7 +229,7 @@ impl FailedPayableDao for FailedPayableDaoReal<'_> { let (gas_price_wei_high_b, gas_price_wei_low_b) = BigIntDivider::deconstruct(gas_price_wei_checked); format!( - "('{:?}', '{:?}', {}, {}, {}, {}, {}, {}, '{:?}', '{:?}')", + "('{:?}', '{:?}', {}, {}, {}, {}, {}, {}, '{}', '{:?}')", tx.hash, tx.receiver_address, amount_high_b, From c13e2a3d52a6f2745d0279e5d7e54540a57f3ea3 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 16:59:04 +0530 Subject: [PATCH 5/8] GH-672: From conversion is properly tested --- .../db_access_objects/failed_payable_dao.rs | 91 +++++++++++++++---- 1 file changed, 75 insertions(+), 16 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index 719f3c462..cd6b9ab23 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -49,9 +49,14 @@ pub enum ApiError { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum PoolError { - NonceTooHigh, // GH-672: We can also store the used vs expected nonce - NonceTooLow, - OrphanedTransaction, // GH-672: We can also store the successful transaction and block number + NonceIssue { + latest_used_nonce: u64, + tx_nonce: u64, + }, + OrphanedTransaction { + tx_hash: TxHash, + block_number: u64, + }, } impl From for FailureReason { @@ -59,15 +64,15 @@ impl From for FailureReason { match error { // Local Errors Web3Error::Decoder(error) => FailureReason::Local(LocalError::Decoder(error)), - Web3Error::Transport(error) => FailureReason::Local(LocalError::Transport(error)), - + Web3Error::Internal => FailureReason::Local(LocalError::Internal), Web3Error::Io(error) => FailureReason::Local(LocalError::Io(error.to_string())), Web3Error::Signing(error) => { + // This variant can't be tested because it's not possible to create a Web3Error of this type. FailureReason::Local(LocalError::Signing(error.to_string())) } - Web3Error::Internal => FailureReason::Local(LocalError::Internal), + Web3Error::Transport(error) => FailureReason::Local(LocalError::Transport(error)), + // Api Errors - Web3Error::Unreachable => FailureReason::Api(ApiError::Unreachable), Web3Error::InvalidResponse(response) => { FailureReason::Api(ApiError::InvalidResponse(response)) } @@ -75,6 +80,7 @@ impl From for FailureReason { code: web3_rpc_error.code.code(), message: web3_rpc_error.message, }), + Web3Error::Unreachable => FailureReason::Api(ApiError::Unreachable), } } } @@ -418,16 +424,19 @@ mod tests { use crate::accountant::db_access_objects::test_utils::{ make_read_only_db_connection, FailedTxBuilder, }; - use crate::accountant::db_access_objects::utils::current_unix_timestamp; + use crate::accountant::db_access_objects::utils::{current_unix_timestamp, TxHash}; use crate::blockchain::test_utils::make_tx_hash; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, }; use crate::database::test_utils::ConnectionWrapperMock; + use jsonrpc_core::types::error::Error as ErrorKind; + use jsonrpc_core::types::error::ErrorCode as Web3RpcErrorCode; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::Connection; use std::collections::{HashMap, HashSet}; use std::str::FromStr; + use web3::error::Error as Web3Error; #[test] fn insert_new_records_works() { @@ -666,16 +675,21 @@ mod tests { // Pool errors (lexicographically sorted) assert_eq!( - FailureReason::from_str(r#"{"Pool":{"NonceTooHigh":null}}"#).unwrap(), - FailureReason::Pool(PoolError::NonceTooHigh) - ); - assert_eq!( - FailureReason::from_str(r#"{"Pool":{"NonceTooLow":null}}"#).unwrap(), - FailureReason::Pool(PoolError::NonceTooLow) + FailureReason::from_str( + r#"{"Pool":{"NonceIssue":{"latest_used_nonce":123,"tx_nonce":456}}}"# + ) + .unwrap(), + FailureReason::Pool(PoolError::NonceIssue { + latest_used_nonce: 123, + tx_nonce: 456 + }) ); assert_eq!( - FailureReason::from_str(r#"{"Pool":{"OrphanedTransaction":null}}"#).unwrap(), - FailureReason::Pool(PoolError::OrphanedTransaction) + FailureReason::from_str(r#"{"Pool":{"OrphanedTransaction":{"tx_hash":"0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef","block_number":789}}}"#).unwrap(), + FailureReason::Pool(PoolError::OrphanedTransaction { + tx_hash: TxHash::from_str("1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef").unwrap(), + block_number: 789 + }) ); // PendingTooLong @@ -704,6 +718,51 @@ mod tests { ); } + #[test] + fn web3_error_to_failure_reason_conversion_works() { + // Local Errors + assert_eq!( + FailureReason::from(Web3Error::Decoder("Decoder error".to_string())), + FailureReason::Local(LocalError::Decoder("Decoder error".to_string())) + ); + assert_eq!( + FailureReason::from(Web3Error::Internal), + FailureReason::Local(LocalError::Internal) + ); + assert_eq!( + FailureReason::from(Web3Error::Io(std::io::Error::new( + std::io::ErrorKind::Other, + "IO error" + ))), + FailureReason::Local(LocalError::Io("IO error".to_string())) + ); + assert_eq!( + FailureReason::from(Web3Error::Transport("Transport error".to_string())), + FailureReason::Local(LocalError::Transport("Transport error".to_string())) + ); + + // Api Errors + assert_eq!( + FailureReason::from(Web3Error::InvalidResponse("Invalid response".to_string())), + FailureReason::Api(ApiError::InvalidResponse("Invalid response".to_string())) + ); + assert_eq!( + FailureReason::from(Web3Error::Rpc(ErrorKind { + code: Web3RpcErrorCode::ServerError(42), + message: "RPC error".to_string(), + data: None, + })), + FailureReason::Api(ApiError::Web3RpcError { + code: 42, + message: "RPC error".to_string(), + }) + ); + assert_eq!( + FailureReason::from(Web3Error::Unreachable), + FailureReason::Api(ApiError::Unreachable) + ); + } + #[test] fn failure_status_from_str_works() { assert_eq!(FailureStatus::from_str("RetryRequired"), Ok(RetryRequired)); From a562f6fe0bf526ba392bbc5d1284716e3a785ee6 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 17:04:41 +0530 Subject: [PATCH 6/8] GH-672: final touches --- .../accountant/db_access_objects/failed_payable_dao.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index cd6b9ab23..e5ea29e0a 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -26,7 +26,7 @@ pub enum FailedPayableDaoError { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum FailureReason { Local(LocalError), - Api(ApiError), // It includes the RPC + Api(ApiError), Pool(PoolError), PendingTooLong, } @@ -67,7 +67,7 @@ impl From for FailureReason { Web3Error::Internal => FailureReason::Local(LocalError::Internal), Web3Error::Io(error) => FailureReason::Local(LocalError::Io(error.to_string())), Web3Error::Signing(error) => { - // This variant can't be tested because it's not possible to create a Web3Error of this type. + // This variant cannot be tested due to import limitations. FailureReason::Local(LocalError::Signing(error.to_string())) } Web3Error::Transport(error) => FailureReason::Local(LocalError::Transport(error)), @@ -430,8 +430,6 @@ mod tests { DbInitializationConfig, DbInitializer, DbInitializerReal, }; use crate::database::test_utils::ConnectionWrapperMock; - use jsonrpc_core::types::error::Error as ErrorKind; - use jsonrpc_core::types::error::ErrorCode as Web3RpcErrorCode; use masq_lib::test_utils::utils::ensure_node_home_directory_exists; use rusqlite::Connection; use std::collections::{HashMap, HashSet}; @@ -747,8 +745,8 @@ mod tests { FailureReason::Api(ApiError::InvalidResponse("Invalid response".to_string())) ); assert_eq!( - FailureReason::from(Web3Error::Rpc(ErrorKind { - code: Web3RpcErrorCode::ServerError(42), + FailureReason::from(Web3Error::Rpc(jsonrpc_core::types::error::Error { + code: jsonrpc_core::types::error::ErrorCode::ServerError(42), message: "RPC error".to_string(), data: None, })), From e5fb3509ce5ce64bff13acf11df93c8a60727960 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Sun, 20 Jul 2025 17:09:57 +0530 Subject: [PATCH 7/8] GH-672: return String as Error instead of an error from serde --- .../db_access_objects/failed_payable_dao.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index e5ea29e0a..266e3eac0 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -95,10 +95,10 @@ impl Display for FailureReason { } impl FromStr for FailureReason { - type Err = serde_json::Error; + type Err = String; fn from_str(s: &str) -> Result { - serde_json::from_str(s) + serde_json::from_str(s).map_err(|e| e.to_string()) } } @@ -698,9 +698,7 @@ mod tests { // Invalid Variant assert_eq!( - FailureReason::from_str(r#"{"UnknownReason":null}"#) - .unwrap_err() - .to_string(), + FailureReason::from_str(r#"{"UnknownReason":null}"#).unwrap_err(), "unknown variant `UnknownReason`, \ expected one of `Local`, `Api`, `Pool`, `PendingTooLong` \ at line 1 column 16" @@ -709,9 +707,7 @@ mod tests { // Invalid Input assert_eq!( - FailureReason::from_str("random string") - .unwrap_err() - .to_string(), + FailureReason::from_str("random string").unwrap_err(), "expected value at line 1 column 1".to_string() ); } From e3c2590999eb440b055b95baaead88fd19dbbf2c Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Mon, 21 Jul 2025 14:16:24 +0530 Subject: [PATCH 8/8] GH-672: better error classification --- .../db_access_objects/failed_payable_dao.rs | 196 +++--------------- node/src/blockchain/errors.rs | 127 ++++++++++++ node/src/blockchain/mod.rs | 1 + 3 files changed, 156 insertions(+), 168 deletions(-) create mode 100644 node/src/blockchain/errors.rs diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs index 266e3eac0..00d16080e 100644 --- a/node/src/accountant/db_access_objects/failed_payable_dao.rs +++ b/node/src/accountant/db_access_objects/failed_payable_dao.rs @@ -4,6 +4,7 @@ use crate::accountant::db_access_objects::utils::{ }; use crate::accountant::db_big_integer::big_int_divider::BigIntDivider; use crate::accountant::{checked_conversion, comma_joined_stringifiable}; +use crate::blockchain::errors::AppRpcError; use crate::database::rusqlite_wrappers::ConnectionWrapper; use itertools::Itertools; use masq_lib::utils::ExpectValue; @@ -11,7 +12,6 @@ use serde_derive::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; use std::fmt::{Display, Formatter}; use std::str::FromStr; -use web3::error::Error as Web3Error; use web3::types::Address; #[derive(Debug, PartialEq, Eq)] @@ -25,66 +25,12 @@ pub enum FailedPayableDaoError { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum FailureReason { - Local(LocalError), - Api(ApiError), - Pool(PoolError), + Submission(AppRpcError), + Validation(AppRpcError), + Reverted, PendingTooLong, } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub enum LocalError { - Decoder(String), - Internal, - Io(String), - Signing(String), - Transport(String), -} - -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub enum ApiError { - InvalidResponse(String), - Unreachable, - Web3RpcError { code: i64, message: String }, -} - -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub enum PoolError { - NonceIssue { - latest_used_nonce: u64, - tx_nonce: u64, - }, - OrphanedTransaction { - tx_hash: TxHash, - block_number: u64, - }, -} - -impl From for FailureReason { - fn from(error: Web3Error) -> Self { - match error { - // Local Errors - Web3Error::Decoder(error) => FailureReason::Local(LocalError::Decoder(error)), - Web3Error::Internal => FailureReason::Local(LocalError::Internal), - Web3Error::Io(error) => FailureReason::Local(LocalError::Io(error.to_string())), - Web3Error::Signing(error) => { - // This variant cannot be tested due to import limitations. - FailureReason::Local(LocalError::Signing(error.to_string())) - } - Web3Error::Transport(error) => FailureReason::Local(LocalError::Transport(error)), - - // Api Errors - Web3Error::InvalidResponse(response) => { - FailureReason::Api(ApiError::InvalidResponse(response)) - } - Web3Error::Rpc(web3_rpc_error) => FailureReason::Api(ApiError::Web3RpcError { - code: web3_rpc_error.code.code(), - message: web3_rpc_error.message, - }), - Web3Error::Unreachable => FailureReason::Api(ApiError::Unreachable), - } - } -} - impl Display for FailureReason { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match serde_json::to_string(self) { @@ -410,21 +356,21 @@ impl FailedPayableDaoFactory for DaoFactoryReal { #[cfg(test)] mod tests { - use crate::accountant::db_access_objects::failed_payable_dao::ApiError::Unreachable; use crate::accountant::db_access_objects::failed_payable_dao::FailureReason::{ - Api, PendingTooLong, + PendingTooLong, Reverted, }; use crate::accountant::db_access_objects::failed_payable_dao::FailureStatus::{ Concluded, RecheckRequired, RetryRequired, }; use crate::accountant::db_access_objects::failed_payable_dao::{ - ApiError, FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureReason, - FailureRetrieveCondition, FailureStatus, LocalError, PoolError, + FailedPayableDao, FailedPayableDaoError, FailedPayableDaoReal, FailureReason, + FailureRetrieveCondition, FailureStatus, }; use crate::accountant::db_access_objects::test_utils::{ make_read_only_db_connection, FailedTxBuilder, }; - use crate::accountant::db_access_objects::utils::{current_unix_timestamp, TxHash}; + use crate::accountant::db_access_objects::utils::current_unix_timestamp; + use crate::blockchain::errors::{AppRpcError, LocalError, RemoteError}; use crate::blockchain::test_utils::make_tx_hash; use crate::database::db_initializer::{ DbInitializationConfig, DbInitializer, DbInitializerReal, @@ -434,7 +380,6 @@ mod tests { use rusqlite::Connection; use std::collections::{HashMap, HashSet}; use std::str::FromStr; - use web3::error::Error as Web3Error; #[test] fn insert_new_records_works() { @@ -445,7 +390,7 @@ mod tests { .unwrap(); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(Api(Unreachable)) + .reason(Reverted) .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) @@ -626,68 +571,28 @@ mod tests { #[test] fn failure_reason_from_str_works() { - // Local errors (lexicographically sorted) - assert_eq!( - FailureReason::from_str(r#"{"Local":{"Decoder":"Test decoder error"}}"#).unwrap(), - FailureReason::Local(LocalError::Decoder("Test decoder error".to_string())) - ); - assert_eq!( - FailureReason::from_str(r#"{"Local":{"Internal":null}}"#).unwrap(), - FailureReason::Local(LocalError::Internal) - ); - assert_eq!( - FailureReason::from_str(r#"{"Local":{"Io":"Test IO error"}}"#).unwrap(), - FailureReason::Local(LocalError::Io("Test IO error".to_string())) - ); + // Submission error assert_eq!( - FailureReason::from_str(r#"{"Local":{"Signing":"Test signing error"}}"#).unwrap(), - FailureReason::Local(LocalError::Signing("Test signing error".to_string())) - ); - assert_eq!( - FailureReason::from_str(r#"{"Local":{"Transport":"Test transport error"}}"#).unwrap(), - FailureReason::Local(LocalError::Transport("Test transport error".to_string())) - ); - - // Api errors (lexicographically sorted) - assert_eq!( - FailureReason::from_str(r#"{"Api":{"InvalidResponse":"Test invalid response"}}"#) + FailureReason::from_str(r#"{"Submission":{"Local":{"Decoder":"Test decoder error"}}}"#) .unwrap(), - FailureReason::Api(ApiError::InvalidResponse( - "Test invalid response".to_string() - )) - ); - assert_eq!( - FailureReason::from_str(r#"{"Api":{"Unreachable":null}}"#).unwrap(), - FailureReason::Api(ApiError::Unreachable) + FailureReason::Submission(AppRpcError::Local(LocalError::Decoder( + "Test decoder error".to_string() + ))) ); + + // Validation error assert_eq!( - FailureReason::from_str( - r#"{"Api":{"Web3RpcError":{"code":123,"message":"Test RPC error"}}}"# - ) - .unwrap(), - FailureReason::Api(ApiError::Web3RpcError { - code: 123, + FailureReason::from_str(r#"{"Validation":{"Remote":{"Web3RpcError":{"code":42,"message":"Test RPC error"}}}}"#).unwrap(), + FailureReason::Validation(AppRpcError::Remote(RemoteError::Web3RpcError { + code: 42, message: "Test RPC error".to_string() - }) + })) ); - // Pool errors (lexicographically sorted) - assert_eq!( - FailureReason::from_str( - r#"{"Pool":{"NonceIssue":{"latest_used_nonce":123,"tx_nonce":456}}}"# - ) - .unwrap(), - FailureReason::Pool(PoolError::NonceIssue { - latest_used_nonce: 123, - tx_nonce: 456 - }) - ); + // Reverted assert_eq!( - FailureReason::from_str(r#"{"Pool":{"OrphanedTransaction":{"tx_hash":"0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef","block_number":789}}}"#).unwrap(), - FailureReason::Pool(PoolError::OrphanedTransaction { - tx_hash: TxHash::from_str("1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef").unwrap(), - block_number: 789 - }) + FailureReason::from_str(r#"{"Reverted":null}"#).unwrap(), + FailureReason::Reverted ); // PendingTooLong @@ -700,8 +605,8 @@ mod tests { assert_eq!( FailureReason::from_str(r#"{"UnknownReason":null}"#).unwrap_err(), "unknown variant `UnknownReason`, \ - expected one of `Local`, `Api`, `Pool`, `PendingTooLong` \ - at line 1 column 16" + expected one of `Submission`, `Validation`, `Reverted`, `PendingTooLong` \ + at line 1 column 16" .to_string() ); @@ -712,51 +617,6 @@ mod tests { ); } - #[test] - fn web3_error_to_failure_reason_conversion_works() { - // Local Errors - assert_eq!( - FailureReason::from(Web3Error::Decoder("Decoder error".to_string())), - FailureReason::Local(LocalError::Decoder("Decoder error".to_string())) - ); - assert_eq!( - FailureReason::from(Web3Error::Internal), - FailureReason::Local(LocalError::Internal) - ); - assert_eq!( - FailureReason::from(Web3Error::Io(std::io::Error::new( - std::io::ErrorKind::Other, - "IO error" - ))), - FailureReason::Local(LocalError::Io("IO error".to_string())) - ); - assert_eq!( - FailureReason::from(Web3Error::Transport("Transport error".to_string())), - FailureReason::Local(LocalError::Transport("Transport error".to_string())) - ); - - // Api Errors - assert_eq!( - FailureReason::from(Web3Error::InvalidResponse("Invalid response".to_string())), - FailureReason::Api(ApiError::InvalidResponse("Invalid response".to_string())) - ); - assert_eq!( - FailureReason::from(Web3Error::Rpc(jsonrpc_core::types::error::Error { - code: jsonrpc_core::types::error::ErrorCode::ServerError(42), - message: "RPC error".to_string(), - data: None, - })), - FailureReason::Api(ApiError::Web3RpcError { - code: 42, - message: "RPC error".to_string(), - }) - ); - assert_eq!( - FailureReason::from(Web3Error::Unreachable), - FailureReason::Api(ApiError::Unreachable) - ); - } - #[test] fn failure_status_from_str_works() { assert_eq!(FailureStatus::from_str("RetryRequired"), Ok(RetryRequired)); @@ -822,7 +682,7 @@ mod tests { .build(); let tx2 = FailedTxBuilder::default() .hash(make_tx_hash(2)) - .reason(Api(Unreachable)) + .reason(Reverted) .timestamp(now - 3600) .status(RetryRequired) .build(); @@ -856,7 +716,7 @@ mod tests { let subject = FailedPayableDaoReal::new(wrapped_conn); let tx1 = FailedTxBuilder::default() .hash(make_tx_hash(1)) - .reason(Api(Unreachable)) + .reason(Reverted) .status(RetryRequired) .build(); let tx2 = FailedTxBuilder::default() diff --git a/node/src/blockchain/errors.rs b/node/src/blockchain/errors.rs new file mode 100644 index 000000000..865bea29c --- /dev/null +++ b/node/src/blockchain/errors.rs @@ -0,0 +1,127 @@ +use serde_derive::{Deserialize, Serialize}; +use web3::error::Error as Web3Error; + +// Prefixed with App to clearly distinguish app-specific errors from library errors. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum AppRpcError { + Local(LocalError), + Remote(RemoteError), +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum LocalError { + Decoder(String), + Internal, + Io(String), + Signing(String), + Transport(String), +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum RemoteError { + InvalidResponse(String), + Unreachable, + Web3RpcError { code: i64, message: String }, +} + +// EVM based errors +impl From for AppRpcError { + fn from(error: Web3Error) -> Self { + match error { + // Local Errors + Web3Error::Decoder(error) => AppRpcError::Local(LocalError::Decoder(error)), + Web3Error::Internal => AppRpcError::Local(LocalError::Internal), + Web3Error::Io(error) => AppRpcError::Local(LocalError::Io(error.to_string())), + Web3Error::Signing(error) => { + // This variant cannot be tested due to import limitations. + AppRpcError::Local(LocalError::Signing(error.to_string())) + } + Web3Error::Transport(error) => AppRpcError::Local(LocalError::Transport(error)), + + // Api Errors + Web3Error::InvalidResponse(response) => { + AppRpcError::Remote(RemoteError::InvalidResponse(response)) + } + Web3Error::Rpc(web3_rpc_error) => AppRpcError::Remote(RemoteError::Web3RpcError { + code: web3_rpc_error.code.code(), + message: web3_rpc_error.message, + }), + Web3Error::Unreachable => AppRpcError::Remote(RemoteError::Unreachable), + } + } +} + +mod tests { + use crate::blockchain::errors::{AppRpcError, LocalError, RemoteError}; + use web3::error::Error as Web3Error; + + #[test] + fn web3_error_to_failure_reason_conversion_works() { + // Local Errors + assert_eq!( + AppRpcError::from(Web3Error::Decoder("Decoder error".to_string())), + AppRpcError::Local(LocalError::Decoder("Decoder error".to_string())) + ); + assert_eq!( + AppRpcError::from(Web3Error::Internal), + AppRpcError::Local(LocalError::Internal) + ); + assert_eq!( + AppRpcError::from(Web3Error::Io(std::io::Error::new( + std::io::ErrorKind::Other, + "IO error" + ))), + AppRpcError::Local(LocalError::Io("IO error".to_string())) + ); + assert_eq!( + AppRpcError::from(Web3Error::Transport("Transport error".to_string())), + AppRpcError::Local(LocalError::Transport("Transport error".to_string())) + ); + + // Api Errors + assert_eq!( + AppRpcError::from(Web3Error::InvalidResponse("Invalid response".to_string())), + AppRpcError::Remote(RemoteError::InvalidResponse("Invalid response".to_string())) + ); + assert_eq!( + AppRpcError::from(Web3Error::Rpc(jsonrpc_core::types::error::Error { + code: jsonrpc_core::types::error::ErrorCode::ServerError(42), + message: "RPC error".to_string(), + data: None, + })), + AppRpcError::Remote(RemoteError::Web3RpcError { + code: 42, + message: "RPC error".to_string(), + }) + ); + assert_eq!( + AppRpcError::from(Web3Error::Unreachable), + AppRpcError::Remote(RemoteError::Unreachable) + ); + } + + #[test] + fn app_rpc_error_serialization_deserialization() { + let errors = vec![ + // Local Errors + AppRpcError::Local(LocalError::Decoder("Decoder error".to_string())), + AppRpcError::Local(LocalError::Internal), + AppRpcError::Local(LocalError::Io("IO error".to_string())), + AppRpcError::Local(LocalError::Signing("Signing error".to_string())), + AppRpcError::Local(LocalError::Transport("Transport error".to_string())), + // Remote Errors + AppRpcError::Remote(RemoteError::InvalidResponse("Invalid response".to_string())), + AppRpcError::Remote(RemoteError::Unreachable), + AppRpcError::Remote(RemoteError::Web3RpcError { + code: 42, + message: "RPC error".to_string(), + }), + ]; + + errors.into_iter().for_each(|error| { + let serialized = serde_json::to_string(&error).unwrap(); + let deserialized: AppRpcError = serde_json::from_str(&serialized).unwrap(); + assert_eq!(error, deserialized, "Error: {:?}", error); + }); + } +} diff --git a/node/src/blockchain/mod.rs b/node/src/blockchain/mod.rs index 48698c408..f3ef3d323 100644 --- a/node/src/blockchain/mod.rs +++ b/node/src/blockchain/mod.rs @@ -5,6 +5,7 @@ pub mod blockchain_agent; pub mod blockchain_bridge; pub mod blockchain_interface; pub mod blockchain_interface_initializer; +pub mod errors; pub mod payer; pub mod signature; #[cfg(test)]