Add support for the Filecoin.EthCall V2
#6339
Conversation
WalkthroughAdded a V2 variant of the Filecoin.EthCall RPC (EthCallV2) that accepts Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (9)
🔇 Additional comments (1)
Comment |
sudo-shashank
changed the title
Filecoin.EthCall V2 RPCFilecoin.EthCall V2
sudo-shashank
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (1)
54-55: Place the new EthCall v2 entry under “Forest unreleased → Added” instead of v0.30.5The description and link are fine, but the guide at the top says new entries should go into the Forest unreleased section. Unless you’re intentionally backfilling the 0.30.5 release notes, this should live under
Forest unreleased / Addedand be removed from the 0.30.5 list to avoid confusion about when the feature ships.@@ ## Forest unreleased ### Breaking ### Added + +- [#6339](https://github.com/ChainSafe/forest/pull/6339) Implemented `Filecoin.EthCall` for API v2. @@ -## Forest v0.30.5 "Dulce de Leche" +## Forest v0.30.5 "Dulce de Leche" @@ -- [#6323](https://github.com/ChainSafe/forest/pull/6323) Implemented `Filecoin.FilecoinAddressToEthAddress` for API v1 and v2. - -- [#6339](https://github.com/ChainSafe/forest/pull/6339) Implemented `Filecoin.EthCall` for API v2. +- [#6323](https://github.com/ChainSafe/forest/pull/6323) Implemented `Filecoin.FilecoinAddressToEthAddress` for API v1 and v2.src/rpc/methods/eth.rs (1)
907-927: V2 predefined tipset resolution correctly integrates F3 safe/finalized, but finalized fallback could be refinedThe new
resolve_predefined_tipset_v2/tipset_by_block_number_or_hash_v2pair cleanly reuses existing logic forEarliest/Pending/Latestwhile routingSafe/FinalizedthroughChainGetTipSetV2::{get_latest_safe_tipset,get_latest_finalized_tipset}, which is the right direction for v2 semantics.One nuance: when
get_latest_finalized_tipsetreturnsNone, the fallback currently resolves to height0viatipset_by_height(0, head, ResolveNullTipset::TakeOlder). That’s very conservative and effectively treats"finalized"as “genesis” on nodes without F3 finality, which may surprise callers and diverges from the height‑based approximation used inresolve_ext_predefined_tipset(which subtractschain_finalityfrom the latest height).Consider instead reusing the existing finalized‐height logic (e.g., via
resolve_ext_predefined_tipsetor by computinglatest_height - chain_finality) as a fallback so that"finalized"behaves consistently across v1/v2 call sites when F3 data is unavailable.Also applies to: 1019-1042
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)src/rpc/methods/eth.rs(5 hunks)src/rpc/mod.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
CHANGELOG.mdsrc/rpc/methods/eth.rs
🧬 Code graph analysis (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/rpc/methods/eth.rs (7)
from_predefined(362-364)from_predefined(422-424)from_str(285-287)from_str(393-404)from_str(453-472)from_block_number(366-368)from_block_number(426-428)src/rpc/reflect/mod.rs (2)
request_with_alias(322-340)request(288-298)
⏰ 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). (6)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Coverage
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
🔇 Additional comments (5)
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
54-56: EthCallV2 snapshots are well‑placed; just confirm remote artifacts existThe three
filecoin_ethcall_v2_*.rpcsnap.json.zstentries follow the existing naming pattern and are grouped correctly with the existingfilecoin_ethcall_*snapshots. Since this file is just a manifest and the actual.rpcsnapfiles are pulled from the DO bucket, please double‑check that the corresponding objects have been uploaded and that any newapi_compare_testscases reference these exact filenames. Based on learnings, the manifest is only an index for remote snapshots.src/rpc/mod.rs (1)
103-105: Correctly wiring EthCallV2 into the RPC method registryRegistering
$crate::rpc::eth::EthCallV2infor_each_rpc_method!is consistent with how other v2 variants (e.g.,StateGetActorV2,ChainGetTipSetV2) are exposed. This ensures EthCallV2 is included in module registration, OpenRPC generation, and the prelude without any extra boilerplate.src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1445-1474: EthCall/EthCallV2 shared message construction and new v2 tag coverage look goodReusing a single
EthCallMessageper(to, data)case and driving bothEthCallandEthCallV2tests from it keeps the two methods in lock‑step. The addedExtPredefined::{Latest, Safe, Finalized}variants exercise the new v2 resolver without changing existing v1 behaviour. No issues spotted here.
1549-1580: Extending eth_call API error coverage to EthCallV2 is consistentThe loop now creates paired tests for
EthCallandEthCallV2against the same bytecode and epoch, with identicalPolicyOnRejected::PassWithIdenticalError. This preserves the original v1 semantics and adds symmetric v2 coverage for error cases; implementation looks correct.src/rpc/methods/eth.rs (1)
36-36: EthCallV2 wiring matches EthCall semantics and is correctly scoped to v2Introducing
EthCallV2with:
type Params = (EthCallMessage, ExtBlockNumberOrHash),API_PATHS = make_bitflags!(ApiPaths::{ V2 }), and- resolution via
tipset_by_block_number_or_hash_v2,ensures v2 requests hit the new resolver while reusing the exact same execution and decoding path as
EthCall(sameMessage::try_from,apply_message, and return‑data handling, including the EAM special case). The added imports forChainGetTipSetV2andenumflags2::{BitFlags, make_bitflags}are consistent with this usage. This looks correct and non‑breaking with respect to existing v1 callers.Also applies to: 66-66, 2661-2693
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 387 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
907-927: EthCallV2 wiring and v2 tipset resolution look consistent and safe
resolve_predefined_tipset_v2correctly reusesPredefined::try_fromforearliest/pending/latestand delegatessafe/finalizedtoChainGetTipSetV2::get_latest_safe_tipsetandget_latest_finalized_tipset, aligning EthCallV2’s semantics with the newer v2 chain helpers instead of the olderSAFE_EPOCH_DELAYheuristics.- The
finalizedbranch’sunwrap_or(ctx.chain_index().tipset_by_height(0, head, ResolveNullTipset::TakeOlder)?)fallback is a reasonable “no finalized tipset yet” behavior; if you ever want that case to fail loudly instead of silently using genesis, consider turning it into an explicit error, but it’s not required for correctness.tipset_by_block_number_or_hash_v2mirrorstipset_by_ext_block_number_or_hashfor numeric and hash variants and only diverges onPredefinedBlockby calling the v2 resolver, which keeps behavior consistent for non‑predefined selectors.EthCallV2’sRpcMethod<2>impl is a near‑clone ofEthCall: same method name and alias, same params/return types except forExtBlockNumberOrHash, and identical execution / decoding logic. RestrictingAPI_PATHStoV2viamake_bitflags!(ApiPaths::{ V2 })ensures v1 behavior is untouched while exposing the new v2 semantics on the dedicated path.Overall this is a clean, low‑risk extension that keeps the call semantics aligned between v1 and v2 while letting v2 benefit from the improved safe/finalized resolution.
Also applies to: 1019-1042, 2660-2692
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)src/rpc/methods/eth.rs(5 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd/test_snapshots.txt
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
src/rpc/methods/eth.rs (7)
from_predefined(362-364)from_predefined(422-424)from_str(285-287)from_str(393-404)from_str(453-472)from_block_number(366-368)from_block_number(426-428)
src/rpc/methods/eth.rs (3)
src/rpc/methods/chain.rs (4)
chain(447-447)chain(485-485)get_latest_safe_tipset(1045-1062)get_latest_finalized_tipset(1064-1091)src/rpc/methods/eth/types.rs (9)
try_from(235-242)try_from(248-250)try_from(256-258)try_from(270-272)try_from(278-280)try_from(353-393)default(483-485)default(629-631)default(657-659)src/rpc/methods/eth/utils.rs (1)
decode_payload(63-76)
⏰ 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). (8)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Build MacOS
- GitHub Check: tests-release
- GitHub Check: Coverage
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
🔇 Additional comments (3)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1444-1474: EthCall / EthCallV2 happy‑path tests look correct and cover new tagsReusing a single
EthCallMessageper(to, data)case and feeding it to bothEthCall(withBlockNumberOrHash::from_predefined(Predefined::Latest)) andEthCallV2(withExtBlockNumberOrHash::PredefinedBlockforLatest,Safe, andFinalized) matches the respective RPC parameter types and ensures v2 coverage of the new predefined tags without changing existing v1 behavior. No issues here.
1535-1581: Error‑path coverage for EthCall and EthCallV2 is symmetric and well‑structuredThe refactored
eth_call_api_err_testsbuilds a singleEthCallMessageper contract bytecode and then issues bothEthCallandEthCallV2requests at the sameepoch(viafrom_block_numberforBlockNumberOrHashandExtBlockNumberOrHash). UsingRpcTest::identity(...).policy_on_rejected(PolicyOnRejected::PassWithIdenticalError)ensures that v1 and v2 surface identical error text for these failing contracts. This is a good, explicit expansion of coverage with clear construction of inputs.src/rpc/methods/eth.rs (1)
36-36: New imports are minimal and scoped to the EthCallV2 implementationBringing in
ChainGetTipSetV2andBitFlags/make_bitflagsis appropriate for the new v2 tipset resolution andAPI_PATHSdefinition; no unused or overly broad imports introduced.Also applies to: 66-66
There was a problem hiding this comment.
I have couple of points here:
1 - The changes are getting duplicated now, we need a change in the implementation where we can have two different resolvers which handles everything related to V1 and V2 separately and can use common functions if needed. As more V2 API are added we might have more duplicate logic.
- This is just a suggestion but we can do something like:
V1andV2types which
implements the a common interface and resolve things separately. I would like to avoid having code duplication in both V1 and V2 if possible.
2 - We should move the V2 API's to eth_v2 file or something similar, the eth.rs is already quite big and adding V2 API's there will bloat it.
Summary of changes
Changes introduced in this pull request:
Filecoin.EthCallV2 API and test snapshots.Reference issue to close (if applicable)
Closes #6290
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.