Skip to content

Conversation

@sudo-shashank
Copy link
Contributor

@sudo-shashank sudo-shashank commented Jan 7, 2026

Summary of changes

Changes introduced in this pull request:

  • Impl Filecoin.EthGetTransactionCount V2 and added test.

Reference issue to close (if applicable)

Closes #6303

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

Release Notes

  • New Features

    • Added Filecoin.EthGetTransactionCount v2 API endpoint with extended block parameter support for more flexible transaction count queries.
  • Tests

    • Added test coverage for the new EthGetTransactionCount v2 endpoint, including variants for block hashes and predefined block identifiers.

✏️ Tip: You can customize this high-level summary in your review settings.

@sudo-shashank sudo-shashank added the RPC requires calibnet RPC checks to run on CI label Jan 7, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 7, 2026

Walkthrough

This PR implements Filecoin.EthGetTransactionCount for the V2 API surface. A new EthGetTransactionCountV2 RPC method is introduced using ExtBlockNumberOrHash parameters, while the existing method is refactored to use a shared helper function that centralizes actor state nonce and sequence retrieval logic.

Changes

Cohort / File(s) Summary
Core RPC Implementation
src/rpc/methods/eth.rs
Added EthGetTransactionCountV2 method for V2 API with ExtBlockNumberOrHash parameter type. Refactored existing EthGetTransactionCount to branch on block parameter: pending blocks return mpool sequence directly; other blocks resolve tipset and call shared helper. Introduced internal eth_get_transaction_count helper that fetches actor state, returning nonce for EVM contracts or sequence for other actors.
Method Registration
src/rpc/mod.rs
Registered new EthGetTransactionCountV2 method in the for_each_rpc_method macro to expose it as a public RPC method.
Testing & Snapshots
src/tool/subcommands/api_cmd/api_compare_tests.rs, test_snapshots.txt
Added test cases for EthGetTransactionCountV2 with block hash, predefined block variants (Earliest, Pending, Latest, Safe, Finalized), and corresponding snapshot entries. Tests apply PassWithQuasiIdenticalError policy consistent with existing patterns.
Documentation
CHANGELOG.md
Added entry documenting implementation of Filecoin.EthGetTransactionCount for API v2 (#6387).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant RPC as EthGetTransactionCount
    participant BlockResolver
    participant ActorState as Actor State/Mpool
    
    Client->>RPC: eth_getTransactionCount(address, blockParam)
    
    alt blockParam == Pending
        RPC->>ActorState: mpool.get_sequence(address)
        ActorState-->>RPC: sequence
    else Other Block
        RPC->>BlockResolver: tipset_by_block_number_or_hash(blockParam)
        BlockResolver-->>RPC: tipset
        RPC->>ActorState: eth_get_transaction_count(ctx, tipset, address)
        ActorState->>ActorState: Load actor from state
        alt Actor is EVM Contract
            ActorState-->>RPC: actor.nonce
        else Other Actor Type
            ActorState-->>RPC: actor.sequence
        end
    end
    
    RPC-->>Client: EthUint64(nonce/sequence)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • akaladarshi
  • LesnyRumcajs
  • hanabi1224
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding V2 API support for the Filecoin.EthGetTransactionCount RPC method.
Linked Issues check ✅ Passed The PR successfully implements the Filecoin.EthGetTransactionCount V2 endpoint as required by issue #6303, including proper V2 API routing, test coverage, and CHANGELOG documentation.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the V2 RPC endpoint: new method definition, helper refactoring, method registration, tests, and documentation—no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ccd220c and 66e97a3.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • src/rpc/methods/eth.rs
  • src/rpc/mod.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-05T12:54:40.850Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.

Applied to files:

  • src/rpc/mod.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/rpc/methods/eth.rs
📚 Learning: 2026-01-05T12:56:13.802Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.

Applied to files:

  • src/rpc/mod.rs
  • src/tool/subcommands/api_cmd/api_compare_tests.rs
  • src/rpc/methods/eth.rs
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.

Applied to files:

  • src/tool/subcommands/api_cmd/test_snapshots.txt
📚 Learning: 2025-09-02T10:05:34.350Z
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.

Applied to files:

  • src/rpc/methods/eth.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). (3)
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: Coverage
🔇 Additional comments (7)
src/rpc/methods/eth.rs (3)

