fix(rpc): pass http::Extensions to RpcMethod::handle#6646
fix(rpc): pass http::Extensions to RpcMethod::handle#6646hanabi1224 wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds a third parameter Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Reflect as "Rpc Reflect/Registry"
participant Handler as "RpcMethod::handle"
participant Downstream as "Downstream RPC / Helpers"
participant Store as "Blockstore/DB"
Client->>Reflect: send RPC request (method, params, http extensions)
Reflect->>Handler: invoke handle(ctx, params, &extensions)
Handler->>Downstream: call nested RPCs / helpers with &extensions
Downstream->>Store: read/write using ctx
Downstream-->>Handler: return results
Handler-->>Reflect: return response
Reflect-->>Client: send RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai create a PR against this branch to fill |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1604 🏁 Script executed: Length of output: 138201 🏁 Script executed: Length of output: 437
🏁 Script executed: Length of output: 2456 🏁 Script executed: Length of output: 5364 🏁 Script executed: Length of output: 486 🏁 Script executed: Length of output: 4 🏁 Script executed: Length of output: 51
Task: Add
|
|
@coderabbitai generate a patch to fill |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 22 Let me examine the actual format of the handle methods first: 🏁 Script executed: Length of output: 2241
🏁 Script executed: Length of output: 56 Let me fix the script to avoid the process substitution issue: 🏁 Script executed: Length of output: 43283
DetailsThe patch updates **132 handle methods** across **15 files**:Files modified:
Note: In To apply the patch: Save the patch content to a file (e.g., patch -p0 < rpc-extensions.patchOr apply it with git: git apply rpc-extensions.patchThe complete patch is shown in the script output above. Would you like me to generate a cleaned-up version that excludes the duplicate in common.rs? 🧠 Learnings used |
|
@coderabbitai go ahead with the plan and create a PR |
|
Tip For best results, initiate chat on the files or code changes.
Task: Add
|
0edec18 to
8bd5d45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/state.rs (1)
108-115:⚠️ Potential issue | 🔴 CriticalAdd missing
httpimport to resolve compilation error.The code uses
http::Extensionsthroughout the method signatures without importinghttp. Adduse http::Extensions;to the imports at the top of the file, or useuse http;if the full path is preferred. Without this import, the code will not compile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/state.rs` around lines 108 - 115, The compiler error is caused by missing import for the http::Extensions type used in the handle signature; add an import for http::Extensions at the top of the file (e.g., `use http::Extensions;`) or `use http;` so the handle function declaration (async fn handle(ctx: Ctx<impl Blockstore + Send + Sync + 'static>, (message, ApiTipsetKey(tsk)): Self::Params, _: &http::Extensions) -> Result<Self::Ok, ServerError>)` can resolve the http symbol.
🧹 Nitpick comments (1)
src/rpc/methods/chain.rs (1)
205-208: Consider threading the caller’s Extensions instead of Default::default().
If extensions start carrying request context, these internal F3 calls will drop it. Passing through the same Extensions would preserve consistency.Also applies to: 1112-1114
🤖 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 205 - 208, The call to crate::rpc::f3::F3GetLatestCertificate::handle uses Default::default() for Extensions which drops caller request context; replace Default::default() with the caller's Extensions taken from ctx (e.g., use ctx.extensions()/ctx.extensions.clone() or the appropriate accessor) so the same Extensions are threaded through, and apply the same change for the other internal F3 handle call sites (the other F3GetLatestCertificate::handle usages around the later block).
🤖 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 4234-4238: Eth_trace replay handlers are dropping per-request
extensions by using `_` and passing &Default::default() into
EthGetTransactionHashByCid::handle; change the parameter name `_` to `ext` in
EthTraceReplayBlockTransactions::handle and
EthTraceReplayBlockTransactionsV2::handle, update the trace replay helper
signature to accept ext: &http::Extensions, and forward that ext through to the
inner call to EthGetTransactionHashByCid::handle so the real request extensions
are preserved.
---
Outside diff comments:
In `@src/rpc/methods/state.rs`:
- Around line 108-115: The compiler error is caused by missing import for the
http::Extensions type used in the handle signature; add an import for
http::Extensions at the top of the file (e.g., `use http::Extensions;`) or `use
http;` so the handle function declaration (async fn handle(ctx: Ctx<impl
Blockstore + Send + Sync + 'static>, (message, ApiTipsetKey(tsk)): Self::Params,
_: &http::Extensions) -> Result<Self::Ok, ServerError>)` can resolve the http
symbol.
---
Nitpick comments:
In `@src/rpc/methods/chain.rs`:
- Around line 205-208: The call to
crate::rpc::f3::F3GetLatestCertificate::handle uses Default::default() for
Extensions which drops caller request context; replace Default::default() with
the caller's Extensions taken from ctx (e.g., use
ctx.extensions()/ctx.extensions.clone() or the appropriate accessor) so the same
Extensions are threaded through, and apply the same change for the other
internal F3 handle call sites (the other F3GetLatestCertificate::handle usages
around the later block).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/rpc/methods/auth.rssrc/rpc/methods/beacon.rssrc/rpc/methods/chain.rssrc/rpc/methods/common.rssrc/rpc/methods/eth.rssrc/rpc/methods/f3.rssrc/rpc/methods/gas.rssrc/rpc/methods/market.rssrc/rpc/methods/miner.rssrc/rpc/methods/misc.rssrc/rpc/methods/mpool.rssrc/rpc/methods/msig.rssrc/rpc/methods/net.rssrc/rpc/methods/node.rssrc/rpc/methods/state.rssrc/rpc/methods/sync.rssrc/rpc/methods/wallet.rssrc/rpc/reflect/mod.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
8bd5d45 to
7a5f196
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rpc/methods/eth.rs (1)
4234-4334: Covered in prior review.🤖 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 4234 - 4334, The two RpcMethod::handle implementations for EthTraceReplayBlockTransactions and EthTraceReplayBlockTransactionsV2 duplicate the same validation and replay logic; consolidate by extracting the common logic (trace_types check, tipset resolution, and call to eth_trace_replay_block_transactions) into a single helper or have V2 call the same shared helper used by V1. Specifically, keep eth_trace_replay_block_transactions as the replay worker, and in EthTraceReplayBlockTransactions::handle and EthTraceReplayBlockTransactionsV2::handle only perform the trace_types validation then call a shared function that resolves the tipset (use tipset_by_ext_block_number_or_hash or tipset_by_block_number_or_hash_v2 as appropriate) and then invokes eth_trace_replay_block_transactions(ctx, &ts, ext). Ensure you remove the duplicated loop/logic from the handles so both impls reuse the same helper and fix any incorrect arguments passed to the tipset resolution functions (match the expected signatures for tipset_by_ext_block_number_or_hash and tipset_by_block_number_or_hash_v2).
🤖 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/f3.rs`:
- Around line 665-673: Add a doc comment to the public async helper
F3GetLatestCertificate::get describing its purpose (fetches the latest
FinalityCertificate via the RPC client), the return value
(anyhow::Result<FinalityCertificate>), and failure modes (propagates RPC or
client creation errors); place the doc comment immediately above the impl or the
get method and reference that it uses get_rpc_http_client() and the RPC method
name via RpcMethod::NAME so readers understand it performs an RPC request and
can fail with an anyhow error.
---
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 4234-4334: The two RpcMethod::handle implementations for
EthTraceReplayBlockTransactions and EthTraceReplayBlockTransactionsV2 duplicate
the same validation and replay logic; consolidate by extracting the common logic
(trace_types check, tipset resolution, and call to
eth_trace_replay_block_transactions) into a single helper or have V2 call the
same shared helper used by V1. Specifically, keep
eth_trace_replay_block_transactions as the replay worker, and in
EthTraceReplayBlockTransactions::handle and
EthTraceReplayBlockTransactionsV2::handle only perform the trace_types
validation then call a shared function that resolves the tipset (use
tipset_by_ext_block_number_or_hash or tipset_by_block_number_or_hash_v2 as
appropriate) and then invokes eth_trace_replay_block_transactions(ctx, &ts,
ext). Ensure you remove the duplicated loop/logic from the handles so both impls
reuse the same helper and fix any incorrect arguments passed to the tipset
resolution functions (match the expected signatures for
tipset_by_ext_block_number_or_hash and tipset_by_block_number_or_hash_v2).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/rpc/methods/auth.rssrc/rpc/methods/beacon.rssrc/rpc/methods/chain.rssrc/rpc/methods/common.rssrc/rpc/methods/eth.rssrc/rpc/methods/f3.rssrc/rpc/methods/gas.rssrc/rpc/methods/market.rssrc/rpc/methods/miner.rssrc/rpc/methods/misc.rssrc/rpc/methods/mpool.rssrc/rpc/methods/msig.rssrc/rpc/methods/net.rssrc/rpc/methods/node.rssrc/rpc/methods/state.rssrc/rpc/methods/sync.rssrc/rpc/methods/wallet.rssrc/rpc/reflect/mod.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tool/subcommands/api_cmd/test_snapshot.rs
- src/rpc/methods/miner.rs
- src/rpc/methods/auth.rs
- src/rpc/methods/sync.rs
- src/rpc/methods/mpool.rs
- src/rpc/methods/node.rs
- src/rpc/methods/beacon.rs
Summary of changes
With this change, we could resolve
ApiPathsin RPC methods and merge a few Eth v2 RPC methods with v1Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit