fix(RPC): backport F3 finality resolution to eth V1 methods#6644
Merged
hanabi1224 merged 24 commits intomainfrom Mar 5, 2026
Merged
fix(RPC): backport F3 finality resolution to eth V1 methods#6644hanabi1224 merged 24 commits intomainfrom
hanabi1224 merged 24 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 (
|
hanabi1224
force-pushed
the
hm/f3-await-v1-rpc
branch
from
df5caaf to
550c71d
Compare
LesnyRumcajs
reviewed
Mar 4, 2026
LesnyRumcajs
reviewed
Mar 4, 2026
hanabi1224
commented
Mar 5, 2026
hanabi1224
commented
Mar 5, 2026
akaladarshi
reviewed
Mar 5, 2026
Collaborator
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
Some small nits, Otherwise LGTM
hanabi1224
force-pushed
the
hm/f3-await-v1-rpc
branch
from
06d13db to
a40aba7
Compare
akaladarshi
approved these changes
Mar 5, 2026
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
3 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