fix(RPC): backport F3 finality resolution to eth V1 methods#6644
Open
hanabi1224 wants to merge 17 commits intomainfrom
Open
fix(RPC): backport F3 finality resolution to eth V1 methods#6644hanabi1224 wants to merge 17 commits intomainfrom
hanabi1224 wants to merge 17 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBackports F3 finality resolution into ETH v1 RPCs, adds a TipsetResolver and BlockNumberOrHash flow, converts many ETH RPC handlers to resolver-based async tipset lookup, changes per-request API path to a single ApiPaths value, updates tests/snapshots, and documents an env var to toggle V1 F3 behavior. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as rgba(52,152,219,0.5)
participant Handler as rgba(46,204,113,0.5)
participant Resolver as rgba(155,89,182,0.5)
participant Chain as rgba(241,196,15,0.5)
Client->>Handler: send eth_* request (params, block_param, api_path)
Handler->>Resolver: construct BlockNumberOrHash from block_param
Resolver->>Resolver: choose resolution path (Predefined / Number / Hash) based on api_path and env toggle
alt Predefined -> needs finality
Resolver->>Chain: request predefined tipset (safe/finalized via ChainGetTipSet)
Chain-->>Resolver: Tipset
else Number or Hash
Resolver->>Chain: lookup tipset by number or hash
Chain-->>Resolver: Tipset
end
Resolver-->>Handler: resolved Tipset
Handler->>Handler: produce response using Tipset
Handler-->>Client: return RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
hanabi1224
force-pushed
the
hm/f3-await-v1-rpc
branch
from
4082f71 to
f94db17
Compare
hanabi1224
changed the title
hanabi1224
requested review from
akaladarshi and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/eth.rs (1)
2712-2718:⚠️ Potential issue | 🟠 MajorBug: V1 handler
EthGetTransactionByBlockNumberAndIndexusestipset_by_block_number_or_hash_v2instead of_v1.This method has
API_PATHS: BitFlags<ApiPaths> = ApiPaths::all()(non-V2), yet it callstipset_by_block_number_or_hash_v2. All other V1 handlers in this PR consistently use_v1. The V2 counterpart at line 2740 also correctly uses_v2. This means V1 callers of this endpoint won't respect theFOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTIONenv flag.Proposed fix
let ts = BlockNumberOrHash::from(block_param) - .tipset_by_block_number_or_hash_v2(&ctx, ResolveNullTipset::TakeOlder) + .tipset_by_block_number_or_hash_v1(&ctx, ResolveNullTipset::TakeOlder) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 2712 - 2718, The V1 handler EthGetTransactionByBlockNumberAndIndex currently calls tipset_by_block_number_or_hash_v2 which bypasses V1 feature gating; change the call in the async fn handle (the BlockNumberOrHash::from(block_param) chain) to use tipset_by_block_number_or_hash_v1 with the same ResolveNullTipset::TakeOlder and ctx so V1 callers respect FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION and match other V1 handlers.
🧹 Nitpick comments (10)
docs/docs/users/reference/env_variables.md (1)
61-61: Tighten phrasing in the description.Consider shortening to “Whether to disable …” for concision.
✍️ Suggested tweak
-| `FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION` | 1 or true | empty | 1 | Whether or not to disable F3 finality resolution in Eth v1 RPC methods | +| `FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION` | 1 or true | empty | 1 | Whether to disable F3 finality resolution in Eth v1 RPC methods |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/users/reference/env_variables.md` at line 61, Update the description for the environment variable FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION to a more concise phrasing: replace "Whether or not to disable F3 finality resolution in Eth v1 RPC methods" with "Whether to disable F3 finality resolution in Eth v1 RPC methods" in the docs table row to tighten the wording.src/rpc/methods/eth/types.rs (1)
270-275: Add a doc comment for the public enum.
BlockNumberOrPredefinedis public; please add a short doc comment describing its purpose.📝 Suggested doc comment
+/// Block selector used by eth RPCs; accepts either a predefined tag or a block number. #[derive(PartialEq, Debug, Clone, Serialize, Deserialize, JsonSchema, derive_more::From)] #[serde(untagged)] pub enum BlockNumberOrPredefined {As per coding guidelines “Document all public functions and structs with doc comments”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/types.rs` around lines 270 - 275, The public enum BlockNumberOrPredefined lacks a doc comment; add a concise Rust doc comment (///) above the enum BlockNumberOrPredefined explaining that it represents either an explicit block number (BlockNumber variant holding EthInt64) or a predefined block specifier (PredefinedBlock variant holding Predefined), and mention its serialization behavior if relevant (serde untagged) so callers understand its usage in RPC types.src/tool/subcommands/api_cmd/test_snapshots.txt (1)
143-148: Verify new snapshot artifacts are available remotely.Please confirm these new snapshot files have been uploaded to the snapshot bucket; otherwise the fetch step will fail for these test cases.
Based on learnings “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.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/test_snapshots.txt` around lines 143 - 148, The test snapshot manifest src/tool/subcommands/api_cmd/test_snapshots.txt lists several .rpcsnap.json.zst artifacts (e.g., filecoin_filecoinaddresstoethaddress_1771943760273649.rpcsnap.json.zst, _1771943760275947..., _1771943760315677..., _1771946498262342..., _1771946498262483..., _1771946498262885...) but these are not stored in-repo and must exist in the remote snapshot bucket; verify each of these exact files has been uploaded to the DigitalOcean snapshot bucket, ensure the bucket manifest/catalog used by the fetch step includes these entries, confirm object ACLs/public access or credentials permit the fetch, and if any are missing upload them (or update the manifest file referenced by the fetch logic) so the test fetch step succeeds.src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1475-1483: Minor inconsistency: usefrom_predefinedinstead of the variant directly.Every other call site in this file uses
BlockNumberOrHash::from_predefined(tag). The direct variant form works, but it exposes the internal enum structure unnecessarily.♻️ Suggested fix
- (msg.clone(), BlockNumberOrHash::PredefinedBlock(tag)), + (msg.clone(), BlockNumberOrHash::from_predefined(tag)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1475 - 1483, Replace the direct enum construction BlockNumberOrHash::PredefinedBlock(tag) with the helper constructor BlockNumberOrHash::from_predefined(tag) in the loop that builds tests (inside the for tag in [Predefined::Latest, Predefined::Safe, Predefined::Finalized] block). Update the EthCallV2::request_with_alias call used when creating RpcTest::identity so it passes BlockNumberOrHash::from_predefined(tag) instead of the PredefinedBlock variant to match other call sites and avoid exposing the enum internals.src/rpc/methods/chain.rs (2)
1049-1049: Consider simplifying with.map(Some)instead ofSome(…).transpose().Both are semantically identical —
Result<Tipset>→Result<Option<Tipset>>— but.map(Some)is more idiomatic and easier to read at a glance:- TipsetTag::Finalized => Some(Self::get_latest_finalized_tipset(ctx).await).transpose(), + TipsetTag::Finalized => Self::get_latest_finalized_tipset(ctx).await.map(Some),(The
Safearm on line 1050 uses the same existing pattern; both could be changed for consistency, or left as-is — it's a pure readability preference.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` at line 1049, The match arm using TipsetTag::Finalized currently wraps an awaited Result in Some(...).transpose(); replace that pattern by mapping the Result to an Option via .map(Some) on the async call (i.e., change the expression involving get_latest_finalized_tipset(ctx).await to get_latest_finalized_tipset(ctx).await.map(Some)); do the same for the Safe arm that uses get_latest_safe_tipset(ctx).await so both arms use the more idiomatic .map(Some) conversion from Result<Tipset> to Result<Option<Tipset>>.
1071-1077: New public function is missing a doc comment (required by coding guidelines).A brief description clarifying this is the EC-only (non-F3) safe-tipset path would also aid consumers in
eth.rs.📝 Suggested doc comment
+ /// Returns the EC-based safe tipset, i.e. the tipset at [`SAFE_HEIGHT_DISTANCE`] epochs + /// behind the current head, without consulting F3. Use [`get_latest_safe_tipset`] when + /// F3 finality should be preferred. pub fn get_ec_safe_tipset(ctx: &Ctx<impl Blockstore>) -> anyhow::Result<Tipset> {As per coding guidelines: "Document all public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1071 - 1077, Add a Rust doc comment (///) to the public function get_ec_safe_tipset describing its purpose and behaviour: state that it returns the EC-only (non-F3) safe tipset by computing safe_height = head.epoch() - SAFE_HEIGHT_DISTANCE and resolving the tipset at that height with ResolveNullTipset::TakeOlder; note that this is the EC-only safe-tipset path (not F3) and mention that consumers like eth.rs rely on this contract. Ensure the comment sits immediately above the get_ec_safe_tipset declaration and follows crate doc style and tone.src/rpc/methods/eth.rs (4)
362-377:resolve_common_predefined_tipsetreturnsOk(None)forSafeandFinalized, delegating to callers.The wildcard arm
_ => Ok(None)matchesSafeandFinalized, which is correct since these are handled by the caller (resolve_predefined_tipset_v1/_v2). However, if new variants are ever added toPredefined, they'd silently fall through here too.Consider using an explicit match instead of the wildcard to get a compile-time reminder if new variants are added:
Suggested change
- _ => Ok(None), + Predefined::Safe | Predefined::Finalized => Ok(None),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 362 - 377, The match in resolve_common_predefined_tipset currently uses a wildcard arm that returns Ok(None), which will silently accept any future Predefined variants; change the match to explicitly list all known variants (Earliest, Pending, Latest, Safe, Finalized) and return Ok(None) for Safe and Finalized so the compiler will error if a new variant is added, updating the match in the resolve_common_predefined_tipset function to remove the `_ => Ok(None)` arm and replace it with explicit arms for Safe and Finalized (and keep the existing behavior for Earliest, Pending, Latest).
439-452:BlockNumberOrHash::from_strshadowsFromStr::from_str.The method
from_stris defined as an inherent method (not implementing theFromStrtrait), which silently shadows any potentialFromStrtrait implementation. This is fine functionally, but ifFromStris ever derived or implemented, the inherent method will always take precedence, which could cause confusion.Also, the addition of
"safe"and"finalized"parsing at lines 444-445 is correct and aligns with the PR objective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 439 - 452, The current inherent method BlockNumberOrHash::from_str shadows the standard FromStr trait; replace it by implementing std::str::FromStr for BlockNumberOrHash (impl FromStr for BlockNumberOrHash { type Err = Error; fn from_str(&str) -> Result<Self, Self::Err> { ... } }) reusing the existing logic (keep the "earliest", "pending", "latest"/"", "safe", "finalized" branches and the hex branch that calls hex_str_to_epoch and then from_block_number/from_predefined); remove or rename the original inherent from_str to avoid shadowing (e.g., rename to parse_str) so there is no ambiguity between the trait impl and an inherent method.
3857-3865:EthTraceCallserves V1 and V2 but always uses_v2resolution.This single handler serves both
V1 | V2paths. Usingtipset_by_block_number_or_hash_v2means V1 callers passing"safe"or"finalized"asblock_paramwill get F3-based resolution regardless ofFOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION. The default case ("latest") is unaffected since both paths resolve it identically.If the intent is to respect the env flag for V1, consider either splitting into V1/V2 handlers (like most other methods in this PR) or passing the API version dynamically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3857 - 3865, The handler for EthTraceCall always uses tipset_by_block_number_or_hash_v2, causing V1 callers using BlockNumberOrHash values like "safe" or "finalized" to get F3 resolution regardless of FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION; update the handler to respect API version by branching on the request version (or split into separate V1/V2 handlers): if the caller is V1 use tipset_by_block_number_or_hash (or the legacy resolver) when block_param is "safe"|"finalized", otherwise use tipset_by_block_number_or_hash_v2 for V2; adjust the code around EthTraceCall::handle, the BlockNumberOrHash parsing, and the call to tipset_by_block_number_or_hash_v2 (and ResolveNullTipset::TakeOlder) so V1 flows consult the env flag and legacy resolver while V2 continues to use the _v2 resolver.
3186-3191:FilecoinAddressToEthAddressdefaults toPredefined::Finalizedand always uses_v2.This endpoint is served on
all_with_v2()but always resolves viatipset_by_block_number_or_hash_v2. V1 callers (who omitblock_param) will always get F3-finalized resolution, bypassing theFOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTIONopt-out. Same concern asEthTraceCall– this may be intentional since there's no separate V2 handler, but it's inconsistent with the V1 backport pattern used elsewhere in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3186 - 3191, The FilecoinAddressToEthAddress handler currently defaults block_param to Predefined::Finalized and always calls tipset_by_block_number_or_hash_v2, causing V1 callers (served via all_with_v2()) to be forced to F3-finalized resolution and bypass the FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION opt-out; update the logic in the FilecoinAddressToEthAddress handler to detect V1 requests (as other V1 backports do) and: if V1 and the FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION flag is set, avoid defaulting to Predefined::Finalized and call the non-_v2 resolver (tipset_by_block_number_or_hash or the v1 variant) instead; otherwise keep the existing v2 path (leave the block_param default and call tipset_by_block_number_or_hash_v2) so v2 behavior remains unchanged. Reference symbols: FilecoinAddressToEthAddress, block_param, Predefined::Finalized, tipset_by_block_number_or_hash_v2, all_with_v2(), and FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2712-2718: The V1 handler EthGetTransactionByBlockNumberAndIndex
currently calls tipset_by_block_number_or_hash_v2 which bypasses V1 feature
gating; change the call in the async fn handle (the
BlockNumberOrHash::from(block_param) chain) to use
tipset_by_block_number_or_hash_v1 with the same ResolveNullTipset::TakeOlder and
ctx so V1 callers respect FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION and match
other V1 handlers.
---
Nitpick comments:
In `@docs/docs/users/reference/env_variables.md`:
- Line 61: Update the description for the environment variable
FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION to a more concise phrasing: replace
"Whether or not to disable F3 finality resolution in Eth v1 RPC methods" with
"Whether to disable F3 finality resolution in Eth v1 RPC methods" in the docs
table row to tighten the wording.
In `@src/rpc/methods/chain.rs`:
- Line 1049: The match arm using TipsetTag::Finalized currently wraps an awaited
Result in Some(...).transpose(); replace that pattern by mapping the Result to
an Option via .map(Some) on the async call (i.e., change the expression
involving get_latest_finalized_tipset(ctx).await to
get_latest_finalized_tipset(ctx).await.map(Some)); do the same for the Safe arm
that uses get_latest_safe_tipset(ctx).await so both arms use the more idiomatic
.map(Some) conversion from Result<Tipset> to Result<Option<Tipset>>.
- Around line 1071-1077: Add a Rust doc comment (///) to the public function
get_ec_safe_tipset describing its purpose and behaviour: state that it returns
the EC-only (non-F3) safe tipset by computing safe_height = head.epoch() -
SAFE_HEIGHT_DISTANCE and resolving the tipset at that height with
ResolveNullTipset::TakeOlder; note that this is the EC-only safe-tipset path
(not F3) and mention that consumers like eth.rs rely on this contract. Ensure
the comment sits immediately above the get_ec_safe_tipset declaration and
follows crate doc style and tone.
In `@src/rpc/methods/eth.rs`:
- Around line 362-377: The match in resolve_common_predefined_tipset currently
uses a wildcard arm that returns Ok(None), which will silently accept any future
Predefined variants; change the match to explicitly list all known variants
(Earliest, Pending, Latest, Safe, Finalized) and return Ok(None) for Safe and
Finalized so the compiler will error if a new variant is added, updating the
match in the resolve_common_predefined_tipset function to remove the `_ =>
Ok(None)` arm and replace it with explicit arms for Safe and Finalized (and keep
the existing behavior for Earliest, Pending, Latest).
- Around line 439-452: The current inherent method BlockNumberOrHash::from_str
shadows the standard FromStr trait; replace it by implementing std::str::FromStr
for BlockNumberOrHash (impl FromStr for BlockNumberOrHash { type Err = Error; fn
from_str(&str) -> Result<Self, Self::Err> { ... } }) reusing the existing logic
(keep the "earliest", "pending", "latest"/"", "safe", "finalized" branches and
the hex branch that calls hex_str_to_epoch and then
from_block_number/from_predefined); remove or rename the original inherent
from_str to avoid shadowing (e.g., rename to parse_str) so there is no ambiguity
between the trait impl and an inherent method.
- Around line 3857-3865: The handler for EthTraceCall always uses
tipset_by_block_number_or_hash_v2, causing V1 callers using BlockNumberOrHash
values like "safe" or "finalized" to get F3 resolution regardless of
FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION; update the handler to respect API
version by branching on the request version (or split into separate V1/V2
handlers): if the caller is V1 use tipset_by_block_number_or_hash (or the legacy
resolver) when block_param is "safe"|"finalized", otherwise use
tipset_by_block_number_or_hash_v2 for V2; adjust the code around
EthTraceCall::handle, the BlockNumberOrHash parsing, and the call to
tipset_by_block_number_or_hash_v2 (and ResolveNullTipset::TakeOlder) so V1 flows
consult the env flag and legacy resolver while V2 continues to use the _v2
resolver.
- Around line 3186-3191: The FilecoinAddressToEthAddress handler currently
defaults block_param to Predefined::Finalized and always calls
tipset_by_block_number_or_hash_v2, causing V1 callers (served via all_with_v2())
to be forced to F3-finalized resolution and bypass the
FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION opt-out; update the logic in the
FilecoinAddressToEthAddress handler to detect V1 requests (as other V1 backports
do) and: if V1 and the FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION flag is set,
avoid defaulting to Predefined::Finalized and call the non-_v2 resolver
(tipset_by_block_number_or_hash or the v1 variant) instead; otherwise keep the
existing v2 path (leave the block_param default and call
tipset_by_block_number_or_hash_v2) so v2 behavior remains unchanged. Reference
symbols: FilecoinAddressToEthAddress, block_param, Predefined::Finalized,
tipset_by_block_number_or_hash_v2, all_with_v2(), and
FOREST_ETH_V1_DISABLE_F3_FINALITY_RESOLUTION.
In `@src/rpc/methods/eth/types.rs`:
- Around line 270-275: The public enum BlockNumberOrPredefined lacks a doc
comment; add a concise Rust doc comment (///) above the enum
BlockNumberOrPredefined explaining that it represents either an explicit block
number (BlockNumber variant holding EthInt64) or a predefined block specifier
(PredefinedBlock variant holding Predefined), and mention its serialization
behavior if relevant (serde untagged) so callers understand its usage in RPC
types.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1475-1483: Replace the direct enum construction
BlockNumberOrHash::PredefinedBlock(tag) with the helper constructor
BlockNumberOrHash::from_predefined(tag) in the loop that builds tests (inside
the for tag in [Predefined::Latest, Predefined::Safe, Predefined::Finalized]
block). Update the EthCallV2::request_with_alias call used when creating
RpcTest::identity so it passes BlockNumberOrHash::from_predefined(tag) instead
of the PredefinedBlock variant to match other call sites and avoid exposing the
enum internals.
In `@src/tool/subcommands/api_cmd/test_snapshots.txt`:
- Around line 143-148: The test snapshot manifest
src/tool/subcommands/api_cmd/test_snapshots.txt lists several .rpcsnap.json.zst
artifacts (e.g.,
filecoin_filecoinaddresstoethaddress_1771943760273649.rpcsnap.json.zst,
_1771943760275947..., _1771943760315677..., _1771946498262342...,
_1771946498262483..., _1771946498262885...) but these are not stored in-repo and
must exist in the remote snapshot bucket; verify each of these exact files has
been uploaded to the DigitalOcean snapshot bucket, ensure the bucket
manifest/catalog used by the fetch step includes these entries, confirm object
ACLs/public access or credentials permit the fetch, and if any are missing
upload them (or update the manifest file referenced by the fetch logic) so the
test fetch step succeeds.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mddocs/dictionary.txtdocs/docs/users/reference/env_variables.mdsrc/eth/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/types.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/test_snapshot.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
💤 Files with no reviewable changes (1)
- src/eth/mod.rs
akaladarshi
reviewed
Feb 25, 2026
Collaborator
There was a problem hiding this comment.
@hanabi1224 Couple of things here, I think there’s an opportunity to simplify the design:
Right now it feels a bit complex, with a lot of functions around enum variants. I wonder if we can centralise some of this logic.
-
Consider introducing a
tipsetResolvehere which can help us resolve all the enum variants based on theenvandparams. -
We could move the F3/EC related tag resolution to the
tipsetResolver, keepingChainGetTipSetV2focused purely on API responsibilities. -
It would be good to structure this in a way that avoids unnecessary refactors once
ETH_V1_DISABLE_F3_FINALITY_RESOLUTION_ENV_KEYis removed: filecoin-project/lotus#13315 -
EthGetTransactionByBlockNumberAndIndexstill uses thetipset_by_block_number_or_hash_v2.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/chain.rs (1)
1129-1139:⚠️ Potential issue | 🟠 MajorFallback to EC finality is missing on F3 tipset load errors.
At Line 1129, failure to load the F3 finalized tipset becomes a hard error. That diverges from
ChainGetFinalizedTipset, which falls back to EC finality when F3 resolution fails.Suggested fix
- let ts = ctx - .chain_index() - .load_required_tipset(&f3_finalized_head.key) - .map_err(|e| { - anyhow::anyhow!( - "Failed to load F3 finalized tipset at epoch {} with key {}: {e}", - f3_finalized_head.epoch, - f3_finalized_head.key, - ) - })?; - Ok(ts) + match ctx.chain_index().load_required_tipset(&f3_finalized_head.key) { + Ok(ts) => Ok(ts), + Err(e) => { + tracing::warn!( + "Failed to load F3 finalized tipset at epoch {} with key {}: {e}; falling back to EC finality", + f3_finalized_head.epoch, + f3_finalized_head.key + ); + Self::get_ec_finalized_tipset(ctx) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1129 - 1139, The code currently treats failures to load the F3 finalized tipset (the call to ctx.chain_index().load_required_tipset(&f3_finalized_head.key)) as a hard error; change this to fall back to EC finality like ChainGetFinalizedTipset does by catching the error from loading the F3 tipset, logging the failure (including the original error and f3_finalized_head info), and then attempting to load the EC tipset via ctx.chain_index().load_required_tipset(&ec_finalized_head.key); if the EC load succeeds return that tipset, otherwise return the combined/last error. Ensure you reference f3_finalized_head, ec_finalized_head, and ctx.chain_index().load_required_tipset in the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3233-3238: The shared V1/V2 handler is forcing V2 tipset
resolution by calling
BlockNumberOrHash::from(...).tipset_by_block_number_or_hash_v2(...); replace
that call with the non-_v2 resolver (e.g., tipset_by_block_number_or_hash) so
resolution follows the request API path and respects V1 finality toggles; update
both occurrences (the call at
BlockNumberOrHash::from(...).tipset_by_block_number_or_hash_v2(&ctx,
ResolveNullTipset::TakeOlder).await? and the similar call in the other block) to
use the shared resolver and keep the same ResolveNullTipset argument unless a
different policy is required by the request path.
- Around line 2748-2750: The V1 handler EthGetTransactionByBlockNumberAndIndex
is incorrectly calling tipset_by_block_number_or_hash_v2 (which uses V2
semantics); change the call on the BlockNumberOrHash conversion path so it
invokes the V1 resolver (tipset_by_block_number_or_hash) instead and preserve
the original ResolveNullTipset behavior for the V1 flow (ensure the same
ResolveNullTipset::TakeOlder argument is passed and error handling remains
unchanged for the ts variable resolution).
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1129-1139: The code currently treats failures to load the F3
finalized tipset (the call to
ctx.chain_index().load_required_tipset(&f3_finalized_head.key)) as a hard error;
change this to fall back to EC finality like ChainGetFinalizedTipset does by
catching the error from loading the F3 tipset, logging the failure (including
the original error and f3_finalized_head info), and then attempting to load the
EC tipset via ctx.chain_index().load_required_tipset(&ec_finalized_head.key); if
the EC load succeeds return that tipset, otherwise return the combined/last
error. Ensure you reference f3_finalized_head, ec_finalized_head, and
ctx.chain_index().load_required_tipset in the fix.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/rpc/methods/eth.rs (2)
2722-2724:⚠️ Potential issue | 🟠 MajorUse V1 tipset resolution in the V1 transaction-by-block handler.
Line 2723 still calls
tipset_by_block_number_or_hash_v2insideEthGetTransactionByBlockNumberAndIndex(V1 API path set), which bypasses V1-specific finality behavior.Suggested fix
- let ts = BlockNumberOrHash::from(block_param) - .tipset_by_block_number_or_hash_v2(&ctx, ResolveNullTipset::TakeOlder) + let ts = BlockNumberOrHash::from(block_param) + .tipset_by_block_number_or_hash_v1(&ctx, ResolveNullTipset::TakeOlder) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 2722 - 2724, In EthGetTransactionByBlockNumberAndIndex, the V1 handler currently calls BlockNumberOrHash::from(...).tipset_by_block_number_or_hash_v2(...), which uses V2 finality rules; replace the call to use the V1 resolver by invoking tipset_by_block_number_or_hash(...) instead (keep the same ResolveNullTipset::TakeOlder and ctx/await usage) so the V1 path uses the V1 tipset resolution semantics.
3896-3904:⚠️ Potential issue | 🟠 MajorShared
EthTraceCallstill hardcodes V2 resolution.Line 3903 always uses V2 resolution even though this method serves both V1 and V2 paths, so V1 calls do not honor V1 semantics.
Suggested fix
- async fn handle( - ctx: Ctx<impl Blockstore + Send + Sync + 'static>, - (tx, trace_types, block_param): Self::Params, - _: &http::Extensions, - ) -> Result<Self::Ok, ServerError> { + async fn handle( + ctx: Ctx<impl Blockstore + Send + Sync + 'static>, + (tx, trace_types, block_param): Self::Params, + ext: &http::Extensions, + ) -> Result<Self::Ok, ServerError> { @@ - let ts = block_param - .tipset_by_block_number_or_hash_v2(&ctx, ResolveNullTipset::TakeOlder) - .await?; + let ts = block_param + .tipset_by_block_number_or_hash( + &ctx, + ResolveNullTipset::TakeOlder, + Self::api_path(ext)?, + ) + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3896 - 3904, The handler always calls tipset_by_block_number_or_hash_v2 which forces V2 semantics for all requests; change it to pick the resolution function based on the incoming trace version by checking the trace_types parameter (e.g., inspect trace_types or its discriminator) and call tipset_by_block_number_or_hash (V1) for V1 requests and tipset_by_block_number_or_hash_v2 (V2) for V2 requests, preserving the appropriate ResolveNullTipset behavior for each path (e.g., keep ResolveNullTipset::TakeOlder for V2 and the V1 semantics for V1).
🧹 Nitpick comments (5)
src/rpc/request.rs (1)
35-42: Document new public API-path mutators.
set_api_pathandwith_api_pathare public and should have doc comments.As per coding guidelines, "Document all public functions and structs with doc comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/request.rs` around lines 35 - 42, Add doc comments for the public methods set_api_path and with_api_path: document that set_api_path(&mut self, api_path: ApiPaths) sets the request's API path to the provided ApiPaths value and mention mutates self, and document that with_api_path(self, api_path: ApiPaths) -> Self is a builder-style convenience that sets the API path and returns the modified Request for chaining; include parameter descriptions for api_path and a brief example or usage note if appropriate. Ensure the comments are placed immediately above the function signatures for set_api_path and with_api_path and follow the crate's doc-comment style (///).src/rpc/reflect/mod.rs (1)
367-371: Document the newapi_pathhelper inRpcMethodExt.This helper is part of the public trait surface and should include rustdoc.
As per coding guidelines, "Document all public functions and structs with doc comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/reflect/mod.rs` around lines 367 - 371, Add a rustdoc comment for the new api_path helper in the RpcMethodExt trait: above fn api_path(ext: &http::Extensions) -> anyhow::Result<ApiPaths> add a short summary of what the helper does, document the `ext` parameter (that it reads ApiPaths from http::Extensions), state the return value (Ok(ApiPaths) or Err on missing/copy failure) and mention the error context ("failed to resolve api path") so callers understand failure modes; ensure the doc follows crate guidelines for public trait items and includes examples or usage notes if helpful.src/rpc/methods/eth.rs (1)
307-373: Add rustdoc for newly public resolver APIs.Lines 308, 319, 343, 358, 404, 450, 459, and 468 introduce public methods without doc comments.
As per coding guidelines, "Document all public functions and structs with doc comments."
Also applies to: 402-473
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 307 - 373, Add doc comments for the newly public resolver APIs: impl Predefined methods resolve_predefined_tipset, resolve_predefined_tipset_v1, resolve_predefined_tipset_v2, and resolve_common_predefined_tipset (and any other public functions in this impl) describing their purpose, parameters (ctx, api_version or chain_store), behavior (how V1 vs V2 resolution works, env toggle for F3 finality, and what each Predefined variant returns or errors), and the returned Tipset/result. Keep comments brief, follow existing crate doc style (///), include examples or panics/errors only if helpful, and ensure every public function mentioned in this diff has a top-level rustdoc line.src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1475-1483: UseBlockNumberOrHash::from_predefined(tag)instead of the direct variant constructor.Line 1478 constructs
BlockNumberOrHash::PredefinedBlock(tag)directly, while the rest of the file consistently uses thefrom_predefinedconstructor (see lines 1408, 1468, 1640, etc.). Thefrom_predefinedfactory method is the public API persrc/rpc/methods/eth/types.rs:403-405.♻️ Proposed fix
- (msg.clone(), BlockNumberOrHash::PredefinedBlock(tag)), + (msg.clone(), BlockNumberOrHash::from_predefined(tag)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1475 - 1483, Replace the direct construction BlockNumberOrHash::PredefinedBlock(tag) with the public factory BlockNumberOrHash::from_predefined(tag) inside the loop that builds RpcTest::identity entries; update the EthCallV2::request_with_alias call site (where (msg.clone(), BlockNumberOrHash::PredefinedBlock(tag)) is passed) to use BlockNumberOrHash::from_predefined(tag) so it matches the rest of the file and uses the public API for Predefined variants.
1475-1483:EthCallV2loop skipsPredefined::EarliestandPredefined::Pendingcoverage.The loop tests only
[Latest, Safe, Finalized]. The siblingEthCalltest at lines 1464–1473 covers onlyLatest. Given the PR's goal of backportingsafe/finalizedsupport, addingPendingto the V2 loop (with an appropriatePassWithQuasiIdenticalErrorpolicy) would match coverage parity with other V2 methods (e.g.,EthGetBalanceV2at lines 1908–1933 which tests all five predefined tags).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1475 - 1483, The EthCallV2 loop currently iterates only over [Predefined::Latest, Predefined::Safe, Predefined::Finalized], skipping Predefined::Earliest and Predefined::Pending; update the loop to include both Predefined::Earliest and Predefined::Pending so V2 test coverage matches other V2 methods, and for the Pending case push the test using the PassWithQuasiIdenticalError policy (instead of RpcTest::identity) to account for quasi-identical error behavior; locate the loop building RpcTest entries for EthCallV2 and add entries for Predefined::Earliest and Predefined::Pending (with Pending wrapped by the PassWithQuasiIdenticalError helper) while keeping the existing use of EthCallV2::request_with_alias and RpcTest constructors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2722-2724: In EthGetTransactionByBlockNumberAndIndex, the V1
handler currently calls
BlockNumberOrHash::from(...).tipset_by_block_number_or_hash_v2(...), which uses
V2 finality rules; replace the call to use the V1 resolver by invoking
tipset_by_block_number_or_hash(...) instead (keep the same
ResolveNullTipset::TakeOlder and ctx/await usage) so the V1 path uses the V1
tipset resolution semantics.
- Around line 3896-3904: The handler always calls
tipset_by_block_number_or_hash_v2 which forces V2 semantics for all requests;
change it to pick the resolution function based on the incoming trace version by
checking the trace_types parameter (e.g., inspect trace_types or its
discriminator) and call tipset_by_block_number_or_hash (V1) for V1 requests and
tipset_by_block_number_or_hash_v2 (V2) for V2 requests, preserving the
appropriate ResolveNullTipset behavior for each path (e.g., keep
ResolveNullTipset::TakeOlder for V2 and the V1 semantics for V1).
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 307-373: Add doc comments for the newly public resolver APIs: impl
Predefined methods resolve_predefined_tipset, resolve_predefined_tipset_v1,
resolve_predefined_tipset_v2, and resolve_common_predefined_tipset (and any
other public functions in this impl) describing their purpose, parameters (ctx,
api_version or chain_store), behavior (how V1 vs V2 resolution works, env toggle
for F3 finality, and what each Predefined variant returns or errors), and the
returned Tipset/result. Keep comments brief, follow existing crate doc style
(///), include examples or panics/errors only if helpful, and ensure every
public function mentioned in this diff has a top-level rustdoc line.
In `@src/rpc/reflect/mod.rs`:
- Around line 367-371: Add a rustdoc comment for the new api_path helper in the
RpcMethodExt trait: above fn api_path(ext: &http::Extensions) ->
anyhow::Result<ApiPaths> add a short summary of what the helper does, document
the `ext` parameter (that it reads ApiPaths from http::Extensions), state the
return value (Ok(ApiPaths) or Err on missing/copy failure) and mention the error
context ("failed to resolve api path") so callers understand failure modes;
ensure the doc follows crate guidelines for public trait items and includes
examples or usage notes if helpful.
In `@src/rpc/request.rs`:
- Around line 35-42: Add doc comments for the public methods set_api_path and
with_api_path: document that set_api_path(&mut self, api_path: ApiPaths) sets
the request's API path to the provided ApiPaths value and mention mutates self,
and document that with_api_path(self, api_path: ApiPaths) -> Self is a
builder-style convenience that sets the API path and returns the modified
Request for chaining; include parameter descriptions for api_path and a brief
example or usage note if appropriate. Ensure the comments are placed immediately
above the function signatures for set_api_path and with_api_path and follow the
crate's doc-comment style (///).
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1475-1483: Replace the direct construction
BlockNumberOrHash::PredefinedBlock(tag) with the public factory
BlockNumberOrHash::from_predefined(tag) inside the loop that builds
RpcTest::identity entries; update the EthCallV2::request_with_alias call site
(where (msg.clone(), BlockNumberOrHash::PredefinedBlock(tag)) is passed) to use
BlockNumberOrHash::from_predefined(tag) so it matches the rest of the file and
uses the public API for Predefined variants.
- Around line 1475-1483: The EthCallV2 loop currently iterates only over
[Predefined::Latest, Predefined::Safe, Predefined::Finalized], skipping
Predefined::Earliest and Predefined::Pending; update the loop to include both
Predefined::Earliest and Predefined::Pending so V2 test coverage matches other
V2 methods, and for the Pending case push the test using the
PassWithQuasiIdenticalError policy (instead of RpcTest::identity) to account for
quasi-identical error behavior; locate the loop building RpcTest entries for
EthCallV2 and add entries for Predefined::Earliest and Predefined::Pending (with
Pending wrapped by the PassWithQuasiIdenticalError helper) while keeping the
existing use of EthCallV2::request_with_alias and RpcTest constructors.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/rpc/client.rssrc/rpc/methods/eth.rssrc/rpc/mod.rssrc/rpc/reflect/mod.rssrc/rpc/request.rssrc/tool/subcommands/api_cmd.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
💤 Files with no reviewable changes (1)
- src/rpc/mod.rs
hanabi1224
force-pushed
the
hm/f3-await-v1-rpc
branch
from
71e3403 to
9e831f9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/chain.rs (1)
1121-1131:⚠️ Potential issue | 🟠 MajorMissing EC fallback when F3 tipset load fails can break finalized/safe RPCs.
At Line 1123, a
load_required_tipsetfailure is returned directly. That differs fromChainGetFinalizedTipset::handle, which falls back to EC finality on F3 failures. This can surface avoidable errors for finalized/safe lookups even when EC fallback is available.Suggested fix
- let ts = ctx - .chain_index() - .load_required_tipset(&f3_finalized_head.key) - .map_err(|e| { - anyhow::anyhow!( - "Failed to load F3 finalized tipset at epoch {} with key {}: {e}", - f3_finalized_head.epoch, - f3_finalized_head.key, - ) - })?; - Ok(ts) + match ctx.chain_index().load_required_tipset(&f3_finalized_head.key) { + Ok(ts) => Ok(ts), + Err(e) => { + tracing::warn!( + "Failed to load F3 finalized tipset at epoch {} with key {}: {e}; \ + falling back to EC finality", + f3_finalized_head.epoch, + f3_finalized_head.key, + ); + Self::get_ec_finalized_tipset(ctx) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1121 - 1131, The current direct failure return from ctx.chain_index().load_required_tipset(&f3_finalized_head.key) can abort finalized/safe RPCs; instead, catch the error and invoke the same EC-finality fallback used by ChainGetFinalizedTipset::handle: on load_required_tipset error for f3_finalized_head, call the ChainGetFinalizedTipset::handle's EC-path (the code that computes/loads the EC-finalized tipset from ctx/chain_index) and return that tipset if it succeeds; only propagate an error if both the primary load_required_tipset and the EC fallback fail—also include contextual logging when falling back for easier debugging.src/rpc/methods/eth.rs (1)
299-373:⚠️ Potential issue | 🟡 MinorPublic block-selector API additions need rustdoc.
Predefined,BlockNumberOrHash, and newly added public constructors/parser in this changed section are public API surface but undocumented.As per coding guidelines "
**/*.rs: Document all public functions and structs with doc comments".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 299 - 373, Add rustdoc comments to the public API items introduced in this diff: document the enum Predefined, the structs BlockNumber and BlockHash, the enum BlockNumberOrHash, and all public constructors/parsers (from_predefined, from_block_number, from_block_hash, from_block_number_object, from_block_hash_object, from_str) with brief descriptions of purpose, behavior, parameters and return values; place /// doc comments above each type and method (mention EIP-1898 where relevant for from_block_number_object/from_block_hash_object and note require_canonical semantics for BlockHash and from_block_hash_object) so the public block-selector API is fully documented per guidelines.
🧹 Nitpick comments (2)
src/rpc/methods/eth/tipset_resolver.rs (1)
19-19: Avoid duplicatingSAFE_HEIGHT_DISTANCEacross RPC modules.Line 19 defines the same constant already present in
src/rpc/methods/chain.rs(Line 71). Keeping two copies risks silent behavioral drift between ETH and Chain safe-tipset logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/tipset_resolver.rs` at line 19, Duplicate constant SAFE_HEIGHT_DISTANCE should be removed from tipset_resolver.rs and the code should reference the single source-of-truth constant defined in the chain module instead; locate the SAFE_HEIGHT_DISTANCE symbol in tipset_resolver.rs, delete its local definition, and update any usages to import or refer to the existing SAFE_HEIGHT_DISTANCE from the chain module (e.g., use the chain module's SAFE_HEIGHT_DISTANCE constant) so ETH and Chain logic share the same value.src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1600-1708: Factor repeated ETH request matrices into a helper to reduce drift.This block duplicates near-identical
EthGetBalancecases across explicit-path and default-path variants. A small helper that accepts(api_path, block_selector)would make future updates much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1600 - 1708, Refactor the repeated EthGetBalance test entries by extracting a helper (e.g., build_eth_get_balance_tests) that takes parameters (api_path: Option<ApiPaths>, block_selector: BlockNumberOrHash, policy: Option<PolicyOnRejected>, test_kind: enum {Identity,Basic}) and returns the appropriate RpcTest(s) using EthGetBalance::request, so you replace the duplicated RpcTest::identity(...) and RpcTest::basic(...) calls with calls to this helper; ensure it handles both with_api_path(ApiPaths::V1) and default-path variants, the various BlockNumberOrHash constructors (from_block_number, from_block_number_object, from_block_hash_object, from_predefined, etc.), and preserves policy_on_rejected where used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3029-3034: The FilecoinAddressToEthAddress handler currently
defaults block_param to Predefined::Finalized which differs from EthTraceCall's
Predefined::Latest and affects lookup_required_id() state visibility; either
change the default to Predefined::Latest (modify the block_param unwrap_or in
FilecoinAddressToEthAddress to use Predefined::Latest and keep
resolver.tipset_by_block_number_or_hash calls intact) to match other RPCs like
EthTraceCall, or if Finalized is deliberate, add a concise comment above the
FilecoinAddressToEthAddress logic documenting why lookup_required_id() requires
a finalized tipset (mention lookup_required_id() and
resolver.tipset_by_block_number_or_hash) so the design rationale is clear.
In `@src/rpc/methods/eth/tipset_resolver.rs`:
- Around line 21-35: Add Rustdoc comments for the public TipsetResolver API:
document the TipsetResolver struct and its public methods (notably
TipsetResolver::new) describing purpose, parameters (ctx: &Ctx<DB>, api_version:
ApiPaths), lifetime and trait bounds (DB: Blockstore + Send + Sync + 'static),
and example usage or notes about thread-safety/ownership where relevant; ensure
you update docs for any other public functions in the same file (lines ~37-139)
so all exported items have doc comments explaining behavior, inputs, outputs,
and panics/errors.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1121-1131: The current direct failure return from
ctx.chain_index().load_required_tipset(&f3_finalized_head.key) can abort
finalized/safe RPCs; instead, catch the error and invoke the same EC-finality
fallback used by ChainGetFinalizedTipset::handle: on load_required_tipset error
for f3_finalized_head, call the ChainGetFinalizedTipset::handle's EC-path (the
code that computes/loads the EC-finalized tipset from ctx/chain_index) and
return that tipset if it succeeds; only propagate an error if both the primary
load_required_tipset and the EC fallback fail—also include contextual logging
when falling back for easier debugging.
In `@src/rpc/methods/eth.rs`:
- Around line 299-373: Add rustdoc comments to the public API items introduced
in this diff: document the enum Predefined, the structs BlockNumber and
BlockHash, the enum BlockNumberOrHash, and all public constructors/parsers
(from_predefined, from_block_number, from_block_hash, from_block_number_object,
from_block_hash_object, from_str) with brief descriptions of purpose, behavior,
parameters and return values; place /// doc comments above each type and method
(mention EIP-1898 where relevant for
from_block_number_object/from_block_hash_object and note require_canonical
semantics for BlockHash and from_block_hash_object) so the public block-selector
API is fully documented per guidelines.
---
Nitpick comments:
In `@src/rpc/methods/eth/tipset_resolver.rs`:
- Line 19: Duplicate constant SAFE_HEIGHT_DISTANCE should be removed from
tipset_resolver.rs and the code should reference the single source-of-truth
constant defined in the chain module instead; locate the SAFE_HEIGHT_DISTANCE
symbol in tipset_resolver.rs, delete its local definition, and update any usages
to import or refer to the existing SAFE_HEIGHT_DISTANCE from the chain module
(e.g., use the chain module's SAFE_HEIGHT_DISTANCE constant) so ETH and Chain
logic share the same value.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1600-1708: Refactor the repeated EthGetBalance test entries by
extracting a helper (e.g., build_eth_get_balance_tests) that takes parameters
(api_path: Option<ApiPaths>, block_selector: BlockNumberOrHash, policy:
Option<PolicyOnRejected>, test_kind: enum {Identity,Basic}) and returns the
appropriate RpcTest(s) using EthGetBalance::request, so you replace the
duplicated RpcTest::identity(...) and RpcTest::basic(...) calls with calls to
this helper; ensure it handles both with_api_path(ApiPaths::V1) and default-path
variants, the various BlockNumberOrHash constructors (from_block_number,
from_block_number_object, from_block_hash_object, from_predefined, etc.), and
preserves policy_on_rejected where used.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/tipset_resolver.rssrc/rpc/mod.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
💤 Files with no reviewable changes (1)
- src/rpc/mod.rs
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Contributor
|
Note Docstrings generation - SUCCESS |
coderabbitai bot
added a commit
that referenced
this pull request
Feb 25, 2026
Docstrings generation was requested by @hanabi1224. * #6644 (comment) The following files were modified: * `src/rpc/client.rs` * `src/rpc/methods/chain.rs` * `src/rpc/methods/eth.rs` * `src/rpc/methods/eth/tipset_resolver.rs` * `src/rpc/methods/eth/types.rs` * `src/rpc/reflect/mod.rs` * `src/rpc/request.rs` * `src/tool/subcommands/api_cmd.rs` * `src/tool/subcommands/api_cmd/api_compare_tests.rs` * `src/tool/subcommands/api_cmd/test_snapshot.rs`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/chain.rs (1)
1090-1142:⚠️ Potential issue | 🟡 MinorDocument the changed public tipset helpers.
The changed public methods (
get_latest_safe_tipset,get_latest_finalized_tipset,get_ec_finalized_tipset) need rustdoc describing fallback behavior and guarantees.As per coding guidelines, "Document all public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1090 - 1142, Add rustdoc comments (///) above the three public functions get_latest_safe_tipset, get_latest_finalized_tipset, and get_ec_finalized_tipset that explain their purpose, inputs and return value, the fallback behavior (get_latest_finalized_tipset falls back to get_ec_finalized_tipset when F3 data is unavailable or too old; get_latest_safe_tipset returns the F3-finalized tipset when newer than safe height otherwise returns a tipset at safe_height), the guarantees (what "finalized" or "safe" means relative to chain_finality and SAFE_HEIGHT_DISTANCE), and possible error conditions (when loading tipsets fails). Keep each doc comment short, mention ResolveNullTipset::TakeOlder behavior where relevant, and ensure they follow rustdoc style and cover usage expectations and error semantics.src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1475-1483:⚠️ Potential issue | 🟡 MinorAdd
Safe/Finalizedblock tag tests forEthCallV1 to match V2 coverage.The
EthCallV1 handler uses the sameTipsetResolverthat resolvesSafeandFinalizedpredefined blocks, yet the test (lines 1467–1470) only exercises it withLatest. Expand the V1 test loop to cover[Latest, Safe, Finalized]like V2 (lines 1475–1483) to ensure parity in test coverage for all supported block tag parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1475 - 1483, Update the EthCall V1 test loop to include Predefined::Safe and Predefined::Finalized in addition to Predefined::Latest so it matches the V2 coverage; specifically, modify the loop that pushes RpcTest::identity entries created from EthCall::request_with_alias (the V1 counterpart to EthCallV2::request_with_alias) to iterate over [Predefined::Latest, Predefined::Safe, Predefined::Finalized] and push a test for each tag using the same (msg.clone(), BlockNumberOrHash::PredefinedBlock(tag)) and use_alias parameters.
♻️ Duplicate comments (1)
src/rpc/methods/eth/tipset_resolver.rs (1)
7-23:⚠️ Potential issue | 🟡 MinorAdd rustdoc for the public
TipsetResolverAPI.
TipsetResolverand its public methods are still undocumented. Please add rustdoc for purpose, parameters, return/error behavior, and V1/V2/F3 semantics.As per coding guidelines, "Document all public functions and structs with doc comments".
Also applies to: 23-55, 105-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/tipset_resolver.rs` around lines 7 - 23, Add rustdoc comments for the public TipsetResolver API: document the struct TipsetResolver (purpose and ownership of ctx and api_version), the constructor new(ctx: &Ctx<DB>, api_version: ApiPaths) (describe parameters and lifetime), and the public method tipset_by_block_number_or_hash (describe parameters, return type, error behavior, async semantics, and explicitly state how behavior differs for ApiPaths V1/V2/F3). Ensure the docs follow Rustdoc style, include examples or panics/errors if relevant, and apply the same documentation pattern to the other public methods referenced in the file.
🧹 Nitpick comments (4)
src/rpc/methods/eth.rs (1)
1505-1508: Consider extracting a small helper for repeated resolver calls.The same resolver-init + tipset-lookup sequence is repeated across many handlers; a helper would reduce boilerplate and drift risk.
♻️ Example extraction
+async fn resolve_tipset_for_request<DB: Blockstore + Send + Sync + 'static>( + ctx: &Ctx<DB>, + api_path: ApiPaths, + block_param: impl Into<BlockNumberOrHash>, + resolve: ResolveNullTipset, +) -> anyhow::Result<Tipset> { + TipsetResolver::new(ctx, api_path) + .tipset_by_block_number_or_hash(block_param, resolve) + .await +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 1505 - 1508, Extract a small helper (e.g., resolve_tipset) to consolidate the repeated resolver-init and lookup: have it accept the context (&ctx), extension (ext) or api path inputs (or directly call Self::api_path inside), the block_param, and a ResolveNullTipset policy, then inside call TipsetResolver::new(&ctx, Self::api_path(ext)?) and await tipset_by_block_number_or_hash(block_param, policy). Replace occurrences of TipsetResolver::new + tipset_by_block_number_or_hash with calls to this helper to remove boilerplate and centralize error handling.src/rpc/methods/eth/tipset_resolver.rs (1)
115-123: Avoid duplicating EC-finality computation.
get_ec_finalized_tipsetduplicates logic already present inChainGetTipSetV2::get_ec_finalized_tipset, which increases drift risk.♻️ Suggested deduplication
- pub fn get_ec_finalized_tipset(&self) -> anyhow::Result<Tipset> { - let head = self.ctx.chain_store().heaviest_tipset(); - let ec_finality_epoch = - (head.epoch() - self.ctx.chain_config().policy.chain_finality).max(0); - Ok(self.ctx.chain_index().tipset_by_height( - ec_finality_epoch, - head, - ResolveNullTipset::TakeOlder, - )?) - } + pub fn get_ec_finalized_tipset(&self) -> anyhow::Result<Tipset> { + ChainGetTipSetV2::get_ec_finalized_tipset(self.ctx) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/tipset_resolver.rs` around lines 115 - 123, The get_ec_finalized_tipset function duplicates EC-finality computation; instead delegate to the existing implementation or shared helper in ChainGetTipSetV2 to avoid drift. Replace the manual computation in get_ec_finalized_tipset (which uses self.ctx.chain_store(), self.ctx.chain_config().policy.chain_finality, and self.ctx.chain_index().tipset_by_height) with a call to ChainGetTipSetV2::get_ec_finalized_tipset (or extract the epoch calculation into a new private helper used by both) so both callers use the same logic and return path.src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1868-1873: InconsistentBlockNumberOrHashconstruction style inEthGetStorageAtV1 test.Line 1872 uses the raw variant
BlockNumberOrHash::BlockNumber(EthInt64(shared_tipset.epoch()))directly, while the analogous V2 test (Line 1898) and the majority of other tests use thefrom_block_numberhelper. This inconsistency is harmless but reduces readability.♻️ Proposed fix
- RpcTest::identity(EthGetStorageAt::request(( - // https://filfox.info/en/address/f410fpoidg73f7krlfohnla52dotowde5p2sejxnd4mq - EthAddress::from_str("0x7B90337f65fAA2B2B8ed583ba1Ba6EB0C9D7eA44")?, - EthBytes(vec![0xa]), - BlockNumberOrHash::BlockNumber(EthInt64(shared_tipset.epoch())), - ))?), + RpcTest::identity(EthGetStorageAt::request(( + // https://filfox.info/en/address/f410fpoidg73f7krlfohnla52dotowde5p2sejxnd4mq + EthAddress::from_str("0x7B90337f65fAA2B2B8ed583ba1Ba6EB0C9D7eA44")?, + EthBytes(vec![0xa]), + BlockNumberOrHash::from_block_number(shared_tipset.epoch()), + ))?),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1868 - 1873, Replace the direct construction BlockNumberOrHash::BlockNumber(EthInt64(shared_tipset.epoch())) used in the RpcTest::identity(EthGetStorageAt::request(...)) call with the helper constructor BlockNumberOrHash::from_block_number(EthInt64(shared_tipset.epoch())) so the V1 test matches the V2 style and other tests for consistency and readability.
2430-2438: Add assertion or saturating fallback for snapshot depth.
SAFE_EPOCH_DELAY_FOR_TESTING(20 epochs) equals only 10 minutes of chain history at 30-second block times. If a CI snapshot contains fewer than 20 finalized epochs,shared_tipsetwill end up shallower than intended, potentially undermining the finality guarantee the code attempts to ensure. Consider asserting minimum snapshot depth or using a saturating subtraction to handle undersized snapshots gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 2430 - 2438, The code currently assumes SAFE_EPOCH_DELAY_FOR_TESTING epochs exist when calling store.heaviest_tipset()?.chain(&store).take(SAFE_EPOCH_DELAY_FOR_TESTING as usize).last(); add a guard so the snapshot depth is checked and handled: compute the actual chain length from heaviest_tipset.chain(&store) (e.g., count or collect into an iterator), then use a saturating/minimum value for the take size (let take_depth = min(actual_depth, SAFE_EPOCH_DELAY_FOR_TESTING as usize)) before calling .take(take_depth).last(), or alternatively assert that actual_depth >= SAFE_EPOCH_DELAY_FOR_TESTING and emit a clear error; reference SAFE_EPOCH_DELAY_FOR_TESTING, shared_tipset, heaviest_tipset, and chain(&store) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 340-344: Add a Rust doc comment for the public constructor
BlockNumberOrHash::from_predefined explaining that it constructs a
BlockNumberOrHash from a Predefined tag (e.g., "latest", "earliest", "pending")
and returns the PredefinedBlock variant; include a short example of typical
usage and note the parameter type (Predefined) and the returned type
(BlockNumberOrHash) so consumers understand purpose and usage of the function.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 2142-2149: Add tests exercising the Predefined::Safe and
Predefined::Finalized variants for both EthTraceReplayBlockTransactions and
EthTraceReplayBlockTransactionsV2: for each of the two RPC handlers
(EthTraceReplayBlockTransactions and EthTraceReplayBlockTransactionsV2) add
RpcTest::identity requests that pass BlockNumberOrHash constructed with
Predefined::Safe and Predefined::Finalized (mirroring the existing epoch-based
calls), so the suite covers TipsetResolver-supported tags the same way
EthTraceBlock/EthTraceBlockV2 do.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1090-1142: Add rustdoc comments (///) above the three public
functions get_latest_safe_tipset, get_latest_finalized_tipset, and
get_ec_finalized_tipset that explain their purpose, inputs and return value, the
fallback behavior (get_latest_finalized_tipset falls back to
get_ec_finalized_tipset when F3 data is unavailable or too old;
get_latest_safe_tipset returns the F3-finalized tipset when newer than safe
height otherwise returns a tipset at safe_height), the guarantees (what
"finalized" or "safe" means relative to chain_finality and
SAFE_HEIGHT_DISTANCE), and possible error conditions (when loading tipsets
fails). Keep each doc comment short, mention ResolveNullTipset::TakeOlder
behavior where relevant, and ensure they follow rustdoc style and cover usage
expectations and error semantics.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1475-1483: Update the EthCall V1 test loop to include
Predefined::Safe and Predefined::Finalized in addition to Predefined::Latest so
it matches the V2 coverage; specifically, modify the loop that pushes
RpcTest::identity entries created from EthCall::request_with_alias (the V1
counterpart to EthCallV2::request_with_alias) to iterate over
[Predefined::Latest, Predefined::Safe, Predefined::Finalized] and push a test
for each tag using the same (msg.clone(),
BlockNumberOrHash::PredefinedBlock(tag)) and use_alias parameters.
---
Duplicate comments:
In `@src/rpc/methods/eth/tipset_resolver.rs`:
- Around line 7-23: Add rustdoc comments for the public TipsetResolver API:
document the struct TipsetResolver (purpose and ownership of ctx and
api_version), the constructor new(ctx: &Ctx<DB>, api_version: ApiPaths)
(describe parameters and lifetime), and the public method
tipset_by_block_number_or_hash (describe parameters, return type, error
behavior, async semantics, and explicitly state how behavior differs for
ApiPaths V1/V2/F3). Ensure the docs follow Rustdoc style, include examples or
panics/errors if relevant, and apply the same documentation pattern to the other
public methods referenced in the file.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1505-1508: Extract a small helper (e.g., resolve_tipset) to
consolidate the repeated resolver-init and lookup: have it accept the context
(&ctx), extension (ext) or api path inputs (or directly call Self::api_path
inside), the block_param, and a ResolveNullTipset policy, then inside call
TipsetResolver::new(&ctx, Self::api_path(ext)?) and await
tipset_by_block_number_or_hash(block_param, policy). Replace occurrences of
TipsetResolver::new + tipset_by_block_number_or_hash with calls to this helper
to remove boilerplate and centralize error handling.
In `@src/rpc/methods/eth/tipset_resolver.rs`:
- Around line 115-123: The get_ec_finalized_tipset function duplicates
EC-finality computation; instead delegate to the existing implementation or
shared helper in ChainGetTipSetV2 to avoid drift. Replace the manual computation
in get_ec_finalized_tipset (which uses self.ctx.chain_store(),
self.ctx.chain_config().policy.chain_finality, and
self.ctx.chain_index().tipset_by_height) with a call to
ChainGetTipSetV2::get_ec_finalized_tipset (or extract the epoch calculation into
a new private helper used by both) so both callers use the same logic and return
path.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1868-1873: Replace the direct construction
BlockNumberOrHash::BlockNumber(EthInt64(shared_tipset.epoch())) used in the
RpcTest::identity(EthGetStorageAt::request(...)) call with the helper
constructor
BlockNumberOrHash::from_block_number(EthInt64(shared_tipset.epoch())) so the V1
test matches the V2 style and other tests for consistency and readability.
- Around line 2430-2438: The code currently assumes SAFE_EPOCH_DELAY_FOR_TESTING
epochs exist when calling
store.heaviest_tipset()?.chain(&store).take(SAFE_EPOCH_DELAY_FOR_TESTING as
usize).last(); add a guard so the snapshot depth is checked and handled: compute
the actual chain length from heaviest_tipset.chain(&store) (e.g., count or
collect into an iterator), then use a saturating/minimum value for the take size
(let take_depth = min(actual_depth, SAFE_EPOCH_DELAY_FOR_TESTING as usize))
before calling .take(take_depth).last(), or alternatively assert that
actual_depth >= SAFE_EPOCH_DELAY_FOR_TESTING and emit a clear error; reference
SAFE_EPOCH_DELAY_FOR_TESTING, shared_tipset, heaviest_tipset, and chain(&store)
when making the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/tipset_resolver.rssrc/rpc/mod.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
💤 Files with no reviewable changes (1)
- src/rpc/mod.rs
Contributor
Author
Done.
The current logic under
Should be easy with the new
V2 enums are now merged with V1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6631
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Documentation
Tests