Skip to content

Add forest api test-stateful subcommand #5836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 90 commits into
base: main
Choose a base branch
from

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented Jul 15, 2025

Summary of changes

Changes introduced in this pull request:

  • Add api test-stateful subcommand to forest-tool

Usage example:

Lotus:

$ ./forest-tool api test-stateful <lotus-multi-addr> --to t410f2jhqlciub25ad3immo5kug2fluj625xiex6lbyi --from t410f5uudc3yoiodsva73rxyx5sxeiaadpaplsu6mofy --payload 40c10f19000000000000000000000000ed28316f0e43872a83fb8df17ecae440003781eb00000000000000000000000000000000000000000000000006f05b59d3b20000 --topic 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef
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

Forest:

$ ./forest-tool api test-stateful <forest-multi-addr> --to t410f2jhqlciub25ad3immo5kug2fluj625xiex6lbyi --from t410f5uudc3yoiodsva73rxyx5sxeiaadpaplsu6mofy --payload 40c10f19000000000000000000000000ed28316f0e43872a83fb8df17ecae440003781eb00000000000000000000000000000000000000000000000006f05b59d3b20000 --topic 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef
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 ... FAILED
test eth_newBlockFilter works ... FAILED
test eth_newPendingTransactionFilter works ... FAILED
test eth_getFilterLogs works ... FAILED
test result: FAILED. 3 passed; 4 failed; 0 ignored; 0 filtered out

Reference issue to close (if applicable)

Closes #5793

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • New Features

    • Added a CLI subcommand to run stateful RPC tests against a live node.
    • Expanded public API for Ethereum transaction utilities and uint formatting.
  • Bug Fixes

    • JSON-RPC notifications now embed proper JSON values instead of stringified JSON.
  • Refactor

    • Unified filter results to return hashes; responses may differ from prior block/tx variants.
  • Documentation

    • New guide: running and extending RPC stateful tests.
  • Tests

    • Introduced stateful RPC test harness and a Calibnet CI job.
  • Chores

    • Added WebSocket dependency for subscription-based flows.

Copy link
Contributor

coderabbitai bot commented Jul 15, 2025

Walkthrough

Adds a stateful RPC test harness and CLI subcommand to run filter-related Ethereum JSON-RPC tests against a running node, introduces CI integration and scripts to execute these tests, adjusts filter result types to a unified Hashes variant, exposes transaction module/methods, tweaks JSON-RPC notification serialization, and adds a WebSocket dependency.

Changes