2396-2410: LGTM!

The refactored logic correctly branches on the block parameter:

  • For Pending, it returns the mpool sequence directly, which represents the next expected nonce
  • For other block parameters, it resolves the tipset and delegates to the shared helper

This is a clean separation of concerns.


2412-2443: LGTM!

The V2 implementation correctly:

  • Uses ExtBlockNumberOrHash to support additional predefined blocks (Safe, Finalized)
  • Uses the async tipset_by_block_number_or_hash_v2 for tipset resolution which properly handles the extended tags
  • Shares the same helper function as V1 for consistent behavior

2445-2466: The current implementation intentionally errors when querying a non-existent address, not returning 0. This design choice is deliberate to match Lotus behavior, as evidenced by the test suite's use of PolicyOnRejected::PassWithQuasiIdenticalError for this exact scenario. While standard Ethereum returns 0 for non-existent addresses, Filecoin's Eth RPC implementation deliberately maintains compatibility with Lotus rather than matching pure Ethereum semantics. No change is needed.

Likely an incorrect or invalid review comment.

CHANGELOG.md (1)

38-38: LGTM!

The changelog entry is correctly formatted and appropriately placed in the "Added" section for the new V2 API endpoint.

src/tool/subcommands/api_cmd/test_snapshots.txt (1)

81-81: LGTM!

The new snapshot file entry follows the established naming convention and is correctly placed in alphabetical order.

src/rpc/mod.rs (1)

128-128: LGTM!

The new EthGetTransactionCountV2 method is correctly registered in the macro and logically placed adjacent to its V1 counterpart, consistent with other V2 method registrations in the codebase.

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

1760-1802: LGTM! Well-structured V2 test coverage.

The EthGetTransactionCountV2 tests are properly implemented and follow these good practices:

  • Consistent structure: Mirrors the existing EthGetTransactionCount V1 tests (lines 1732-1759)
  • Appropriate V2 types: Uses ExtBlockNumberOrHash and ExtPredefined instead of the V1 equivalents
  • Complete predefined block coverage: Tests Earliest, Pending, Latest, Safe, and Finalized (Safe and Finalized are new in V2)
  • Correct test policies: Uses identity tests for deterministic cases (block hash, Earliest) and basic tests for non-deterministic cases (Pending, Latest, Safe, Finalized)
  • Error handling: Applies PassWithQuasiIdenticalError policy for Earliest, consistent with V1

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

@sudo-shashank sudo-shashank marked this pull request as ready for review January 8, 2026 06:22
@sudo-shashank sudo-shashank requested a review from a team as a code owner January 8, 2026 06:22
@sudo-shashank sudo-shashank requested review from LesnyRumcajs and hanabi1224 and removed request for a team January 8, 2026 06:22
@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

❌ Patch coverage is 57.14286% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.50%. Comparing base (ccd220c) to head (66e97a3).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/rpc/methods/eth.rs 57.14% 12 Missing and 6 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/rpc/mod.rs 24.47% <ø> (ø)
src/rpc/methods/eth.rs 66.77% <57.14%> (+0.26%) ⬆️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccd220c...66e97a3. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

LGTM

@sudo-shashank sudo-shashank added this pull request to the merge queue Jan 8, 2026
Merged via the queue into main with commit 0af174d Jan 8, 2026
57 checks passed
@sudo-shashank sudo-shashank deleted the shashank/eth-get-transaction-count-v2 branch January 8, 2026 10:12
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.

[RPC v2] Filecoin.EthGetTransactionCount

3 participants