GH-672: Improve FailureReason (#673)

utkarshg6 · web-flow · commit 3a11c6e2a10f · 2025-07-21T16:18:11.000+05:30
* GH-672: add more errors * GH-672: improve errors; compiling * GH-672: introduce the conversion of FailureReason * GH-672: all tests are passing :) * GH-672: From conversion is properly tested * GH-672: final touches * GH-672: return String as Error instead of an error from serde * GH-672: better error classification
diff --git a/node/src/accountant/db_access_objects/failed_payable_dao.rs b/node/src/accountant/db_access_objects/failed_payable_dao.rs
@@ -4,9 +4,11 @@ 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;
+use serde_derive::{Deserialize, Serialize};
 use std::collections::{HashMap, HashSet};
 use std::fmt::{Display, Formatter};
 use std::str::FromStr;
@@ -21,11 +23,29 @@ pub enum FailedPayableDaoError {
     SqlExecutionFailed(String),
 }
 
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub enum FailureReason {
+    Submission(AppRpcError),
+    Validation(AppRpcError),
+    Reverted,
     PendingTooLong,
-    NonceIssue,
-    General,
+}
+
+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, "<invalid FailureReason>"),
+        }
+    }
+}
+
+impl FromStr for FailureReason {
+    type Err = String;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        serde_json::from_str(s).map_err(|e| e.to_string())
+    }
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -47,19 +67,6 @@ impl FromStr for FailureStatus {
     }
 }
 
-impl FromStr for FailureReason {
-    type Err = String;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        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,
@@ -174,7 +181,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,
@@ -350,7 +357,7 @@ impl FailedPayableDaoFactory for DaoFactoryReal {
 #[cfg(test)]
 mod tests {
     use crate::accountant::db_access_objects::failed_payable_dao::FailureReason::{
-        General, NonceIssue, PendingTooLong,
+        PendingTooLong, Reverted,
     };
     use crate::accountant::db_access_objects::failed_payable_dao::FailureStatus::{
         Concluded, RecheckRequired, RetryRequired,
@@ -363,6 +370,7 @@ mod tests {
         make_read_only_db_connection, FailedTxBuilder,
     };
     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,
@@ -382,7 +390,7 @@ mod tests {
             .unwrap();
         let tx1 = FailedTxBuilder::default()
             .hash(make_tx_hash(1))
-            .reason(NonceIssue)
+            .reason(Reverted)
             .build();
         let tx2 = FailedTxBuilder::default()
             .hash(make_tx_hash(2))
@@ -563,15 +571,49 @@ mod tests {
 
     #[test]
     fn failure_reason_from_str_works() {
+        // Submission error
         assert_eq!(
-            FailureReason::from_str("PendingTooLong"),
-            Ok(PendingTooLong)
+            FailureReason::from_str(r#"{"Submission":{"Local":{"Decoder":"Test decoder error"}}}"#)
+                .unwrap(),
+            FailureReason::Submission(AppRpcError::Local(LocalError::Decoder(
+                "Test decoder error".to_string()
+            )))
         );
-        assert_eq!(FailureReason::from_str("NonceIssue"), Ok(NonceIssue));
-        assert_eq!(FailureReason::from_str("General"), Ok(General));
+
+        // Validation error
+        assert_eq!(
+            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()
+            }))
+        );
+
+        // Reverted
+        assert_eq!(
+            FailureReason::from_str(r#"{"Reverted":null}"#).unwrap(),
+            FailureReason::Reverted
+        );
+
+        // PendingTooLong
+        assert_eq!(
+            FailureReason::from_str(r#"{"PendingTooLong":null}"#).unwrap(),
+            FailureReason::PendingTooLong
+        );
+
+        // Invalid Variant
+        assert_eq!(
+            FailureReason::from_str(r#"{"UnknownReason":null}"#).unwrap_err(),
+            "unknown variant `UnknownReason`, \
+            expected one of `Submission`, `Validation`, `Reverted`, `PendingTooLong` \
+            at line 1 column 16"
+                .to_string()
+        );
+
+        // Invalid Input
         assert_eq!(
-            FailureReason::from_str("InvalidReason"),
-            Err("Invalid FailureReason: InvalidReason".to_string())
+            FailureReason::from_str("random string").unwrap_err(),
+            "expected value at line 1 column 1".to_string()
         );
     }
 
@@ -640,7 +682,7 @@ mod tests {
             .build();
         let tx2 = FailedTxBuilder::default()
             .hash(make_tx_hash(2))
-            .reason(NonceIssue)
+            .reason(Reverted)
             .timestamp(now - 3600)
             .status(RetryRequired)
             .build();
@@ -674,7 +716,7 @@ mod tests {
         let subject = FailedPayableDaoReal::new(wrapped_conn);
         let tx1 = FailedTxBuilder::default()
             .hash(make_tx_hash(1))
-            .reason(NonceIssue)
+            .reason(Reverted)
             .status(RetryRequired)
             .build();
         let tx2 = FailedTxBuilder::default()
diff --git a/node/src/blockchain/errors.rs 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<Web3Error> 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
@@ -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)]