Cohort / File(s) Summary
Docs: Stateful RPC tests
documentation/src/developer_documentation/rpc_stateful_tests.md
New doc describing prerequisites, usage, and extension of stateful RPC tests for Ethereum filter/subscription methods.
Eth transaction module exposure
src/eth/mod.rs, src/eth/transaction.rs
Made transaction module public; added Debug derive to EthTx; made EthTx::rlp_signed_message public.
Filter result refactor
src/rpc/methods/eth/types.rs
Replaced Blocks/Txs with unified Hashes(Vec<EthHash>) in EthFilterResult; updated equality and emptiness logic.
Filter result producers updated
src/rpc/methods/eth.rs
Added EthUint64::to_hex_string; changed filter helpers to return EthFilterResult::Hashes instead of Txs.
JSON-RPC notifications serialization
src/rpc/channel.rs
Switched notification result serialization from JSON string (to_string) to JSON value (to_value).
CLI: Stateful test harness
src/tool/subcommands/api_cmd.rs, src/tool/subcommands/api_cmd/stateful_tests.rs
Added api test-stateful subcommand; introduced test framework, scenarios (newFilter, newBlockFilter, newPendingTxFilter, getFilterLogs, limit tests), runners, and helpers (WS subscribe, mempool wait, filter ops).
CI for stateful tests
.github/workflows/forest.yml, scripts/tests/calibnet_api_test_stateful_check.sh, scripts/tests/harness.sh
New workflow job to run calibnet stateful API tests; new script to execute tests with preloaded wallet; harness exports admin token and FULLNODE_API_INFO.
Dependency
Cargo.toml
Added tokio-tungstenite = "0.27.0".

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant CI as CI Job
  participant Tool as forest-tool (api test-stateful)
  participant Tests as Stateful Test Harness
  participant RPC as rpc::Client (HTTP)
  participant WS as WebSocket (tokio-tungstenite)
  participant Node as Forest Node

  Dev->>CI: Push/PR triggers workflow
  CI->>Node: Start/initialize via harness (admin token)
  CI->>Tool: forest-tool api test-stateful --addr ... --to ... --from ... --payload ... --topic ...
  Tool->>RPC: Build client with FULLNODE_API_INFO
  Tool->>Tests: create_tests(TestTransaction)
  Tool->>Tests: run_tests(tests, client, filter)

  rect rgba(200,230,255,0.3)
    note over Tests,RPC: Install filters
    Tests->>RPC: eth_newFilter / eth_newBlockFilter / eth_newPendingTransactionFilter
    RPC->>Node: Create filters
    Node-->>RPC: Filter IDs
    RPC-->>Tests: IDs
  end

  rect rgba(220,255,220,0.3)
    note over Tests,WS: Subscribe/poll
    Tests->>WS: eth_subscribe newHeads (ws)
    WS->>Node: Subscribe newHeads
    Node-->>WS: Tipset notifications
    Tests->>RPC: eth_getFilterChanges / eth_getFilterLogs
    RPC->>Node: Query results
    Node-->>RPC: EthFilterResult::Hashes / Logs
    RPC-->>Tests: Results
  end

  rect rgba(255,235,200,0.35)
    note over Tests,Node: Stimulate pending tx/logs
    Tests->>Node: Trigger tx via mempool wait helpers (external activity assumed)
    Node-->>RPC: Updated changes/logs on poll
  end

  Tests-->>Tool: Pass/Fail per scenario
  Tool-->>CI: Exit with status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Assessment against linked issues

