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

231-246: Optional: parse strongly-typed args at the clap boundary to centralize validation

Let clap parse Address/EthHash/hex bytes for you to reduce error-prone String plumbing and push validation to the boundary. This keeps run() lean and avoids duplicated parsing logic.

Minimal change (no external deps), using a small parser for hex:

-        /// Test Transaction `to` address
-        #[arg(long)]
-        to: String,
-        /// Test Transaction `from` address
-        #[arg(long)]
-        from: String,
-        /// Test Transaction hex `payload`
-        #[arg(long)]
-        payload: String,
-        /// Log `topic` to search for
-        #[arg(long)]
-        topic: String,
+        /// Test Transaction `to` address
+        #[arg(long, value_parser = clap::builder::ValueParser::new(Address::from_str))]
+        to: Address,
+        /// Test Transaction `from` address
+        #[arg(long, value_parser = clap::builder::ValueParser::new(Address::from_str))]
+        from: Address,
+        /// Test Transaction hex `payload` (accepts optional 0x prefix)
+        #[arg(long, value_parser = parse_hex_bytes)]
+        payload: Vec<u8>,
+        /// Log `topic` to search for (0x‑prefixed 32‑byte hash)
+        #[arg(long, value_parser = clap::builder::ValueParser::new(EthHash::from_str))]
+        topic: EthHash,

Add this helper in the same file (top-level):

fn parse_hex_bytes(s: &str) -> Result<Vec<u8>, String> {
    let clean = s.strip_prefix("0x").unwrap_or(s);
    hex::decode(clean).map_err(|e| format!("invalid hex: {s} ({e})"))
}

With this, the run() arm can drop all manual parsing lines and directly construct TestTransaction.

---

395-417: Accept “0x”-prefixed payloads and add parsing context to improve UX

hex::decode(payload) will reject the commonly used 0x-prefixed form; address/topic errors also lack context. Strip an optional 0x and wrap parses with with_context. Also trim filter to avoid surprises from trailing spaces.

             } => {
-                let client = Arc::new(rpc::Client::default_or_from_env(None)?);
+                let client = Arc::new(
+                    rpc::Client::default_or_from_env(None)
+                        .with_context(|| "could not initialize RPC client; check FULLNODE_API_INFO or that a node is running on 127.0.0.1:2345")?
+                );
 
-                let to = Address::from_str(&to)?;
-                let from = Address::from_str(&from)?;
-                let payload = hex::decode(payload)?;
-                let topic = EthHash::from_str(&topic)?;
+                let to = Address::from_str(&to)
+                    .with_context(|| format!("invalid --to address: {to}"))?;
+                let from = Address::from_str(&from)
+                    .with_context(|| format!("invalid --from address: {from}"))?;
+                let payload = {
+                    let clean = payload.strip_prefix("0x").unwrap_or(&payload);
+                    hex::decode(clean)
+                        .with_context(|| format!("invalid --payload hex: {payload}"))?
+                };
+                let topic = EthHash::from_str(&topic)
+                    .with_context(|| format!("invalid --topic (expect 0x-prefixed 32-byte hash): {topic}"))?;
                 let tx = TestTransaction {
                     to,
                     from,
                     payload,
                     topic,
                 };
 
-                let tests = stateful_tests::create_tests(tx).await;
-                stateful_tests::run_tests(tests, client, filter).await?;
+                let tests = stateful_tests::create_tests(tx).await;
+                let filter = filter.trim().to_string();
+                stateful_tests::run_tests(tests, client, filter).await?;
             }

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

208-246: Tighten CLI help: document address/hex formats, FULLNODE_API_INFO, and filter semantics

Current help omits crucial format guidance and environment usage. Clarify that:

• to/from must be Filecoin addresses (f/t or delegated f4).
• payload is hex and may include an optional 0x prefix.
• topic is a 32‑byte Keccak hash (0x… with 64 hex nibbles).
• connection is taken from FULLNODE_API_INFO (or defaults to 127.0.0.1:2345).
• filter is case‑sensitive and matches by method-name prefix.

Apply this doc-only diff:

-    /// Run multiple stateful JSON-RPC API tests against a Filecoin node.
+    /// Run a suite of stateful JSON-RPC API tests against a Filecoin node.
     ///
-    /// Some tests require sending a transaction to trigger events; the provided
-    /// `from`, `to`, `payload`, and `topic` inputs are used for those cases.
+    /// Connection:
+    /// - Uses `FULLNODE_API_INFO` if set (e.g. `/ip4/127.0.0.1/tcp/2345/http` or a multiaddr with token).
+    ///   Falls back to `http://127.0.0.1:2345/`.
+    ///
+    /// Inputs:
+    /// - `--from`, `--to`: Filecoin addresses (`f…`/`t…`, including delegated `f4`).
+    /// - `--payload`: hex data; both `deadbeef` and `0xdeadbeef` are accepted.
+    /// - `--topic`: 32-byte event topic (Keccak-256), 0x-prefixed 64-nybble hex.
     ///
-    /// Useful for verifying methods like `eth_newFilter`, `eth_getFilterLogs`, and others
-    /// that rely on internal state.
+    /// Useful for verifying methods like `eth_newFilter`, `eth_getFilterLogs`, and others that rely on internal state.
     ///
-    /// Use `--filter` to run only tests that interact with a specific RPC method.
+    /// Use `--filter` to run only tests that interact with a specific RPC method.
+    /// The match is case-sensitive and by method-name prefix (e.g. `--filter eth_new`).
     ///
-    /// Example output:
-    /// ```text
+    /// Example (Lotus; all pass):
+    /// ```console
     /// running 7 tests
     /// test eth_newFilter install/uninstall ... ok
     /// test eth_newFilter under limit ... ok
     /// test eth_newFilter just under limit ... ok
     /// test eth_newFilter over limit ... ok
     /// test eth_newBlockFilter works ... ok
     /// test eth_newPendingTransactionFilter works ... ok
     /// test eth_getFilterLogs works ... ok
     /// test result: ok. 7 passed; 0 failed; 0 ignored; 0 filtered out
     /// ```
+    /// Example (Forest; some tests currently ignored/pending fixes):
+    /// ```console
+    /// running 7 tests
+    /// test eth_newFilter install/uninstall ... ok
+    /// test eth_newFilter under limit ... ok
+    /// test eth_newFilter just under limit ... ok
+    /// test eth_newFilter over limit ... ignored
+    /// test eth_newBlockFilter works ... ok
+    /// test eth_newPendingTransactionFilter works ... ignored
+    /// test eth_getFilterLogs works ... ignored
+    /// test result: ok. 4 passed; 0 failed; 3 ignored; 0 filtered out
+    /// ```

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.

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

236-252: Prefer strong typing at the clap boundary for --to, --from, --topic

Let clap parse Address and EthHash via FromStr to fail fast with better help and remove ad-hoc parsing later. Keep payload as a string (we still need custom 0x handling). This mirrors prior feedback.

Apply this refactor (fields only):

-        /// Test Transaction `to` address
-        #[arg(long)]
-        to: String,
-        /// Test Transaction `from` address
-        #[arg(long)]
-        from: String,
+        /// Test Transaction `to` address (delegated f4)
+        #[arg(long)]
+        to: Address,
+        /// Test Transaction `from` address (delegated f4)
+        #[arg(long)]
+        from: Address,
@@
-        /// Log `topic` to search for
-        #[arg(long)]
-        topic: String,
+        /// Log `topic` (32‑byte Keccak hash) to search for
+        #[arg(long)]
+        topic: EthHash,

If you adopt this, the manual Address::from_str/EthHash::from_str calls in the match arm can be dropped (see follow-up diff in Lines 401‑423). Also, you can remove the unused use std::str::FromStr; import (Line 30).

---

401-423: Add user-friendly parse context, accept 0x‑prefixed payloads, and improve FULLNODE_API_INFO error

• Wrap default_or_from_env with context to guide users when the env var is missing/invalid.
• Accept “0x…” payloads by stripping the prefix before hex::decode.
• Add with_context on parse errors for clearer CLI UX.
• Pass client directly (already done; no clone needed).

