refactor: use typed errors in eth trace

src/rpc/methods/eth/trace/parity.rs

@@ -13,7 +13,7 @@ use super::super::{decode_payload, encode_filecoin_params_as_abi, encode_filecoi
 use super::Environment;
 use super::types::{
     EthCallTraceAction, EthCallTraceResult, EthCreateTraceAction, EthCreateTraceResult, EthTrace,
-    TraceAction, TraceResult,
+    TraceAction, TraceError, TraceResult,
 };
 use super::utils::trace_to_address;
 use crate::eth::{EAMMethod, EVMMethod};
@@ -29,17 +29,6 @@ use fvm_ipld_blockstore::Blockstore;
 use num::FromPrimitive;
 use tracing::debug;
 
-/// Error string used in Parity-format traces.
-pub const PARITY_TRACE_REVERT_ERROR: &str = "Reverted";
-pub const PARITY_EVM_INVALID_INSTRUCTION: &str = "invalid instruction";
-pub const PARITY_EVM_UNDEFINED_INSTRUCTION: &str = "undefined instruction";
-pub const PARITY_EVM_STACK_UNDERFLOW: &str = "stack underflow";
-pub const PARITY_EVM_STACK_OVERFLOW: &str = "stack overflow";
-pub const PARITY_EVM_ILLEGAL_MEMORY_ACCESS: &str = "illegal memory access";
-pub const PARITY_EVM_BAD_JUMPDEST: &str = "invalid jump destination";
-pub const PARITY_EVM_SELFDESTRUCT_FAILED: &str = "self destruct failed";
-pub const PARITY_EVM_OUT_OF_GAS: &str = "out of gas";
-
 /// Returns `true` if the invoked actor is an EVM contract or the Ethereum Account Manager.
 fn trace_is_evm_or_eam(trace: &ExecutionTrace) -> bool {
     if let Some(invoked_actor) = &trace.invoked_actor {
@@ -50,49 +39,45 @@ fn trace_is_evm_or_eam(trace: &ExecutionTrace) -> bool {
     }
 }
 
-/// Converts a trace's exit code into a human-readable Parity-style error string.
+/// Converts a trace's exit code into a typed [`TraceError`].
 /// Returns `None` when the trace completed successfully.
-fn trace_err_msg(trace: &ExecutionTrace) -> Option<String> {
+fn trace_err_msg(trace: &ExecutionTrace) -> Option<TraceError> {
     let code = trace.msg_rct.exit_code;
 
     if code.is_success() {
         return None;
     }
 
-    // EVM tools often expect this literal string.
     if code == ExitCode::SYS_OUT_OF_GAS {
-        return Some(PARITY_EVM_OUT_OF_GAS.into());
+        return Some(TraceError::OutOfGas);
     }
 
-    // indicate when we have a "system" error.
     if code < ExitCode::FIRST_ACTOR_ERROR_CODE.into() {
-        return Some(format!("vm error: {code}"));
+        return Some(TraceError::VmError(code.value()));
     }
 
-    // handle special exit codes from the EVM/EAM.
     if trace_is_evm_or_eam(trace) {
         match code.into() {
-            evm12::EVM_CONTRACT_REVERTED => return Some(PARITY_TRACE_REVERT_ERROR.into()), // capitalized for compatibility
+            evm12::EVM_CONTRACT_REVERTED => return Some(TraceError::Reverted),
             evm12::EVM_CONTRACT_INVALID_INSTRUCTION => {
-                return Some(PARITY_EVM_INVALID_INSTRUCTION.into());
+                return Some(TraceError::InvalidInstruction);
             }
             evm12::EVM_CONTRACT_UNDEFINED_INSTRUCTION => {
-                return Some(PARITY_EVM_UNDEFINED_INSTRUCTION.into());
+                return Some(TraceError::UndefinedInstruction);
             }
-            evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some(PARITY_EVM_STACK_UNDERFLOW.into()),
-            evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some(PARITY_EVM_STACK_OVERFLOW.into()),
+            evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some(TraceError::StackUnderflow),
+            evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some(TraceError::StackOverflow),
             evm12::EVM_CONTRACT_ILLEGAL_MEMORY_ACCESS => {
-                return Some(PARITY_EVM_ILLEGAL_MEMORY_ACCESS.into());
+                return Some(TraceError::IllegalMemoryAccess);
             }
-            evm12::EVM_CONTRACT_BAD_JUMPDEST => return Some(PARITY_EVM_BAD_JUMPDEST.into()),
+            evm12::EVM_CONTRACT_BAD_JUMPDEST => return Some(TraceError::BadJumpDest),
             evm12::EVM_CONTRACT_SELFDESTRUCT_FAILED => {
-                return Some(PARITY_EVM_SELFDESTRUCT_FAILED.into());
+                return Some(TraceError::SelfDestructFailed);
             }
             _ => (),
         }
     }
-    // everything else...
-    Some(format!("actor error: {code}"))
+    Some(TraceError::ActorError(code.value()))
 }
 
 /// Recursively builds the traces for a given ExecutionTrace by walking the subcalls

src/rpc/methods/eth/trace/types.rs

@@ -9,13 +9,99 @@
 use super::super::types::{EthAddress, EthAddressList, EthBytes, EthHash};
 use super::super::{EthBigInt, EthUint64};
 use crate::lotus_json::lotus_json_with_self;
+use crate::rpc::eth::trace::GETH_TRACE_REVERT_ERROR;
 use crate::rpc::eth::trace::utils::extract_revert_reason;
-use crate::rpc::eth::trace::{GETH_TRACE_REVERT_ERROR, PARITY_TRACE_REVERT_ERROR};
+use crate::shim::error::ExitCode;
 use anyhow::{Context as _, Result, bail};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use std::collections::BTreeMap;
 
+/// Typed error for Parity-style EVM trace entries.
+#[derive(Debug, Hash, Clone, PartialEq, Eq, thiserror::Error)]
+pub enum TraceError {
+    #[error("Reverted")]
+    Reverted,
+    #[error("out of gas")]
+    OutOfGas,
+    #[error("invalid instruction")]
+    InvalidInstruction,
+    #[error("undefined instruction")]
+    UndefinedInstruction,
+    #[error("stack underflow")]
+    StackUnderflow,
+    #[error("stack overflow")]
+    StackOverflow,
+    #[error("illegal memory access")]
+    IllegalMemoryAccess,
+    #[error("invalid jump destination")]
+    BadJumpDest,
+    #[error("self destruct failed")]
+    SelfDestructFailed,
+    /// System-level VM error (exit code < FIRST_ACTOR_ERROR_CODE).
+    #[error("vm error: {}", ExitCode::from(*.0))]
+    VmError(u32),
+    /// Actor-level error (catch-all for unrecognised exit codes).
+    #[error("actor error: {}", ExitCode::from(*.0))]
+    ActorError(u32),
+}
+
+impl Serialize for TraceError {
+    fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
+        serializer.serialize_str(&self.to_string())
+    }
+}
+
+impl<'de> Deserialize<'de> for TraceError {
+    fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+        let s = String::deserialize(deserializer)?;
+        Ok(TraceError::from_string(&s))
+    }
+}
+
+impl TraceError {
+    pub fn from_string(s: &str) -> Self {
+        match s {
+            "Reverted" => Self::Reverted,
+            "out of gas" => Self::OutOfGas,
+            "invalid instruction" => Self::InvalidInstruction,
+            "undefined instruction" => Self::UndefinedInstruction,
+            "stack underflow" => Self::StackUnderflow,
+            "stack overflow" => Self::StackOverflow,
+            "illegal memory access" => Self::IllegalMemoryAccess,
+            "invalid jump destination" => Self::BadJumpDest,
+            "self destruct failed" => Self::SelfDestructFailed,
+            other => {
+                if let Some(rest) = other.strip_prefix("vm error: ") {
+                    Self::VmError(parse_exit_code_display(rest))
+                } else if let Some(rest) = other.strip_prefix("actor error: ") {
+                    Self::ActorError(parse_exit_code_display(rest))
+                } else {
+                    Self::ActorError(0)
+                }
+            }
+        }
+    }
+
+    /// Converts this Parity-format error to the equivalent Geth-format string.
+    pub fn to_geth_error_string(&self) -> String {
+        match self {
+            Self::Reverted => GETH_TRACE_REVERT_ERROR.into(),
+            other => other.to_string(),
+        }
+    }
+}
+
+/// Parses `ExitCode`'s display format back to its `u32` value.
+/// Handles both `"Name(N)"` (e.g., `"SysErrOutOfGas(7)"`) and plain `"N"`.
+fn parse_exit_code_display(s: &str) -> u32 {
+    s.rsplit_once('(')
+        .and_then(|(_, n)| n.strip_suffix(')'))
+        .unwrap_or(s)
+        .parse()
+        .unwrap_or(0)
+}
+
 #[derive(Eq, Hash, PartialEq, Default, Serialize, Deserialize, Debug, Clone, JsonSchema)]
 #[serde(rename_all = "camelCase")]
 pub struct EthCallTraceAction {
@@ -419,7 +505,8 @@ pub struct EthTrace {
     pub action: TraceAction,
     pub result: TraceResult,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub error: Option<String>,
+    #[schemars(with = "Option<String>")]
+    pub error: Option<TraceError>,
 }
 
 impl EthTrace {
@@ -428,23 +515,13 @@ impl EthTrace {
     }
 
     /// Returns true if the trace is a revert error.
-    ///
-    /// This is not a complete check for reverted traces (there are other possible revert reasons).
     pub fn is_reverted(&self) -> bool {
-        self.error
-            .as_deref()
-            .is_some_and(|e| e == PARITY_TRACE_REVERT_ERROR)
+        matches!(self.error, Some(TraceError::Reverted))
     }
 
     /// Converts the Parity-format error stored in this trace to the Geth-format.
     pub fn to_geth_error(&self) -> Option<String> {
-        self.error.as_deref().map(|error| {
-            if error == PARITY_TRACE_REVERT_ERROR {
-                GETH_TRACE_REVERT_ERROR.into()
-            } else {
-                error.to_string()
-            }
-        })
+        self.error.as_ref().map(TraceError::to_geth_error_string)
     }
 
     /// Converts a Parity-style [`EthTrace`] into a Geth-style [`GethCallFrame`].
@@ -1131,7 +1208,7 @@ mod tests {
                 gas_used: EthUint64(100),
                 output: EthBytes(vec![]),
             }),
-            error: Some(PARITY_TRACE_REVERT_ERROR.into()),
+            error: Some(TraceError::Reverted),
             ..EthTrace::default()
         };
 
@@ -1240,7 +1317,7 @@ mod tests {
                 code: EthBytes(vec![]),
             }),
             error: if result_address.is_none() {
-                Some("ErrForbidden".into())
+                Some(TraceError::Reverted)
             } else {
                 None
             },