Objective Addressed Explanation
Implement tests for filter methods: eth_newFilter/eth_uninstallFilter; eth_newBlockFilter; eth_newPendingTransactionFilter; eth_getFilterLogs; eth_getFilterChanges (#5793)
Enforce/test practical limit and verify max results limit (e.g., FOREST_MAX_FILTER_RESULTS) (#5793) Limit-related tests are mentioned but specific max-results verification is not clearly present.
Test behavior with many concurrently installed filters (~100) and performance comparability to Lotus (#5793) No explicit scenarios for high-concurrency/performance testing are evident.
Test filter expiration after timeout (~5 minutes) leading to “filter not found” on getFilterChanges (#5793) No explicit timeout/expiration scenario is described.
Ensure scripts work against Lotus for cross-validation (#5793) Docs note Lotus/Forest prerequisites, but cross-run compatibility is not validated in code.

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
Unify EthFilterResult variants: remove Blocks/Txs; add Hashes (src/rpc/methods/eth/types.rs) This alters API surface/semantics; the issue requests adding tests, not changing filter result types.
Change filter helpers to return Hashes instead of Txs (src/rpc/methods/eth.rs) Functional behavior change tied to API result variant, beyond test addition.
JSON-RPC notification payload serialization from string to value (src/rpc/channel.rs) Notification formatting change is unrelated to adding filter tests.
Make eth::transaction module and EthTx::rlp_signed_message public (src/eth/mod.rs, src/eth/transaction.rs) Public API exposure isn’t required by the testing objectives.

Possibly related PRs

Suggested reviewers

  • akaladarshi
  • LesnyRumcajs

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch elmattic/api-run-subcommand

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd.rs (1)

205-246: Tidy help text; add usage example and a default client address for ergonomics.

The doc-comments are helpful and address the previously raised need for help text. To improve UX:

  • Provide a concrete usage example (not only output).
  • Give addr a default to match other subcommands, and name the value in help for clarity.

Also, consider clarifying accepted address formats for --to/--from (Filecoin f/t addresses vs 0x Ethereum addresses) to set expectations.

Apply this diff to enhance help and defaults:

@@
-    /// 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 (prefix match).
     ///
+    /// Example usage:
+    /// forest-tool api test-stateful /ip4/127.0.0.1/tcp/2345/http \
+    ///   --to f410f... --from f410f... \
+    ///   --payload 0x<hex-call-data> \
+    ///   --topic 0x<32-byte-topic>
+    ///
     /// Example output:
@@
-        /// Client address
-        addr: UrlFromMultiAddr,
+        /// Client address
+        #[arg(value_name = "ADDR", default_value = "/ip4/127.0.0.1/tcp/2345/http")]
+        addr: UrlFromMultiAddr,
@@
-        /// Test Transaction `to` address
+        /// Test Transaction `to` address (Filecoin f/t or delegated EVM)
         #[arg(long)]
         to: String,
@@
-        /// Test Transaction `from` address
+        /// Test Transaction `from` address (Filecoin f/t or delegated EVM)
         #[arg(long)]
         from: String,
@@
-        /// Test Transaction hex `payload`
+        /// Test Transaction hex `payload` (0x-prefixed or raw hex)
         #[arg(long)]
         payload: String,
@@
-        /// Log `topic` to search for
+        /// Log `topic` to search for (0x-prefixed 32-byte keccak hash)
         #[arg(long)]
         topic: String,
📜 Review details

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 248fa27 and cd831aa.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd.rs (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/tool/subcommands/api_cmd.rs (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (2)
  • create_tests (601-650)
  • run_tests (91-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: All lint checks
  • GitHub Check: Check
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (2)
src/tool/subcommands/api_cmd.rs (2)

5-5: Good modularization; isolates stateful test logic.

Pulling stateful test logic into a dedicated module keeps this file focused on CLI wiring.


21-21: Imports look correct and minimal.

Bringing TestTransaction and FromStr in here is appropriate and keeps parsing local to the command handler.

Also applies to: 30-30

Comment on lines +27 to +30
TO_ADDRESS="t410fp6e7drelxau7nf76tcn6gva22t5jafefhevubwi"
FROM_ADDRESS="t410f2avianksmit2cl2bqk53qant7nm7rdmk63twa5y"
PAYLOAD="40c10f190000000000000000000000007f89f1c48bb829f697fe989be3541ad4fa9014850000000000000000000000000000000000000000000000000de0b6b3a7640000"
TOPIC="0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to add some comments; what is this payload and topic?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (1)

553-556: Fix: Potential u64 underflow for from_block.

block_num.0 - BLOCK_RANGE underflows at height 0. Clamp using saturating_sub for correctness and consistency with earlier usage.

-            let filter_spec = EthFilterSpec {
-                from_block: Some(format!("0x{:x}", block_num.0 - BLOCK_RANGE)),
+            let filter_spec = EthFilterSpec {
+                from_block: Some(EthUint64(block_num.0.saturating_sub(BLOCK_RANGE)).to_hex_string()),
                 to_block: Some(block_num.to_hex_string()),
                 topics: Some(topics),
                 ..Default::default()
             };
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (1)

267-274: Be defensive on WS close/error: close stream even if subscription wasn’t opened.

If the stream errors or closes before channel_id is set, the current code bails without attempting to close the socket. Close the WS stream regardless; best-effort close of channel only if present.

-            Err(..) | Ok(WsMessage::Close(..)) => {
-                let id = channel_id
-                    .as_ref()
-                    .ok_or_else(|| anyhow::anyhow!("subscription not opened"))?;
-                close_channel(&mut ws_stream, id).await?;
-                ws_stream.close(None).await?;
-                anyhow::bail!("unexpected error or close message");
-            }
+            Err(..) | Ok(WsMessage::Close(..)) => {
+                if let Some(id) = channel_id.as_ref() {
+                    let _ = close_channel(&mut ws_stream, id).await;
+                }
+                let _ = ws_stream.close(None).await;
+                anyhow::bail!("unexpected error or close message");
+            }
📜 Review details

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.

📥 Commits

Reviewing files that changed from the base of the PR and between e8ac160 and 4146221.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/api_run_tests.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
PR: ChainSafe/forest#5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.

Applied to files:

  • src/tool/subcommands/api_cmd/api_run_tests.rs
📚 Learning: 2025-08-11T13:50:04.489Z
Learnt from: elmattic
PR: ChainSafe/forest#5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:211-268
Timestamp: 2025-08-11T13:50:04.489Z
Learning: In Filecoin WebSocket channels (used for methods like `Filecoin.ChainNotify`), the protocol only uses Text messages for communication. Non-Text WebSocket message types (Binary, Ping, Pong, etc.) can be safely ignored when processing Filecoin channel notifications.

Applied to files:

  • src/tool/subcommands/api_cmd/api_run_tests.rs
🧬 Code Graph Analysis (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (3)
src/tool/subcommands/api_cmd.rs (1)
  • run (249-461)
src/rpc/client.rs (3)
  • new (209-248)
  • client (114-114)
  • request (272-285)
src/rpc/channel.rs (3)
  • default (291-343)
  • channel_id (169-171)
  • channel_id (205-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: Check
  • GitHub Check: All lint checks
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (1)

599-645: Nice: Clear, traceable test matrix with method attribution and ignores linked to issues.

The scenarios are well organized, use the with_methods! helper effectively, and the ignores reference concrete issues. Good balance of expected-failure coverage and cleanup.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
src/tool/subcommands/api_cmd/api_run_tests.rs (3)

154-160: Invert the final ensure: suite currently fails on success and succeeds on failure

ensure!(failed > 0, …) only errors when there are zero failures. It should error when there are any failures.

     let status = if failed == 0 { "ok" } else { "FAILED" };
     println!(
         "test result: {status}. {passed} passed; {failed} failed; {ignored} ignored; {filtered} filtered out"
     );
-    ensure!(failed > 0, "{failed} test(s) failed");
+    ensure!(failed == 0, "{failed} test(s) failed");
     Ok(())

554-559: Prevent u64 underflow when computing from_block

block_num.0 - BLOCK_RANGE can underflow (e.g., at genesis), yielding a huge hex number and breaking the filter.

-            let filter_spec = EthFilterSpec {
-                from_block: Some(format!("0x{:x}", block_num.0 - BLOCK_RANGE)),
+            let from = block_num.0.saturating_sub(BLOCK_RANGE);
+            let filter_spec = EthFilterSpec {
+                from_block: Some(format!("0x{:x}", from)),
                 to_block: Some(block_num.to_hex_string()),
                 topics: Some(topics),
                 ..Default::default()
             };

285-300: Fix inverted retry check and too-aggressive timeout in wait_pending_message

  • The ensure!(retries == 0, …) condition is inverted; it bails immediately on the first loop when the message isn’t present yet.
  • Total wait ≈ 1s with 10ms sleeps is too tight and will be flaky in CI.

Use a deadline-based loop with a reasonable timeout.

-async fn wait_pending_message(client: &rpc::Client, message_cid: Cid) -> anyhow::Result<()> {
-    let mut retries = 100;
-    loop {
-        let pending = client
-            .call(MpoolPending::request((ApiTipsetKey(None),))?)
-            .await?;
-
-        if pending.0.iter().any(|msg| msg.cid() == message_cid) {
-            break Ok(());
-        }
-        ensure!(retries == 0, "Message not found in mpool");
-        retries -= 1;
-
-        tokio::time::sleep(Duration::from_millis(10)).await;
-    }
-}
+async fn wait_pending_message(client: &rpc::Client, message_cid: Cid) -> anyhow::Result<()> {
+    let deadline = std::time::Instant::now() + Duration::from_secs(10);
+    loop {
+        let pending = client
+            .call(MpoolPending::request((ApiTipsetKey(None),))?)
+            .await?;
+
+        if pending.0.iter().any(|msg| msg.cid() == message_cid) {
+            return Ok(());
+        }
+        if std::time::Instant::now() >= deadline {
+            anyhow::bail!("Message not found in mpool within timeout");
+        }
+        tokio::time::sleep(Duration::from_millis(200)).await;
+    }
+}
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (1)

162-163: Remove unnecessary #[allow(unreachable_code)]

You already guard the tail with unreachable!(). The attribute is no longer needed.

-#[allow(unreachable_code)]
 async fn next_tipset(client: &rpc::Client) -> anyhow::Result<()> {
 ...
-    unreachable!("loop always returns within the branches above")
+    unreachable!("loop always returns within the branches above")
 }

Also applies to: 282-283

📜 Review details

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4146221 and 7b2cdeb.

📒 Files selected for processing (1)
  • src/tool/subcommands/api_cmd/api_run_tests.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
PR: ChainSafe/forest#5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.

Applied to files:

  • src/tool/subcommands/api_cmd/api_run_tests.rs
📚 Learning: 2025-08-11T13:50:04.489Z
Learnt from: elmattic
PR: ChainSafe/forest#5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:211-268
Timestamp: 2025-08-11T13:50:04.489Z
Learning: In Filecoin WebSocket channels (used for methods like `Filecoin.ChainNotify`), the protocol only uses Text messages for communication. Non-Text WebSocket message types (Binary, Ping, Pong, etc.) can be safely ignored when processing Filecoin channel notifications.

Applied to files:

  • src/tool/subcommands/api_cmd/api_run_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Check
  • GitHub Check: All lint checks
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (rust)
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/api_run_tests.rs (1)

598-647: Nice coverage and organization of scenarios

Good breadth across filter install/uninstall, limits, block/pending filters, and logs, with clear naming and method registration via the with_methods! macro. Ignored tests reference tracking issues, which is great for CI hygiene.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd.rs (1)

392-415: Accept 0x-prefixed payloads, add error context, and avoid unnecessary clone

Without stripping a leading 0x, hex::decode will fail for typical Ethereum-style input. Also, wrap parses with with_context for actionable errors and pass client by move (run_tests accepts Into<Arc<_>>).

Apply this diff:

             Self::TestStateful {
                 addr: UrlFromMultiAddr(url),
                 to,
                 from,
                 payload,
                 topic,
                 filter,
             } => {
                 let client = Arc::new(rpc::Client::from_url(url));

-                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.clone(), filter).await?;
+                stateful_tests::run_tests(tests, client, filter).await?;
             }
🧹 Nitpick comments (4)
src/tool/subcommands/api_cmd.rs (2)

205-246: Great help text, but clarify accepted address formats and hex encoding expectations

The added docs are helpful. Please explicitly state what formats are accepted:

  • whether --to/--from must be Filecoin addresses (f/t or delegated f4) or if 0x Ethereum addresses are accepted,
  • that --payload accepts hex with or without a 0x prefix,
  • that --topic should be a 0x-prefixed 32-byte hash.

This avoids confusion when running against Ethereum-centric tooling.

Apply this diff to augment the help text:

     /// Some tests require sending a transaction to trigger events; the provided
     /// `from`, `to`, `payload`, and `topic` inputs are used for those cases.
     ///
     /// 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.
+    ///
+    /// Inputs:
+    /// - `--to`/`--from`: Filecoin addresses (f.../t... or delegated f4). Ethereum 0x addresses are
+    ///   not currently accepted here.
+    /// - `--payload`: hex-encoded calldata; both 0x-prefixed and non-prefixed forms are accepted.
+    /// - `--topic`: 0x-prefixed 32-byte topic hash.

231-236: Consider accepting 0x Ethereum addresses by converting to delegated f4

Right now Address::from_str only handles Filecoin f/t (and f4 delegated). Many users will pass 0x Ethereum addresses. Consider detecting 0x… here, parsing to H160, and converting to a delegated f4 before constructing Address. If you prefer not to implement now, please make the docs explicit (see prior comment).

Would you like me to draft a helper that converts an H160 to a delegated f4 Address if the codebase exposes such a conversion utility?

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

293-308: Reduce flakiness: increase wait window for mempool detection

1 second total wait (100 × 10ms) can be too short in CI or under load, causing intermittent failures. Bump retries and delay modestly.

-async fn wait_pending_message(client: &rpc::Client, message_cid: Cid) -> anyhow::Result<()> {
-    let mut retries = 100;
+async fn wait_pending_message(client: &rpc::Client, message_cid: Cid) -> anyhow::Result<()> {
+    let mut retries = 300;
     loop {
         let pending = client
             .call(MpoolPending::request((ApiTipsetKey(None),))?)
             .await?;
 
         if pending.0.iter().any(|msg| msg.cid() == message_cid) {
             break Ok(());
         }
         ensure!(retries != 0, "Message not found in mpool");
         retries -= 1;
 
-        tokio::time::sleep(Duration::from_millis(10)).await;
+        tokio::time::sleep(Duration::from_millis(50)).await;
     }
 }

518-523: Type normalization helper may hide server bugs only when empty; consider explicit validation

as_logs converts only empty Hashes to empty Logs, allowing success only when logs are non-empty. If the server incorrectly returns the Hashes variant for eth_getFilterLogs, this helper won’t surface it unless it’s non-empty. Consider explicitly asserting the returned variant is Logs to catch type mismatches early, and drop the normalization unless you need it for known compatibility.

📜 Review details

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 86f4c3f and 7492451.

📒 Files selected for processing (2)
  • src/tool/subcommands/api_cmd.rs (5 hunks)
  • src/tool/subcommands/api_cmd/stateful_tests.rs (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/tool/subcommands/api_cmd/stateful_tests.rs (1)
src/tool/subcommands/api_cmd.rs (1)
  • run (249-461)
src/tool/subcommands/api_cmd.rs (5)
src/rpc/client.rs (2)
  • client (114-114)
  • from_url (78-88)
src/rpc/methods/eth.rs (13)
  • new (512-524)
  • new (677-682)
  • from_str (262-264)
  • from_str (370-381)
  • from_str (430-449)
  • from (136-138)
  • from (142-144)
  • from (268-271)
  • from (275-277)
  • filter (3101-3101)
  • filter (3151-3151)
  • filter (3176-3176)
  • filter (3208-3208)
src/shim/address.rs (2)
  • from_str (178-183)
  • from_str (299-302)
src/utils/mod.rs (1)
  • from_str (42-53)
src/tool/subcommands/api_cmd/stateful_tests.rs (2)
  • create_tests (606-655)
  • run_tests (91-168)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: All lint checks
  • GitHub Check: Check
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)

Comment on lines +562 to +567
let filter_spec = EthFilterSpec {
from_block: Some(format!("0x{:x}", block_num.0 - BLOCK_RANGE)),
to_block: Some(block_num.to_hex_string()),
topics: Some(topics),
..Default::default()
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Possible underflow when computing from_block; use saturating_sub and consistent hex formatting

If the included block height were ever 0 (or very low), this subtraction could underflow. Use saturating_sub and the same to_hex_string helper used elsewhere for consistency.

-            let filter_spec = EthFilterSpec {
-                from_block: Some(format!("0x{:x}", block_num.0 - BLOCK_RANGE)),
-                to_block: Some(block_num.to_hex_string()),
-                topics: Some(topics),
-                ..Default::default()
-            };
+            let filter_spec = EthFilterSpec {
+                from_block: Some(EthUint64(block_num.0.saturating_sub(BLOCK_RANGE)).to_hex_string()),
+                to_block: Some(block_num.to_hex_string()),
+                topics: Some(topics),
+                ..Default::default()
+            };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let filter_spec = EthFilterSpec {
from_block: Some(format!("0x{:x}", block_num.0 - BLOCK_RANGE)),
to_block: Some(block_num.to_hex_string()),
topics: Some(topics),
..Default::default()
};
let filter_spec = EthFilterSpec {
from_block: Some(EthUint64(block_num.0.saturating_sub(BLOCK_RANGE)).to_hex_string()),
to_block: Some(block_num.to_hex_string()),
topics: Some(topics),
..Default::default()
};
🤖 Prompt for AI Agents
In src/tool/subcommands/api_cmd/stateful_tests.rs around lines 562 to 567, the
computation for from_block uses raw subtraction which can underflow when
block_num is small; change it to use block_num.saturating_sub(BLOCK_RANGE) and
format using the same to_hex_string helper (e.g. call .to_hex_string() on the
result) so both from_block and to_block use consistent hex formatting and avoid
underflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC requires calibnet RPC checks to run on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test coverage for filter-related Ethereum API methods
4 participants