fix(rpc): register RPC methods for different versions into different modules #6219

hanabi1224 · 2025-11-03T11:05:33Z

Summary of changes

To unblock #6182

V2 version of ChainGetTipSet uses a different parameter, this PR allows implementing and registering the V2 implementation separately.

// V0 and V1
func (m *ChainModule) ChainGetTipSet(ctx context.Context, key types.TipSetKey) (*types.TipSet, error) {
	...
}

// V2
func (cm *ChainModuleV2) ChainGetTipSet(ctx context.Context, selector types.TipSetSelector) (*types.TipSet, error) {
        ...
}

This PR also adds an unimplemented Filecoin.ChainGetTipSet for V2 to prove the correctness

➜  forest git:(hm/fix-rpc-registration) curl -s -X POST http://localhost:2345/rpc/v2 -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"Filecoin.ChainGetTipSet","id":1}'
{"jsonrpc":"2.0","id":1,"error":{"code":-32001,"message":"unsupported method","data":"This method is not supported by the current version of the Forest node"}}

➜  forest git:(hm/fix-rpc-registration) curl -s -X POST http://localhost:2345/rpc/v1 -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"Filecoin.ChainGetTipSet","id":1}'
{"jsonrpc":"2.0","id":1,"result":{...}}

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

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
- Per-request multi-version RPC routing (V0, V1, V2).
- V2 variant of ChainGetTipSet added (handler currently unimplemented).
- RPC test snapshots now include an optional api_path for version-filtered test runs.
Changes
- ChainGetTipSet exposure restricted to V0 and V1.
- RPC startup and registration reworked to register and dispatch methods per API version.
- Test snapshot references updated to new revision numbers.

…modules

coderabbitai · 2025-11-03T11:05:46Z

Walkthrough

Refactors RPC to support per-API-version modules and per-request method lookup; restricts existing ChainGetTipSet to V0|V1, adds ChainGetTipSetV2 (unimplemented), changes extension-layer path handling to use an ApiPaths value directly, and threads api_path through test snapshot tooling.

Changes