Minimal, backward-compatible patch:

             } => {
-                let client = Arc::new(rpc::Client::default_or_from_env(None)?);
+                let client = Arc::new(
+                    rpc::Client::default_or_from_env(None)
+                        .with_context(|| "failed to initialize RPC client (set FULLNODE_API_INFO or ensure a node is listening on http://127.0.0.1:2345/)")?
+                );
 
-                let to = Address::from_str(&to)?;
-                let from = Address::from_str(&from)?;
-                let payload = hex::decode(payload)?;
-                let topic = EthHash::from_str(&topic)?;
+                let to = Address::from_str(&to)
+                    .with_context(|| format!("invalid --to address: {to}"))?;
+                let from = Address::from_str(&from)
+                    .with_context(|| format!("invalid --from address: {from}"))?;
+                let payload = {
+                    let clean = payload.strip_prefix("0x").unwrap_or(&payload);
+                    hex::decode(clean)
+                        .with_context(|| format!("invalid --payload hex: {payload}"))?
+                };
+                let topic = EthHash::from_str(&topic)
+                    .with_context(|| format!("invalid --topic (expect 0x‑prefixed 32‑byte hash): {topic}"))?;
 
                 let tx = TestTransaction {
                     to,
                     from,
                     payload,
                     topic,
                 };
 
                 let tests = stateful_tests::create_tests(tx).await;
                 stateful_tests::run_tests(tests, client, filter).await?;
             }

If you adopt typed clap fields (Lines 236‑252), simplify this arm further:

-                let to = Address::from_str(&to)
-                    .with_context(|| format!("invalid --to address: {to}"))?;
-                let from = Address::from_str(&from)
-                    .with_context(|| format!("invalid --from address: {from}"))?;
-                let payload = {
+                let payload = {
                     let clean = payload.strip_prefix("0x").unwrap_or(&payload);
                     hex::decode(clean)
                         .with_context(|| format!("invalid --payload hex: {payload}"))?
                 };
-                let topic = EthHash::from_str(&topic)
-                    .with_context(|| format!("invalid --topic (expect 0x‑prefixed 32‑byte hash): {topic}"))?;
 
                 let tx = TestTransaction {
-                    to,
-                    from,
+                    to,   // already Address
+                    from, // already Address
                     payload,
-                    topic,
+                    topic, // already EthHash
                 };

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

208-235: Tighten docs: clarify connection fallback, input formats, and filter semantics

• Mention the default fallback when FULLNODE_API_INFO is not set (http://127.0.0.1:2345/).
• Call out that hex inputs commonly include a “0x” prefix and whether it’s accepted.
• Clarify that --filter does a prefix match (starts_with), not substring.
• The “all ok (7 passed)” example doesn’t reflect current ignores/expected failures; either add a disclaimer or show a realistic mixed/ignored output.

Apply this doc-only diff:

-    /// Connection: uses `FULLNODE_API_INFO` from the environment.
+    /// Connection: uses `FULLNODE_API_INFO` from the environment; falls back to http://127.0.0.1:2345/.
@@
-    /// - `--payload`: calldata in hex
-    /// - `--topic`: `32‑byte` event topic in hex
-    /// - `--filter`: run only tests that interact with a specific RPC method
+    /// - `--payload`: calldata in hex (accept “0x” prefix or raw hex)
+    /// - `--topic`: 32‑byte event topic in hex (e.g., 0x…)
+    /// - `--filter`: run only tests whose method name starts with this string (case sensitive)
@@
-    /// Example output:
+    /// Example output (results may include ignored or expected failures, depending on node):
@@
-    /// test eth_newFilter over limit ... ok
+    /// test eth_newFilter over limit ... ignored
@@
-    /// test eth_newPendingTransactionFilter works ... ok
-    /// test eth_getFilterLogs works ... ok
-    /// test result: ok. 7 passed; 0 failed; 0 ignored; 0 filtered out
+    /// test eth_newPendingTransactionFilter works ... ignored
+    /// test eth_getFilterLogs works ... ignored
+    /// test result: ok. 4 passed; 0 failed; 3 ignored; 0 filtered out

---

30-30: use std::str::FromStr; likely unnecessary if clap parses typed args

If you move to typed clap fields for to, from, and topic, this import can be removed.

-use std::str::FromStr;

---

208-252: Optional: make payload optional to ease running filter-only tests

Right now users must provide --payload even when running, e.g., --filter EthNewBlockFilter, which doesn’t require tx data. Consider Option<String> and validate only when a test needs it. Happy to draft if desired.

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.

scripts/tests/harness.sh (2)

127-135: Backfill path should wait for full healthcheck readiness to avoid flaky stateful tests

Per team learnings, when backfilling you should wait for “healthcheck ready” (not just API up/sync) before running tests that depend on completed DB backfill.

Add this guard near the end of forest_init after sync (outside the changed hunk, shown here as a plain snippet):

# After forest_wait_for_sync
if [[ "${1:-}" == "--backfill-db" ]]; then
  forest_wait_for_healthcheck_ready
fi

---

143-147: Fail fast on missing token, trim newline, and avoid overriding FULLNODE_API_INFO unless necessary

Right now, if the token file is missing/empty, you’ll silently export an auth-less FULLNODE_API_INFO and the client will not fall back to the default token loader, causing 401s. Also, most token files end with a newline, which will break auth unless trimmed.

Apply this diff to make the block robust and cwd-agnostic while keeping the env override opt-in:

-  DATA_DIR=$( $FOREST_CLI_PATH config dump | grep "data_dir" | cut -d' ' -f3- | tr -d '"' )
-  ADMIN_TOKEN=$(cat "${DATA_DIR}/token")
-  FULLNODE_API_INFO="${ADMIN_TOKEN}:/ip4/127.0.0.1/tcp/2345/http"
-
-  export FULLNODE_API_INFO
+  # Prefer default token path resolution in forest-tool; only export FULLNODE_API_INFO if not already provided.
+  if [[ -z "${FULLNODE_API_INFO:-}" ]]; then
+    DATA_DIR=$($FOREST_CLI_PATH config dump | grep "data_dir" | cut -d' ' -f3- | tr -d '"')
+    TOKEN_FILE="${DATA_DIR}/token"
+    if [[ ! -s "$TOKEN_FILE" ]]; then
+      echo "ERROR: RPC token file missing or empty at: $TOKEN_FILE" >&2
+      return 1
+    fi
+    # Trim trailing newline to avoid an invalid password
+    ADMIN_TOKEN="$(tr -d '\n' < "$TOKEN_FILE")"
+    export FULLNODE_API_INFO="${ADMIN_TOKEN}:/ip4/127.0.0.1/tcp/2345/http"
+  fi

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

207-251: Tighten docs: connection fallback and filter semantics

• Clarify that connection falls back to 127.0.0.1:2345 + default token file when FULLNODE_API_INFO is not set (matches rpc::Client::default_or_from_env).
• Be explicit that filter uses a case-sensitive prefix match (matches run_tests).

Apply this doc tweak:

-    /// Connection: uses `FULLNODE_API_INFO` from the environment.
+    /// Connection: uses `FULLNODE_API_INFO` from the environment, or falls back to
+    /// `http://127.0.0.1:2345` with the default token file if unset.
@@
-    /// - `--topic`: `32‑byte` event topic in hex
-    /// - `--filter`: run only tests that interact with a specific RPC method
+    /// - `--topic`: `32‑byte` event topic in hex (expects 0x‑prefixed)
+    /// - `--filter`: run only tests whose method name starts with this string (prefix, case sensitive)

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: hanabi1224
PR: ChainSafe/forest#5886
File: scripts/tests/calibnet_eth_mapping_check.sh:49-49
Timestamp: 2025-08-07T09:37:03.079Z
Learning: In Forest project scripts, `forest_wait_api` (called within `forest_init`) ensures basic RPC service readiness, while `forest_wait_for_healthcheck_ready` performs additional comprehensive checks including DB backfill completion. When using `--backfill-db` flag, basic RPC operations can proceed after `forest_init`, but operations requiring complete DB backfill should wait for `forest_wait_for_healthcheck_ready`.