refactor(katana-node): flatten rpc server building logic #2792
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/katana/node/src/lib.rs (2)
86-86: Consider makingrpc_servera private fieldOhayo, sensei! The
rpc_serverfield in theNodestruct is currently public. If it's not intended to be accessed outside of theNodeimplementation, making it private can enhance encapsulation and prevent unintended usage.
253-253: Avoid using.unwrap()with header parsingOhayo, sensei! Using
.unwrap()on header parsing can cause a panic if the header name is invalid. Consider using.expect("Valid header name")for better error messages or handling the potential error gracefully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/katana/node/src/exit.rs(1 hunks)crates/katana/node/src/lib.rs(6 hunks)
🔇 Additional comments (2)
crates/katana/node/src/lib.rs (1)
97-97: Verify ownership change in launch method
Ohayo, sensei! The launch method now consumes self instead of borrowing it mutably. Please ensure that this change doesn't affect other parts of the code where Node might be used after launching.
To check for any usages of Node after launch, you can run the following script:
✅ Verification successful
The previous search wasn't effective because the pattern was too specific. Let's broaden our search to find all usages of launch() and understand the context better, along with the Node struct definition and test cases.
The previous results show that launch() is used in several places, but we need to verify the ownership patterns and see if there are any post-launch usages of the Node instance.
Ownership change in launch method is safe
Ohayo, sensei! After analyzing the codebase, the ownership change is correct and safe. The Node instance is properly consumed by launch() and transformed into a LaunchedNode struct, which contains all the necessary components. Looking at the implementation:
- The
launch()method takes ownership ofselfand returns aLaunchedNode - All usages of
Node::launch()show that the node is only used through the returnedLaunchedNodeafterward - The
LaunchedNodestruct receives ownership of all important components from the originalNode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of `Node` after the `launch` method is called.
# Test: Find instances where `Node` is used after `launch` is invoked.
# Expect: No usages of `Node` after `launch` method is called.
rg -A5 'let\s+.*=\s+Node::.*;\s+.*\.launch\(\)' --threads 1 | rg -A5 'Node::' --threads 1
Length of output: 89
Script:
#!/bin/bash
# Let's search for all usages of the launch method
rg "\.launch\(\)" -B 5 -A 2
# Also search for Node struct definition and its usages
ast-grep --pattern 'struct Node {
$$$
}'
# Find all test cases involving Node and launch
rg "test.*Node.*launch" -A 5
Length of output: 1730
Script:
#!/bin/bash
# Let's find the Node struct and its methods to understand ownership patterns
ast-grep --pattern 'impl Node {
$$$
}'
# Search for any references to Node after launch
rg "node\." -A 3 crates/katana/cli/src/args.rs
# Look for any potential clones or references of Node
rg "Clone|&Node|&mut Node" crates/katana/node/src/
Length of output: 11469
crates/katana/node/src/exit.rs (1)
21-21: Ensuring proper shutdown with handle.rpc.handle.clone().stopped().await
Ohayo, sensei! The change correctly waits for the RPC server to stop, ensuring that all asynchronous tasks are fully terminated before the node shuts down.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2792 +/- ##
=======================================
Coverage 55.60% 55.61%
=======================================
Files 439 439
Lines 55678 55672 -6
=======================================
+ Hits 30961 30962 +1
+ Misses 24717 24710 -7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
crates/katana/rpc/rpc/src/lib.rs (1)
Line range hint
46-50: Consider adding error documentationThe
stop()method's error conditions should be documented to help users handle theAlreadyStoppedcase appropriately./// Tell the server to stop without waiting for the server to stop. + /// + /// # Errors + /// Returns `Error::AlreadyStopped` if the server has already been stopped. pub fn stop(&self) -> Result<(), Error> {crates/dojo/test-utils/src/sequencer.rs (1)
45-46: Consider error handling improvementWhile the URL parsing looks good, we could make the error handling more specific.
- Url::parse(&format!("http://{}", handle.rpc.addr())).expect("Failed to parse URL"); + Url::parse(&format!("http://{}", handle.rpc.addr())) + .expect("Failed to create URL from valid socket address");crates/katana/node/src/lib.rs (2)
247-253: Consider documenting the Argent-specific headers, senseiThe CORS configuration includes Argent-specific headers. It would be helpful to document why these headers are needed and if they're required for specific client compatibility.
Line range hint
247-287: Consider extracting RPC module building logicThe RPC module building logic could be extracted into a separate function for better maintainability and testability.
Example refactor:
+ fn build_rpc_modules( + config: &Config, + backend: Arc<Backend<BlockifierFactory>>, + pool: TxPool, + block_producer: BlockProducer<BlockifierFactory>, + forked_client: Option<ForkedClient>, + ) -> Result<RpcModule<()>> { + let mut rpc_modules = RpcModule::new(()); + // ... existing module building logic ... + Ok(rpc_modules) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/dojo/test-utils/Cargo.toml(1 hunks)crates/dojo/test-utils/src/sequencer.rs(3 hunks)crates/katana/node/src/exit.rs(1 hunks)crates/katana/node/src/lib.rs(7 hunks)crates/katana/rpc/rpc/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/node/src/exit.rs
🔇 Additional comments (9)
crates/dojo/test-utils/Cargo.toml (1)
17-17: Ohayo! LGTM - Workspace dependency properly added
The addition of katana-rpc as a workspace dependency aligns well with the RPC server restructuring objective.
crates/katana/rpc/rpc/src/lib.rs (2)
36-42: Ohayo sensei! Documentation improvements enhance clarity
The added documentation clearly explains the RPC server handle's components and their purposes.
56-59: LGTM - Getter method follows Rust conventions
The addr() method properly implements the getter pattern, returning a reference to avoid unnecessary cloning.
crates/dojo/test-utils/src/sequencer.rs (2)
13-13: LGTM - Error type import aligned with new architecture
The switch to katana_rpc::Error provides better error handling integration with the RPC subsystem.
108-108: LGTM - Simplified RPC server shutdown
The simplified stop call aligns with the flattened RPC server structure, making the code more maintainable.
crates/katana/node/src/lib.rs (4)
86-86: Ohayo! Nice architectural improvement, sensei!
Moving the RPC server into the Node struct improves encapsulation and makes the ownership model clearer.
97-97: Taking ownership in launch method prevents misuse, excellent work sensei!
The ownership model change ensures that the Node instance cannot be used after launching, preventing potential concurrent access issues.
Also applies to: 138-140
292-300: Clean and well-organized Node construction, sensei!
The Node construction follows a logical order and includes all necessary components.
67-67: Ohayo sensei! Consider implementing graceful shutdown
The TODO comment suggests waiting for the RPC server to stop. Currently, there might be a race condition if new requests arrive during shutdown.
Let's check if there are any existing graceful shutdown implementations we can reference:
move all the code that is in
spawntokatana_node::buildfunction, and include theRpcServerstruct in theNodestructSummary by CodeRabbit
New Features
Bug Fixes
Documentation
RpcServerHandlestruct and its methods for better clarity.Chores
katana-rpcas a workspace dependency in the project configuration.