Cohort / File(s)	Summary
Chain method versioning `src/rpc/methods/chain.rs`	Restricted `ChainGetTipSet` `API_PATHS` to `V0
Module creation & dispatch `src/rpc/mod.rs`	Replaced `create_module` → `create_modules` returning `HashMap<ApiPaths, RpcModule<...>>`; build and merge per-version modules (including pubsub) and expose per-request methods lookup; removed stored `Methods` from `PerConnection`; threaded `ApiPaths` via `SetExtensionLayer`.
Method registration trait `src/rpc/reflect/mod.rs`	Changed `RpcMethodExt::register` to accept `&mut HashMap<ApiPaths, RpcModule<...>>` and register methods per `ApiPaths`; removed `register_alias`; adjusted imports to use `ahash::HashMap` and `clap::ValueEnum as _`.
Extension layer / path handling `src/rpc/set_extension_layer.rs`	`SetExtensionLayer` no longer derives `Default`; `path` changed from `Option<ApiPaths>` → `ApiPaths` on layer and service; removed internal `path()` helper and updated call/batch/notification logic to use `self.path`.
RPC test snapshot tooling `src/tool/subcommands/api_cmd.rs`, `src/tool/subcommands/api_cmd/test_snapshot.rs`, `src/tool/subcommands/api_cmd/test_snapshots.txt`	Added optional `api_path: Option<rpc::ApiPaths>` to `RpcTestSnapshot`, initialized to `None` in snapshot generation; deserialization defaults to `ApiPaths::V1` and test dispatch filters methods by snapshot's `api_path`. Updated two snapshot filenames to new revisions in `test_snapshots.txt`.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant RPC_Server
    participant Modules_Map
    participant Module
    participant Handler

    rect rgb(240,248,255)
    Note over RPC_Server,Modules_Map: Startup builds per-version RpcModule map
    Client->>RPC_Server: JSON-RPC request (method, api_path)
    RPC_Server->>Modules_Map: lookup RpcModule for api_path
    Modules_Map-->>RPC_Server: selected RpcModule
    end

    rect rgb(240,248,255)
    RPC_Server->>Module: resolve method in module
    Module->>Handler: invoke handler
    end

    rect rgb(255,240,240)
    alt api_path == V2 and method == Filecoin.ChainGetTipSet
        Handler-->>Client: error "ChainGetTipSet V2 not yet implemented"
    else
        Handler-->>Client: normal Tipset response
    end
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas to review closely:
- HashMap-based module creation/lookup and cloning/ownership semantics in src/rpc/mod.rs
- Trait signature changes and all registration call sites in src/rpc/reflect/mod.rs
- SetExtensionLayer path type change and batch/call/notification code paths
- Registration and naming collisions for ChainGetTipSetV2 vs existing method versions
- Test snapshot filtering logic in src/tool/subcommands/api_cmd/test_snapshot.rs

Possibly related PRs

fix(rpc): batch request failures #6206 — Modifications to src/rpc/set_extension_layer.rs and per-entry path insertion in batch handling; closely related to extension-layer changes.
feat: enable V2 API support for basic Eth and Filecoin RPC methods #6171 — Works on V2 API routing and method-versioning; overlaps with per-version dispatch and ApiPaths handling.
spec: Add ChainGetFinalizedTipset API #6114 — Adds RPC methods and updates method registration flow similar to changes here.

Suggested reviewers

akaladarshi
sudo-shashank
LesnyRumcajs

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'fix(rpc): register RPC methods for different versions into different modules' accurately and specifically describes the main change in the changeset. The title captures the core objective of the PR—restructuring RPC registration to support different implementations for different API versions across separate modules—which directly aligns with the extensive modifications across src/rpc/mod.rs, src/rpc/reflect/mod.rs, and src/rpc/set_extension_layer.rs. The title is concise, uses the conventional 'fix' prefix appropriate for a bug fix enabling version-specific routing, and provides sufficient clarity for teammates reviewing the Git history.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hm/fix-rpc-registration

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17de43e and 2fcea77.

📒 Files selected for processing (1)

src/tool/subcommands/api_cmd/test_snapshots.txt (2 hunks)

✅ Files skipped from review due to trivial changes (1)

src/tool/subcommands/api_cmd/test_snapshots.txt

⏰ 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). (7)

GitHub Check: Build MacOS
GitHub Check: Build Ubuntu
GitHub Check: cargo-publish-dry-run
GitHub Check: tests
GitHub Check: tests-release
GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: All lint checks

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

hanabi1224 · 2025-11-03T11:09:07Z

src/rpc/methods/chain.rs

+    const PERMISSION: Permission = Permission::Read;
+    const DESCRIPTION: Option<&'static str> = Some("Returns the tipset with the specified CID.");
+
+    type Params = (ApiTipsetKey,);


It should be TipsetSelector. The method will be implemented in a subsequent PR.

type TipSetSelector struct { Key *TipSetKey `json:"key,omitempty"` Height *TipSetHeight `json:"height,omitempty"` Tag *TipSetTag `json:"tag,omitempty"` }

LesnyRumcajs · 2025-11-03T11:13:14Z

src/rpc/methods/chain.rs

+    type Ok = Tipset;
+
+    async fn handle(_: Ctx<impl Blockstore>, _: Self::Params) -> Result<Self::Ok, ServerError> {
+        Err(anyhow::anyhow!("Filecoin.ChainGetTipSet for V2 is not yet implemented").into())


should we rather return

forest/src/rpc/error.rs

Lines 76 to 82 in a8e0dbc

pub fn unsupported_method() -> Self {

Self::new(

implementation_defined_errors::UNSUPPORTED_METHOD,

"unsupported method",

Some("This method is not supported by the current version of the Forest node".into()),

)

}

?

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6317843 and 244ccee.

📒 Files selected for processing (1)

src/rpc/methods/chain.rs (2 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

📚 Learning: 2025-08-11T14:00:47.060Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.

Applied to files:

src/rpc/methods/chain.rs

📚 Learning: 2025-08-28T12:52:46.927Z

Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6011
File: src/cli/main.rs:18-25
Timestamp: 2025-08-28T12:52:46.927Z
Learning: In Forest CLI (src/cli/main.rs), the early RPC network check before Cli::parse_from() does not block help/version commands because clap processes these internally before reaching the RPC call. LesnyRumcajs confirmed this implementation works correctly and that RPC call failures are acceptable in this context.

Applied to files:

src/rpc/methods/chain.rs

📚 Learning: 2025-08-08T12:11:55.266Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.

Applied to files:

src/rpc/methods/chain.rs

📚 Learning: 2025-10-17T09:36:15.757Z

Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.

Applied to files:

src/rpc/methods/chain.rs

🧬 Code graph analysis (1)

src/rpc/methods/chain.rs (2)

src/rpc/methods/state.rs (16)

handle (109-114)

handle (132-138)

handle (151-157)

handle (172-178)

handle (196-205)

handle (223-231)

handle (248-255)

handle (272-282)

handle (298-305)

handle (335-465)

handle (484-492)

handle (508-538)

handle (555-561)

handle (577-597)

handle (615-624)

handle (641-663)

src/rpc/error.rs (1)

unsupported_method (76-82)

⏰ 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). (7)

GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: cargo-publish-dry-run
GitHub Check: Build MacOS
GitHub Check: Build Ubuntu
GitHub Check: tests-release
GitHub Check: tests
GitHub Check: All lint checks

🔇 Additional comments (2)

src/rpc/methods/chain.rs (2)

970-970: LGTM! Proper API version segregation.

Restricting the existing ChainGetTipSet to V0 and V1 paths is correct, as V2 will be handled by the separate ChainGetTipSetV2 implementation.

999-1001: LGTM! Correct use of unsupported_method error.

The handle implementation correctly returns ServerError::unsupported_method() to indicate the V2 endpoint is not yet implemented, which aligns with the PR's goal of demonstrating proper version-based routing.

Based on past review comments.

coderabbitai · 2025-11-03T11:25:05Z

src/rpc/methods/chain.rs

+    const PARAM_NAMES: [&'static str; 1] = ["tipsetSelector"];
+    const API_PATHS: BitFlags<ApiPaths> = make_bitflags!(ApiPaths::{ V2 });
+    const PERMISSION: Permission = Permission::Read;
+    const DESCRIPTION: Option<&'static str> = Some("Returns the tipset with the specified CID.");
+
+    type Params = (ApiTipsetKey,);


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Clarify parameter type vs. parameter name inconsistency.

The parameter name "tipsetSelector" (line 991) suggests a different type than V0/V1, but Params is still (ApiTipsetKey,) (line 996)—the same type used in the V0/V1 implementation. According to the PR objectives and past review comments, V2 should accept a TipSetSelector type.

Is the intention to:

Add the TipSetSelector type now and update line 996 to use it, or

Keep the same type temporarily and change the parameter name to "tipsetKey" to avoid confusion?

The current mismatch creates a misleading API surface.

Based on past review comments indicating the parameter should be TipSetSelector with fields for Key, Height, and Tag.

--- 🏁 Script executed: ```shell #!/bin/bash # Search for ApiTipsetKey type definition rg -n "type\s+ApiTipsetKey" --type=rs -A 3

