feat(tool): dump RPC tests to a directory (#5054)

CHANGELOG.md

@@ -53,6 +53,9 @@ methods, fixes (notably to the garbage collection), and other improvements.
 - [#5028](https://github.com/ChainSafe/forest/pull/5028) Added
   `forest-cli f3 powertable get-proportion` CLI command.
 
+- [#5054](https://github.com/ChainSafe/forest/pull/5054) Added `--dump-dir`
+  option to `forest-tool api compare` CLI command.
+
 - [#4704](https://github.com/ChainSafe/forest/issues/4704) Add support for the
   `Filecoin.EthGetTransactionReceiptLimited` RPC method.
 

src/rpc/client.rs

@@ -79,7 +79,7 @@ impl Client {
             timeout,
             ..
         } = req;
-
+        let method_name = method_name.as_ref();
         let client = self.get_or_init_client(api_paths).await?;
         let span = tracing::debug_span!("request", method = %method_name, url = %client.url);
         let work = async {

src/rpc/reflect/mod.rs

@@ -96,11 +96,12 @@ pub enum Permission {
 /// Which paths should this method be exposed on?
 ///
 /// This information is important when using [`crate::rpc::client`].
-#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)]
+#[derive(Debug, Default, Clone, Copy, Hash, Eq, PartialEq, Ord, PartialOrd)]
 pub enum ApiPaths {
     /// Only expose this method on `/rpc/v0`
     V0,
     /// Only expose this method on `/rpc/v1`
+    #[default]
     V1,
     /// Expose this method on both `/rpc/v0` and `/rpc/v1`
     #[allow(dead_code)]
@@ -233,7 +234,7 @@ pub trait RpcMethodExt<const ARITY: usize>: RpcMethod<ARITY> {
         // hardcode calling convention because lotus is by-position only
         let params = Self::request_params(params)?;
         Ok(crate::rpc::Request {
-            method_name: Self::NAME,
+            method_name: Self::NAME.into(),
             params,
             result_type: std::marker::PhantomData,
             api_paths: Self::API_PATHS,
@@ -275,7 +276,7 @@ pub trait RpcMethodExt<const ARITY: usize>: RpcMethod<ARITY> {
         };
 
         Ok(crate::rpc::Request {
-            method_name: name,
+            method_name: name.into(),
             params,
             result_type: std::marker::PhantomData,
             api_paths: Self::API_PATHS,

src/rpc/request.rs

@@ -3,16 +3,20 @@
 
 use super::ApiPaths;
 use jsonrpsee::core::traits::ToRpcParams;
+use serde::{Deserialize, Serialize};
 use std::{marker::PhantomData, time::Duration};
 
 /// An at-rest description of a remote procedure call, created using
 /// [`rpc::RpcMethodExt`](crate::rpc::RpcMethodExt::request), and called using [`rpc::Client::call`](crate::rpc::Client::call).
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct Request<T = serde_json::Value> {
-    pub method_name: &'static str,
+    pub method_name: std::borrow::Cow<'static, str>,
     pub params: serde_json::Value,
+    #[serde(skip)]
     pub result_type: PhantomData<T>,
+    #[serde(skip)]
     pub api_paths: ApiPaths,
+    #[serde(skip)]
     pub timeout: Duration,
 }
 

src/tool/subcommands/api_cmd.rs

@@ -7,17 +7,16 @@ use crate::eth::{EthChainId as EthChainIdType, SAFE_EPOCH_DELAY};
 use crate::lotus_json::HasLotusJson;
 use crate::message::{Message as _, SignedMessage};
 use crate::networks::NetworkChain;
+use crate::rpc;
 use crate::rpc::auth::AuthNewParams;
 use crate::rpc::beacon::BeaconGetEntry;
-use crate::rpc::eth::types::{EthAddress, EthBytes};
+use crate::rpc::eth::{
+    new_eth_tx_from_signed_message, types::*, BlockNumberOrHash, EthInt64, EthUint64, Predefined,
+};
 use crate::rpc::gas::GasEstimateGasLimit;
 use crate::rpc::miner::BlockTemplate;
 use crate::rpc::state::StateGetAllClaims;
 use crate::rpc::types::{ApiTipsetKey, MessageFilter, MessageLookup};
-use crate::rpc::{
-    self,
-    eth::{types::*, *},
-};
 use crate::rpc::{prelude::*, Permission};
 use crate::shim::actors::market;
 use crate::shim::actors::MarketActorStateLoad as _;
@@ -47,8 +46,9 @@ use serde::de::DeserializeOwned;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use similar::{ChangeTag, TextDiff};
-use std::io;
 use std::{
+    borrow::Cow,
+    io,
     path::{Path, PathBuf},
     str::FromStr,
     sync::Arc,
@@ -57,7 +57,6 @@ use std::{
 use tabled::{builder::Builder, settings::Style};
 use tokio::sync::Semaphore;
 use tracing::debug;
-use types::BlockNumberOrPredefined;
 
 const COLLECTION_SAMPLE_SIZE: usize = 5;
 
@@ -139,6 +138,10 @@ pub enum ApiCommands {
 
         #[command(flatten)]
         create_tests_args: CreateTestsArgs,
+
+        /// Specify a directory to which the RPC tests are dumped
+        #[arg(long)]
+        dump_dir: Option<PathBuf>,
     },
     DumpTests {
         #[command(flatten)]
@@ -191,6 +194,7 @@ impl ApiCommands {
                 run_ignored,
                 max_concurrent_requests,
                 create_tests_args,
+                dump_dir,
             } => {
                 let forest = Arc::new(rpc::Client::from_url(forest));
                 let lotus = Arc::new(rpc::Client::from_url(lotus));
@@ -208,6 +212,7 @@ impl ApiCommands {
                         filter.clone(),
                         run_ignored,
                         fail_fast,
+                        dump_dir.clone(),
                     )
                     .await?;
                 }
@@ -344,19 +349,28 @@ impl TestSummary {
 }
 
 /// Data about a failed test. Used for debugging.
+#[derive(Debug, Clone, Serialize, Deserialize)]
 struct TestDump {
     request: rpc::Request,
-    forest_response: Option<String>,
-    lotus_response: Option<String>,
+    forest_response: Result<serde_json::Value, String>,
+    lotus_response: Result<serde_json::Value, String>,
 }
 
 impl std::fmt::Display for TestDump {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         writeln!(f, "Request dump: {:?}", self.request)?;
         writeln!(f, "Request params JSON: {}", self.request.params)?;
-        if let (Some(forest_response), Some(lotus_response)) =
-            (&self.forest_response, &self.lotus_response)
-        {
+        let (forest_response, lotus_response) = (
+            self.forest_response
+                .as_ref()
+                .ok()
+                .and_then(|v| serde_json::to_string_pretty(v).ok()),
+            self.lotus_response
+                .as_ref()
+                .ok()
+                .and_then(|v| serde_json::to_string_pretty(v).ok()),
+        );
+        if let (Some(forest_response), Some(lotus_response)) = (&forest_response, &lotus_response) {
             let diff = TextDiff::from_lines(forest_response, lotus_response);
             let mut print_diff = Vec::new();
             for change in diff.iter_all_changes() {
@@ -371,10 +385,10 @@ impl std::fmt::Display for TestDump {
             writeln!(f, "Lotus response: {}", lotus_response)?;
             writeln!(f, "Diff: {}", print_diff.join("\n"))?;
         } else {
-            if let Some(forest_response) = &self.forest_response {
+            if let Some(forest_response) = &forest_response {
                 writeln!(f, "Forest response: {}", forest_response)?;
             }
-            if let Some(lotus_response) = &self.lotus_response {
+            if let Some(lotus_response) = &lotus_response {
                 writeln!(f, "Lotus response: {}", lotus_response)?;
             }
         };
@@ -499,19 +513,9 @@ impl RpcTest {
 
     async fn run(&self, forest: &rpc::Client, lotus: &rpc::Client) -> TestResult {
         let forest_resp = forest.call(self.request.clone()).await;
+        let forest_response = forest_resp.as_ref().map_err(|e| e.to_string()).cloned();
         let lotus_resp = lotus.call(self.request.clone()).await;
-
-        let forest_json_str = if let Ok(forest_resp) = forest_resp.as_ref() {
-            serde_json::to_string_pretty(forest_resp).ok()
-        } else {
-            None
-        };
-
-        let lotus_json_str = if let Ok(lotus_resp) = lotus_resp.as_ref() {
-            serde_json::to_string_pretty(lotus_resp).ok()
-        } else {
-            None
-        };
+        let lotus_response = lotus_resp.as_ref().map_err(|e| e.to_string()).cloned();
 
         let (forest_status, lotus_status) = match (forest_resp, lotus_resp) {
             (Ok(forest), Ok(lotus))
@@ -550,22 +554,14 @@ impl RpcTest {
             }
         };
 
-        if forest_status == TestSummary::Valid && lotus_status == TestSummary::Valid {
-            TestResult {
-                forest_status,
-                lotus_status,
-                test_dump: None,
-            }
-        } else {
-            TestResult {
-                forest_status,
-                lotus_status,
-                test_dump: Some(TestDump {
-                    request: self.request.clone(),
-                    forest_response: forest_json_str,
-                    lotus_response: lotus_json_str,
-                }),
-            }
+        TestResult {
+            forest_status,
+            lotus_status,
+            test_dump: Some(TestDump {
+                request: self.request.clone(),
+                forest_response,
+                lotus_response,
+            }),
         }
     }
 }
@@ -1769,7 +1765,7 @@ fn create_tests(
             eth_chain_id,
         )?);
     }
-    tests.sort_by_key(|test| test.request.method_name);
+    tests.sort_by_key(|test| test.request.method_name.clone());
     Ok(tests)
 }
 
@@ -1799,6 +1795,7 @@ async fn run_tests(
     filter: String,
     run_ignored: RunIgnored,
     fail_fast: bool,
+    dump_dir: Option<PathBuf>,
 ) -> anyhow::Result<()> {
     let forest = Into::<Arc<rpc::Client>>::into(forest);
     let lotus = Into::<Arc<rpc::Client>>::into(lotus);
@@ -1823,7 +1820,14 @@ async fn run_tests(
                  },
              ignore,
              ..
-         }| (*method_name, params.clone(), *api_paths, ignore.is_some()),
+         }| {
+            (
+                method_name.clone(),
+                params.clone(),
+                *api_paths,
+                ignore.is_some(),
+            )
+        },
     ) {
         // By default, do not run ignored tests.
         if matches!(run_ignored, RunIgnored::Default) && test.ignore.is_some() {
@@ -1834,7 +1838,7 @@ async fn run_tests(
             continue;
         }
 
-        if !filter_list.authorize(test.request.method_name) {
+        if !filter_list.authorize(&test.request.method_name) {
             continue;
         }
 
@@ -1877,15 +1881,34 @@ async fn run_tests(
             }
             _ => false,
         };
+
+        if let (Some(dump_dir), Some(test_dump)) = (&dump_dir, &test_result.test_dump) {
+            let dir = dump_dir.join(if success { "valid" } else { "invalid" });
+            if !dir.is_dir() {
+                std::fs::create_dir_all(&dir)?;
+            }
+            let filename = format!(
+                "{}_{}.json",
+                test_dump
+                    .request
+                    .method_name
+                    .as_ref()
+                    .replace(".", "_")
+                    .to_lowercase(),
+                chrono::Utc::now().timestamp_micros()
+            );
+            std::fs::write(dir.join(filename), serde_json::to_string_pretty(test_dump)?)?;
+        }
+
         let result_entry = (method_name, forest_status, lotus_status);
         if success {
             success_results
                 .entry(result_entry)
                 .and_modify(|v| *v += 1)
                 .or_insert(1u32);
         } else {
-            if let Some(test_result) = test_result.test_dump {
-                fail_details.push(test_result);
+            if let Some(test_dump) = test_result.test_dump {
+                fail_details.push(test_dump);
             }
             failed_results
                 .entry(result_entry)
@@ -1914,8 +1937,8 @@ fn print_error_details(fail_details: &[TestDump]) {
 }
 
 fn print_test_results(
-    success_results: &HashMap<(&'static str, TestSummary, TestSummary), u32>,
-    failed_results: &HashMap<(&'static str, TestSummary, TestSummary), u32>,
+    success_results: &HashMap<(Cow<'static, str>, TestSummary, TestSummary), u32>,
+    failed_results: &HashMap<(Cow<'static, str>, TestSummary, TestSummary), u32>,
 ) {
     // Combine all results
     let mut combined_results = success_results.clone();
@@ -1929,7 +1952,8 @@ fn print_test_results(
     println!("{}", format_as_markdown(&results));
 }
 
-fn format_as_markdown(results: &[((&'static str, TestSummary, TestSummary), u32)]) -> String {
+#[allow(clippy::type_complexity)]
+fn format_as_markdown(results: &[((Cow<'static, str>, TestSummary, TestSummary), u32)]) -> String {
     let mut builder = Builder::default();
 
     builder.push_record(["RPC Method", "Forest", "Lotus"]);
@@ -1973,7 +1997,8 @@ impl FilterList {
 
     /// Authorize (or not) an RPC method based on its name.
     /// If the allow list is empty, all methods are authorized, unless they are rejected.
-    fn authorize(&self, entry: &str) -> bool {
+    fn authorize(&self, entry: impl AsRef<str>) -> bool {
+        let entry = entry.as_ref();
         (self.allow.is_empty() || self.allow.iter().any(|a| entry.contains(a)))
             && !self.reject.iter().any(|r| entry.contains(r))
     }