@@ -1355,4 +1432,55 @@ mod tests {
         };
         assert!(trace.match_filter_criteria(None, None).is_err());
     }
+
+    #[rstest]
+    #[case(TraceError::Reverted, "\"Reverted\"")]
+    #[case(TraceError::OutOfGas, "\"out of gas\"")]
+    #[case(TraceError::InvalidInstruction, "\"invalid instruction\"")]
+    #[case(TraceError::UndefinedInstruction, "\"undefined instruction\"")]
+    #[case(TraceError::StackUnderflow, "\"stack underflow\"")]
+    #[case(TraceError::StackOverflow, "\"stack overflow\"")]
+    #[case(TraceError::IllegalMemoryAccess, "\"illegal memory access\"")]
+    #[case(TraceError::BadJumpDest, "\"invalid jump destination\"")]
+    #[case(TraceError::SelfDestructFailed, "\"self destruct failed\"")]
+    fn test_trace_error_serialization_round_trip(
+        #[case] error: TraceError,
+        #[case] expected_json: &str,
+    ) {
+        let json = serde_json::to_string(&error).unwrap();
+        assert_eq!(json, expected_json);
+
+        let deserialized: TraceError = serde_json::from_str(&json).unwrap();
+        assert_eq!(deserialized, error);
+    }
+
+    #[test]
+    fn test_trace_error_vm_error_serialization() {
+        // ExitCode 7 = SYS_OUT_OF_GAS, but VmError is for other system codes
+        let error = TraceError::VmError(5);
+        let json = serde_json::to_string(&error).unwrap();
+        assert!(json.contains("vm error:"));
+
+        let deserialized: TraceError = serde_json::from_str(&json).unwrap();
+        assert_eq!(deserialized, error);
+    }
+
+    #[test]
+    fn test_trace_error_actor_error_serialization() {
+        let error = TraceError::ActorError(33);
+        let json = serde_json::to_string(&error).unwrap();
+        assert!(json.contains("actor error:"));
+
+        let deserialized: TraceError = serde_json::from_str(&json).unwrap();
+        assert_eq!(deserialized, error);
+    }
+
+    #[test]
+    fn test_trace_error_to_geth_error_string() {
+        assert_eq!(
+            TraceError::Reverted.to_geth_error_string(),
+            "execution reverted"
+        );
+        assert_eq!(TraceError::OutOfGas.to_geth_error_string(), "out of gas");
+    }
 }