Length of output: 26

🏁 Script executed:

#!/bin/bash # Search for TipSetSelector type definition rg -n "type\s+TipSetSelector|struct\s+TipSetSelector" --type=rs -A 5

Length of output: 26

🏁 Script executed:

#!/bin/bash # Examine ChainGetTipSetV2 full implementation (lines 988-1010) sed -n '988,1010p' src/rpc/methods/chain.rs

Length of output: 913

🏁 Script executed:

#!/bin/bash # Search for V0/V1 ChainGetTipSet implementations for comparison rg -n "struct.*ChainGetTipSet" --type=rs -A 12 | head -50

Length of output: 26

🏁 Script executed:

#!/bin/bash # Check for TipsetSelector mentions in comments or code rg -n -i "tipset.*selector|tipsetselector" --type=rs -B 2 -A 2

Length of output: 26

--- 🏁 Script executed: ```shell #!/bin/bash # Search for ApiTipsetKey type definition without type flag rg -n "ApiTipsetKey" -A 3 src/rpc/methods/chain.rs | head -20

Length of output: 651

🏁 Script executed:

#!/bin/bash # Search for TipSetSelector type definition anywhere rg -n "TipSetSelector" -B 1 -A 5

Length of output: 0

🏁 Script executed:

#!/bin/bash # Find V0 ChainGetTipSet implementation rg -n "pub enum ChainGetTipSet" -A 12 src/rpc/methods/chain.rs | head -30

Length of output: 1350

🏁 Script executed:

#!/bin/bash # Find all ChainGetTipSet enum variants to compare parameter names rg -n "ChainGetTipSet" -B 2 -A 8 src/rpc/methods/chain.rs | grep -E "(pub enum|PARAM_NAMES|type Params)"

Length of output: 545

🏁 Script executed:

#!/bin/bash # Search for what other methods use in PARAM_NAMES to understand naming convention rg -n 'PARAM_NAMES.*=.*\[' -A 0 src/rpc/methods/chain.rs | head -15

Length of output: 881

Update PARAM_NAMES to match the actual Params type.

The parameter name "tipsetSelector" (line 991) is misleading. It suggests a different type, but Params is (ApiTipsetKey,) (line 996)—the same type used in ChainGetTipSet V0/V1, which uses the parameter name "tipsetKey" (line 969).

The codebase naming convention consistently matches parameter names to their types (e.g., "messageCid" for CID, "blockCid" for CID). Since TipSetSelector does not exist in the codebase and the actual type is ApiTipsetKey, change line 991 to:

const PARAM_NAMES: [&'static str; 1] = ["tipsetKey"];

This maintains consistency with V0/V1 and accurately reflects the actual parameter type.

🤖 Prompt for AI Agents

In src/rpc/methods/chain.rs around lines 991 to 996, PARAM_NAMES currently uses "tipsetSelector" which does not match the actual Params type (ApiTipsetKey); update PARAM_NAMES to use "tipsetKey" so the parameter name matches the type and aligns with V0/V1 naming conventions (i.e., change the string literal to "tipsetKey").

This is intended as noted in #6219 (comment) . Will be fixed in a subsequent PR.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash # Fetch the referenced discussion comment to understand the context gh api /repos/ChainSafe/forest/pulls/6219/comments -q '.[] | select(.id == 2486104046) | .body'

Length of output: 261

@hanabi1224, understood! Thanks for the clarification. The forward-looking parameter name "tipsetSelector" makes sense as a placeholder indicating the future TipsetSelector type that will be implemented in the subsequent PR. This phased approach—establishing the routing infrastructure now and adding the proper type implementation later—is reasonable for unblocking issue #6182.

🧬 Code Graph Analysis Results

[relevant_snippets]

src/rpc/methods/chain.rs

ForestChainExport

Summary:

RPC: Forest.ChainExport

Params: ForestChainExportParams

Returns: ApiExportResult

Behavior: Manages a chain export operation with a global lock CHAIN_EXPORT_LOCK. Supports dry-run, multiple versions (V1/V2), and cancellation via a CancellationToken. Handles streaming export results and cleans up token on completion or cancellation. Delegates to chain export logic (export:: or export_v2::<Sha256, _> depending on version). Ensures proper error propagation and cleanup of export state.

ForestChainExportStatus

Summary:

RPC: Forest.ChainExportStatus

Params: none

Returns: ApiExportStatus

Behavior: Reports current export progress, whether exporting, cancelled, and start_time. Reads from CHAIN_EXPORT_STATUS.

ForestChainExportCancel

Summary:

RPC: Forest.ChainExportCancel

Params: none

Returns: bool

Behavior: Cancels an in-progress chain export if a token exists in CHAIN_EXPORT_LOCK.

ForestChainExportDiff

Summary:

RPC: Forest.ChainExportDiff

Params: ForestChainExportDiffParams

Returns: ()

Behavior: Validates and starts an export-diff operation, ensuring no concurrent export is active, and that depth satisfies chain_finality. Initiates an archival export for the diff.

ChainExport

Summary:

RPC: Filecoin.ChainExport

Params: ChainExportParams

Returns: ApiExportResult

Behavior: Delegates to ForestChainExport with version V1, mapping ChainExportParams to ForestChainExportParams.

ChainReadObj

Summary:

RPC: Filecoin.ChainReadObj

Params: cid

Returns: Vec

Behavior: Reads IPLD node bytes by CID from the chain blockstore and returns raw bytes.

ChainHasObj

Summary:

RPC: Filecoin.ChainHasObj

Params: cid

Returns: bool

Behavior: Checks existence of a CID in the chain blockstore.

ChainStatObj

Summary:

RPC: Filecoin.ChainStatObj

Params: obj_cid, base_cid

Returns: ObjStat

Behavior: Computes statistics for the graph reachable from obj_cid, optionally as a diff against base_cid. Traverses IPLD links and accumulates size/links.

ChainGetBlockMessages

Summary:

RPC: Filecoin.ChainGetBlockMessages

Params: blockCid

Returns: BlockMessages

Behavior: Loads block header from blockstore, resolves message CIDs, reconstructs and returns all messages in the block (BLS and SECP, plus CIDs).

ChainGetMessagesInTipset

Summary:

RPC: Filecoin.ChainGetMessagesInTipset

Params: tipsetKey

Returns: Vec

Behavior: Loads tipset from store, aggregates all messages in the tipset, including duplicates deduplicated by CID.

ChainGetPath

Summary:

RPC: Filecoin.ChainGetPath

Params: from, to (TipsetKey)

Returns: Vec

Behavior: Computes the sequence of changes to go from one tipset to another (reverts/applies). Uses load_required_tipset_or_heaviest and backtracks via parents to determine changes. Errors on absent common ancestor.

ChainGetTipSetByHeight

Summary:

RPC: Filecoin.ChainGetTipSetByHeight

Params: height, tipsetKey

Returns: Tipset

Behavior: Returns the tipset at a specified height, searching from a given head tipset, respecting null-tipset behavior and head limitations.

ChainGetTipSet (V0/V1)

Summary:

RPC: Filecoin.ChainGetTipSet

Params: tipsetKey

Returns: Tipset

Behavior: Loads and returns the tipset for the provided key, for V0/V1 paths.

ChainGetTipSet (V2)

Summary:

RPC: Filecoin.ChainGetTipSet

Params: tipsetKey (ApiTipsetKey)

Returns: Tipset

Behavior: Placeholder/unsupported in this version; returns unsupported_method error.

ChainHead

Summary:

RPC: Filecoin.ChainHead

Params: none

Returns: Tipset

Behavior: Returns the heaviest (head) tipset.

ChainGetGenesis

Summary:

RPC: Filecoin.ChainGetGenesis

Params: none

Returns: Option

Behavior: Returns the genesis tipset if available.

ChainGetBlock

Summary:

RPC: Filecoin.ChainGetBlock

Params: blockCid

Returns: CachingBlockHeader

Behavior: Returns the block header for a given block CID.

ChainNotify

Summary:

RPC: Filecoin.ChainNotify

Params: none

Returns: Subscriber<Vec>

Behavior: Streams head changes to subscribers, starting with the current tipset.

chain_notify (helper)

Summary:

Context: Helper function returning a Subscriber<Vec>

Behavior: Seeds the subscriber with the current tipset then streams subsequent changes.

load_api_messages_from_tipset (helper)

Summary:

Inputs: store, tipset

Returns: Vec

Behavior: Loads a full tipset from blockstore, iterates blocks, and collects unique messages (by CID) from both BLS and SECP message lists.

ChainGetParentMessages

Summary:

RPC: Filecoin.ChainGetParentMessages

Params: blockCid

Returns: Vec

Behavior: Returns messages included in the parent tipset of the given block CID.

ChainGetParentReceipts

Summary:

RPC: Filecoin.ChainGetParentReceipts

Params: blockCid

Returns: Vec

Behavior: Returns receipts included in the parent tipset of the given block CID.

ChainGetMessagesInTipset (already described)

ChainGetBlockMessages (already described)

Notes

File sections above reference the code paths in Forest’s chain RPC implementation that interact with tipsets, blocks, and chain state. They provide context for understanding how chain.rs supports RPC methods like chain export, tipset queries, and message/block access, which aligns with and complements the behavior noted in the user comment about a PR discussion related to chain finality/export semantics.

🧠 Learnings used

Learnt from: akaladarshi Repo: ChainSafe/forest PR: 5923 File: src/rpc/registry/actors/miner.rs:221-223 Timestamp: 2025-09-02T10:05:34.350Z Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.

Learnt from: hanabi1224 Repo: ChainSafe/forest PR: 5930 File: build.rs:64-77 Timestamp: 2025-08-13T09:43:20.301Z Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224 Repo: ChainSafe/forest PR: 6057 File: src/cli/subcommands/f3_cmd.rs:0-0 Timestamp: 2025-09-09T10:37:17.947Z Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

LesnyRumcajs

LGTM

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/rpc/reflect/mod.rs (1)
259-276: Consider iterating only over Self::API_PATHS instead of all variants.

The current implementation iterates through all ApiPaths::value_variants() and checks Self::API_PATHS.contains(*api_version) for each. For methods that only support V0|V1, this unnecessarily checks V2 (and future versions).

While the performance impact is negligible, iterating directly over Self::API_PATHS.iter() would be more efficient and clearer in intent.

Apply this diff to optimize the iteration:
-        for api_version in ApiPaths::value_variants() {
-            if Self::API_PATHS.contains(*api_version)
-                && let Some(module) = modules.get_mut(api_version)
+        for api_version in Self::API_PATHS.iter() {
+            if let Some(module) = modules.get_mut(&api_version)
             {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9ccec8 and 17de43e.

📒 Files selected for processing (3)

src/rpc/reflect/mod.rs (3 hunks)
src/tool/subcommands/api_cmd.rs (1 hunks)
src/tool/subcommands/api_cmd/test_snapshot.rs (4 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

📚 Learning: 2025-08-25T14:17:09.129Z

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:0-0
Timestamp: 2025-08-25T14:17:09.129Z
Learning: hanabi1224's download_file_with_cache function in src/utils/net/download_file.rs preserves URL path structure in local cache directories by using cache_dir.join(url.path().strip_prefix('/').unwrap_or_else(|| url.path())), so snapshots from https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/ are cached locally at ~/.cache/forest/test/rpc-snapshots/rpc_test/ (including the rpc_test subdirectory from the URL path).

Applied to files:

src/tool/subcommands/api_cmd/test_snapshot.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). (7)

GitHub Check: All lint checks
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-release
GitHub Check: tests

🔇 Additional comments (7)

src/rpc/reflect/mod.rs (2)

28-28: LGTM!

The ahash::HashMap import is appropriate for the new per-version module registration.

119-132: LGTM!

The addition of Deserialize, Serialize, and ValueEnum derives enables per-version test snapshot handling and the new registration loop.

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

19-19: LGTM!

The ApiPaths import is required for the new version-aware test filtering.

51-52: LGTM!

The optional api_path field with skip_serializing_if maintains backward compatibility with existing test snapshots.

90-95: LGTM!

Defaulting api_path to ApiPaths::V1 when None maintains backward compatibility with existing test snapshots that lack this field. This aligns with ApiPaths::all() which currently returns V0|V1.

108-108: LGTM!

The API_PATHS.contains(api_path) check correctly filters test execution to only run against methods that support the snapshot's API version, preventing false failures and enabling targeted version testing.

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

369-370: Verify that issue #6220 exists and confirm a plan to populate api_path during snapshot generation.

The technical concern is valid: api_path is set to None at line 370, which causes all generated snapshots to default to ApiPaths::V1 at line 95 in test_snapshot.rs. This prevents generating snapshots for V0 or V2 endpoints and loses critical metadata about which API version each snapshot targets.

The issue reference exists in the code (line 369), but verify that GitHub issue #6220 tracks this problem and includes a concrete plan to capture the correct api_path during snapshot generation.

hanabi1224 · 2025-11-03T15:18:53Z

src/tool/subcommands/api_cmd/test_snapshots.txt

@@ -1,4 +1,4 @@
-filecoin_beacongetentry_1741270283524366.rpcsnap.json.zst
+filecoin_beacongetentry_1741270283524367.rpcsnap.json.zst


Added "api_path": "V0", to these 2 snapshot JSON files since they don't support V1 or V2

fix(rpc): register RPC methods for different versions into different …

6317843

…modules

hanabi1224 commented Nov 3, 2025

View reviewed changes

hanabi1224 marked this pull request as ready for review November 3, 2025 11:10

hanabi1224 requested a review from a team as a code owner November 3, 2025 11:10

hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team November 3, 2025 11:10

LesnyRumcajs reviewed Nov 3, 2025

View reviewed changes

use unified error

244ccee

hanabi1224 added the RPC requires calibnet RPC checks to run on CI label Nov 3, 2025

coderabbitai bot reviewed Nov 3, 2025

View reviewed changes

hanabi1224 mentioned this pull request Nov 3, 2025

Update the RPC testing framework to support multiple API versions #6220

Closed

LesnyRumcajs previously approved these changes Nov 3, 2025

View reviewed changes

sudo-shashank previously approved these changes Nov 3, 2025

View reviewed changes

hanabi1224 enabled auto-merge November 3, 2025 11:31

hanabi1224 disabled auto-merge November 3, 2025 11:33

hanabi1224 enabled auto-merge November 3, 2025 11:39

hanabi1224 added 2 commits November 3, 2025 20:21

Merge branch 'main' into hm/fix-rpc-registration

a9ccec8

fix unit test

17de43e

hanabi1224 dismissed stale reviews from LesnyRumcajs and sudo-shashank via 17de43e November 3, 2025 14:18

hanabi1224 disabled auto-merge November 3, 2025 14:20

Merge branch 'main' into hm/fix-rpc-registration

1fa1fa7

coderabbitai bot reviewed Nov 3, 2025

View reviewed changes

update v0 only snapshots

2fcea77

hanabi1224 commented Nov 3, 2025

View reviewed changes

hanabi1224 requested review from LesnyRumcajs and sudo-shashank November 3, 2025 15:42

sudo-shashank approved these changes Nov 4, 2025

View reviewed changes

hanabi1224 added this pull request to the merge queue Nov 4, 2025

Merged via the queue into main with commit 717af47 Nov 4, 2025
40 checks passed

hanabi1224 deleted the hm/fix-rpc-registration branch November 4, 2025 07:33

hanabi1224 mentioned this pull request Nov 4, 2025

fix(rpc): method name of V0 only RPC methods #6225

Merged

4 tasks

This was referenced Nov 4, 2025

chore: upgrade rust to 1.91.0 #6226

Merged

feat(rpc): ChainGetTipset v2 #6231

Merged

This was referenced Dec 1, 2025

fix: avoid deep-cloning Tipset and FullTipset #6274

Merged

feat(rpc): V2 RPC methods StateGetActor and StateGetID #6312

Merged

This was referenced Dec 18, 2025

Add support for the Filecoin.EthEstimateGas V2 #6364

Merged

fix(test): speed up test-release and codecov on CI #6372

Merged

This was referenced Jan 5, 2026

Add support for the Filecoin.EthFeeHistory V2 #6380

Merged

Add support for the Filecoin.EthGetTransactionCount V2 #6387

Merged

Add support for the Filecoin.EthGetBalance V2 #6403

Merged

Add support for the Filecoin.EthGetBlockByNumber V2 #6404

Merged

coderabbitai bot mentioned this pull request Jan 13, 2026

Enable support for Filecoin.EthGetLogs V2 #6405

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rpc): register RPC methods for different versions into different modules #6219

fix(rpc): register RPC methods for different versions into different modules #6219

Uh oh!

hanabi1224 commented Nov 3, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 3, 2025 •

edited

Loading

Uh oh!

hanabi1224 Nov 3, 2025

Uh oh!

LesnyRumcajs Nov 3, 2025

Uh oh!

hanabi1224 Nov 3, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Nov 3, 2025 •

edited

Loading

Uh oh!

hanabi1224 Nov 3, 2025 •

edited

Loading

Uh oh!

coderabbitai bot Nov 3, 2025

Uh oh!

LesnyRumcajs left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

hanabi1224 Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	pub fn unsupported_method() -> Self {
	Self::new(
	implementation_defined_errors::UNSUPPORTED_METHOD,
	"unsupported method",
	Some("This method is not supported by the current version of the Forest node".into()),
	)
	}

		@@ -1,4 +1,4 @@
		filecoin_beacongetentry_1741270283524366.rpcsnap.json.zst
		filecoin_beacongetentry_1741270283524367.rpcsnap.json.zst

fix(rpc): register RPC methods for different versions into different modules #6219

fix(rpc): register RPC methods for different versions into different modules #6219

Uh oh!

Conversation

hanabi1224 commented Nov 3, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

hanabi1224 Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

LesnyRumcajs Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

hanabi1224 Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanabi1224 Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

hanabi1224 Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hanabi1224 commented Nov 3, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 3, 2025 •

edited

Loading

coderabbitai bot Nov 3, 2025 •

edited

Loading

hanabi1224 Nov 3, 2025 •

edited

Loading