src/tool/subcommands/api_cmd/api_compare_tests.rs (3)

1142-1161: Make comments future-proof; keep tests as identity checks

The test logic is solid. The comments asserting specific balances (e.g., “666 attoFIL each”) can go stale and aren’t enforced by the tests (which use identity Forest vs Lotus). Recommend softening them to avoid misleading future readers.

Apply this diff to update the comments:

-    let prefunded_wallets = [
-        // the following addresses should have 666 attoFIL each
+    let prefunded_wallets = [
+        // Pre-funded addresses for conformance testing (expected to have non-zero balances)
         Address::from_str("t0168923").unwrap(), // this is the ID address of the `t1w2zb5a723izlm4q3khclsjcnapfzxcfhvqyfoly` address
         Address::from_str("t1w2zb5a723izlm4q3khclsjcnapfzxcfhvqyfoly").unwrap(),
         Address::from_str("t2nfplhzpyeck5dcc4fokj5ar6nbs3mhbdmq6xu3q").unwrap(),
         Address::from_str("t3wmbvnabsj6x2uki33phgtqqemmunnttowpx3chklrchy76pv52g5ajnaqdypxoomq5ubfk65twl5ofvkhshq").unwrap(),
         Address::from_str("t410fx2cumi6pgaz64varl77xbuub54bgs3k5xsvn3ki").unwrap(),
-        // This address should have 0 FIL
+        // Address expected to have zero balance
         Address::from_str("t1qb2x5qctp34rxd7ucl327h5ru6aazj2heno7x5y").unwrap(),
     ];

---

1174-1193: Fix typo in comment and keep error-policy usage

The negative validate-address case and the case-insensitive policy are spot on. Small typo nit in the comment.

-            // Both Forest and Lotus should fail miserably at invocking Cthulhu's name
+            // Both Forest and Lotus should fail miserably at invoking Cthulhu's name

---

3086-3088: Prefer ASCII case-insensitive comparison without allocations

Use eq_ignore_ascii_case to avoid allocations from to_lowercase() and match typical ASCII-only error messages.

-                    reason_forest.to_lowercase() == reason_lotus.to_lowercase()
+                    reason_forest.eq_ignore_ascii_case(reason_lotus)

src/tool/subcommands/api_cmd/test_snapshot.rs (1)

109-109: Switching to inner error formatting aligns outputs across tools

Using e.inner().to_string() makes snapshot error strings originate from the JSON-RPC error object itself, reducing noise from ServerError’s Display. This is consistent with the new accessor and the intent in this PR.

If you want even tighter alignment with API-compare behavior (which compares only the message), consider serializing e.inner().message().to_string() instead. That would be a behavior change and require regenerating affected snapshots, so treat it as optional.

src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)

49-63: Log skipped failures when allow_failure is set

Currently, failures are silently skipped when allow_failure is true. A debug log will help diagnose why a snapshot was generated despite an RPC error.

-                match <$ty>::handle(ctx.clone(), params).await {
+                match <$ty>::handle(ctx.clone(), params).await {
                     Ok(result) => {
                         anyhow::ensure!(
                             allow_response_mismatch
                                 || test_dump.forest_response == Ok(result.into_lotus_json_value()?),
                             "Response mismatch between Forest and Lotus"
                         );
                     }
-                    Err(_) if allow_failure => {
-                        // If we allow failure, we do not check the error
-                    }
+                    Err(e) if allow_failure => {
+                        // If we allow failure, do not assert on the error, but record it for visibility.
+                        tracing::debug!("Allowing failure for {}: {}", <$ty>::NAME, e);
+                    }
                     Err(e) => {
                         bail!("Error running test {}: {}", <$ty>::NAME, e);
                     }
                 }

src/tool/subcommands/api_cmd.rs (2)

175-177: Add --allow-failure flag: good addition; drop redundant default to simplify

Clap treats boolean flags as false by default and sets them to true when present. The explicit default_value_t = false is unnecessary noise.

Apply this minimal cleanup:

-        /// Allow generating snapshot even if the test fails.
-        #[arg(long, default_value_t = false)]
-        allow_failure: bool,
+        /// Allow generating snapshot even if the test fails.
+        #[arg(long)]
+        allow_failure: bool,

Optional: consider mentioning this new flag in the subcommand’s top-level doc block (above Line 156) so users see it in the long description alongside --use-response-from.

---

278-279: Wiring and propagation of allow_failure look correct; consider avoiding positional bools

The flag is correctly destructured and forwarded to run_test_with_dump. However, two adjacent boolean parameters are easy to mix up when maintaining this code.

Short-term readability nit (low-risk and local change): add inline parameter comments at the call site to disambiguate intent.

                     match generate_test_snapshot::run_test_with_dump(
                         &test_dump,
                         tracking_db.clone(),
                         &chain,
-                        allow_response_mismatch,
-                        allow_failure,
+                        /* allow_response_mismatch: */ allow_response_mismatch,
+                        /* allow_failure: */ allow_failure,
                     )

Longer-term: replace the pair of bools with an options struct to make the API self-documenting and future-proof.

Example (outside this file):

// In generate_test_snapshot.rs
pub struct RunTestOptions {
    pub allow_response_mismatch: bool,
    pub allow_failure: bool,
}

// Signature:
pub async fn run_test_with_dump(
    dump: &TestDump,
    db: TrackingDb,
    chain: &NetworkChain,
    opts: RunTestOptions,
) -> anyhow::Result<()>;

And here:

generate_test_snapshot::run_test_with_dump(
    &test_dump,
    tracking_db.clone(),
    &chain,
    RunTestOptions {
        allow_response_mismatch,
        allow_failure,
    },
)
.await

Also applies to: 299-304

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.

src/tool/subcommands/api_cmd/api_compare_tests.rs (4)

3086-3088: Normalize error strings more robustly (ASCII + trim) before comparing

Using to_lowercase() can introduce Unicode case-mapping surprises and extra allocations. Given our error messages are ASCII, a cheaper and more predictable normalization is trim + to_ascii_lowercase.

Apply this diff:

-                PolicyOnRejected::PassWithIdenticalErrorCaseInsensitive => {
-                    reason_forest.to_lowercase() == reason_lotus.to_lowercase()
-                }
+                PolicyOnRejected::PassWithIdenticalErrorCaseInsensitive => {
+                    reason_forest.trim().to_ascii_lowercase()
+                        == reason_lotus.trim().to_ascii_lowercase()
+                }

---

1174-1193: Typo in comment (“invocking” → “invoking”) and good use of the new policy

The case-insensitive policy usage here is spot-on for the network-name message discrepancy. Minor nit: fix the comment spelling.

-            // Both Forest and Lotus should fail miserably at invocking Cthulhu's name
+            // Both Forest and Lotus should fail miserably at invoking Cthulhu's name

---

2982-3001: Dedup key ignores policy_on_rejected; can drop distinct tests unintentionally

Dedup uses (method_name, params, api_paths, ignore.is_some()). If we ever add two tests with the same request but different PolicyOnRejected, one will be discarded unintentionally. Including the enum discriminant in the key fixes this without exploding the key size.

Apply this diff:

-    for test in tests.into_iter().unique_by(
+    for test in tests.into_iter().unique_by(
         |RpcTest {
              request:
                  rpc::Request {
                      method_name,
                      params,
                      api_paths,
                      ..
                  },
              ignore,
              ..
          }| {
-            (
-                method_name.clone(),
-                params.clone(),
-                *api_paths,
-                ignore.is_some(),
-            )
+            (
+                method_name.clone(),
+                params.clone(),
+                *api_paths,
+                ignore.is_some(),
+                std::mem::discriminant(&test.policy_on_rejected),
+            )
         },
     ) {

---

1142-1161: Wallet balance tests: tighten semantics to catch unintended state changes

Great coverage across address kinds (ID, robust t1, t2, t3/BLS, t4/delegated, and an expected-zero account). A quick grep confirms these six test addresses only appear here (no other tests mutate them), so risk of cross-test interference is low.

To further reduce flakiness and explicitly encode intent, switch from RpcTest::identity to RpcTest::validate, checking both cross-node equality and the expected sign:

• src/tool/subcommands/api_cmd/api_compare_tests.rs: lines 1142–1148 (prefunded wallets)
• src/tool/subcommands/api_cmd/api_compare_tests.rs: line 1150 (expected-zero wallet)

Example diff (assuming TokenAmount::is_positive and TokenAmount::is_zero exist):

-    for wallet in prefunded_wallets {
-        tests.push(RpcTest::identity(
-            WalletBalance::request((wallet,)).unwrap(),
-        ));
+    for wallet in prefunded_wallets {
+        tests.push(RpcTest::validate(
+            WalletBalance::request((wallet,)).unwrap(),
+            |forest, lotus| forest == lotus && forest.is_positive(), // >0 for prefunded
+        ));
     }
 
-    // expected-zero
-    tests.push(RpcTest::identity(
-        WalletBalance::request((zero_wallet,)).unwrap(),
-    ));
+    // expected-zero
+    tests.push(RpcTest::validate(
+        WalletBalance::request((zero_wallet,)).unwrap(),
+        |forest, lotus| forest == lotus && forest.is_zero(),
+    ));

If those helpers aren’t available, compare against TokenAmount::zero() or BigInt::from(0) directly. Happy to help wire up a follow-up patch.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.

src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

200-208: Nice addition: case-insensitive error policy variant

The new PolicyOnRejected::PassWithIdenticalErrorCaseInsensitive fills a real gap where only casing differs between Lotus and Forest error reasons. The wiring through evaluate_test_success looks correct.