Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(katana-node): distinguish between launched node handle #2504

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

kariy
Copy link
Member

@kariy kariy commented Oct 8, 2024

abstraction to distinguish between a launched and not yet launch node handle. also appropriately place the stop methods under the launched handle. this PR also includes some ergonomic changes to some of the futures used to stop the node.

having a different struct for this would allow conveying to the readers that the node can either be in static (not yet launched) and already launched node. and thus only expose certain methods based on the appropriate handle.

eg, the LaunchedNode::stop() method is placed under LaunchedNode because you can only stop node that has been started. it doesn't make sense to stop a node that hasn't even been ran.

Summary by CodeRabbit

  • New Features

    • Introduced a new node lifecycle management structure to enhance node initialization and shutdown processes.
    • Added a new NodeStoppedFuture for better handling of node shutdown in an asynchronous context.
    • Implemented a launch method for initializing node metrics and components.
    • Enhanced visibility of the PrometheusHandle type for external access.
  • Bug Fixes

    • Improved handling of OS signals for graceful node shutdown.
  • Documentation

    • Updated comments and documentation to reflect changes in node startup and lifecycle management.

Copy link

coderabbitai bot commented Oct 8, 2024

Walkthrough

Ohayo, sensei! This pull request introduces significant changes across multiple files, primarily focusing on the restructuring of node initialization and shutdown processes. Key modifications include renaming methods for clarity, updating struct definitions, and altering the control flow for handling node lifecycle events. The introduction of new types and futures enhances asynchronous operations, particularly in managing shutdown sequences and backend interactions. Overall, these changes aim to streamline the node's functionality and improve code clarity.

Changes

File Path Change Summary
bin/katana/src/cli/node.rs - Renamed method start to build.
- Updated start_node to call katana_node::build.
- Modified print_intro parameter type from SocketAddr to &str.
- Enhanced OS signal handling for graceful shutdown with handle.stop().await?.
crates/katana/node/src/lib.rs - Added structs: LaunchedNode and Node.
- Added methods: launch, stop, and stopped for Node and LaunchedNode.
- Renamed start method to build.
crates/metrics/src/prometheus_exporter.rs - Updated visibility of PrometheusHandle to public.

Possibly related PRs

Suggested reviewers

  • glihm

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
crates/katana/tasks/src/manager.rs (1)

129-129: Typo in documentation comment on ShutdownFuture

Ohayo, sensei! There's a minor typo in the documentation comment on line 129. It should read "A future" instead of "A futures."

Apply this diff to fix the typo:

-/// A futures that resolves when the [TaskManager] is shutdown.
+/// A future that resolves when the [TaskManager] is shutdown.
crates/katana/node/src/lib.rs (2)

68-68: Fix grammatical error in documentation comment

Ohayo, sensei! The documentation on line 68 should read "wait until it has actually stopped" instead of "wait until it has actually stop".


101-101: Correct grammatical error in launch method documentation

Ohayo, sensei! In the documentation on line 101, it should be "start all the node processes" instead of "start all the node process".

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9c790a9 and 6220494.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • bin/katana/src/cli/node.rs (2 hunks)
  • crates/dojo-test-utils/src/sequencer.rs (4 hunks)
  • crates/katana/node/Cargo.toml (1 hunks)
  • crates/katana/node/src/exit.rs (1 hunks)
  • crates/katana/node/src/lib.rs (5 hunks)
  • crates/katana/tasks/src/manager.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (23)
crates/katana/node/Cargo.toml (1)

22-22: Ohayo, sensei! The addition of the futures crate looks sharp!

The inclusion of the futures crate aligns perfectly with the PR's objective of implementing abstractions for different node states. This dependency will likely prove useful in handling asynchronous operations, particularly when dealing with launched and non-launched node states.

The use of .workspace = true is a wise choice, ensuring version consistency across the project. Keep up the excellent work, sensei!

crates/katana/node/src/exit.rs (4)

13-15: Well-defined NodeStoppedFuture struct

Ohayo, sensei! The NodeStoppedFuture struct is properly defined with the necessary lifetime parameter and internal future.


17-26: Correct implementation of new method

Ohayo, sensei! The new method effectively constructs the NodeStoppedFuture by creating a pinned asynchronous block. The sequence of operations looks appropriate.


19-23: Ensure error handling aligns with overall strategy

Ohayo, sensei! In the asynchronous block within new, the use of handle.stop().await? correctly propagates errors. Please verify that this aligns with the intended error handling approach in the broader context where NodeStoppedFuture is utilized.


28-35: Proper Future trait implementation

Ohayo, sensei! The Future trait is correctly implemented for NodeStoppedFuture, and poll_unpin is appropriately used to poll the inner future.

crates/dojo-test-utils/src/sequencer.rs (5)

10-10: Ohayo, sensei! Importing LaunchedNode

The LaunchedNode is correctly imported from katana_node.


32-32: Updating handle to use LaunchedNode

The handle field in TestSequencer is appropriately updated to LaunchedNode, reflecting the new node state distinction.


48-50: Building the node with the new initialization method

The node is now built using katana_node::build, which aligns with the updated initialization process.


51-51: Launching the node and obtaining the handle

The node is successfully launched with node.launch(), and the handle is obtained as a LaunchedNode.


53-53: Updating URL to use handle's RPC address

The URL is correctly constructed using handle.rpc.addr, ensuring it references the launched node's RPC address.

crates/katana/tasks/src/manager.rs (4)

50-52: Enhanced flexibility by returning a future in wait_for_shutdown

Ohayo, sensei! Changing wait_for_shutdown from an async fn to a function that returns WaitForShutdownFuture (lines 51-52) provides greater flexibility in how the shutdown signal is awaited. This allows for more composability in async code. Well done!


59-72: Refactoring shutdown method improves control over shutdown process

Ohayo, sensei! Modifying shutdown to return a ShutdownFuture instead of being an async fn (lines 59-72) is a smart move. This encapsulates the shutdown logic within a custom future, giving users explicit control over the shutdown sequence. Excellent work!


129-148: Introduction of ShutdownFuture enhances asynchronous shutdown handling

Ohayo, sensei! Adding the ShutdownFuture struct (lines 129-148) and implementing the Future trait for it is a great enhancement. This provides a clear and type-safe way to handle shutdown operations asynchronously. Nice addition!


1-3: Clarify the use of core over std for futures and pinning

Ohayo, sensei! I see that you've switched imports from std to core for Future, Pin, and task modules (lines 1-3), and added BoxFuture and FutureExt from futures (lines 5-6). If the intention is to reduce dependencies or prepare for a no_std environment, this makes sense. Please ensure this aligns with the project's goals.

Also applies to: 5-6

crates/katana/node/src/lib.rs (4)

59-63: Great implementation of the LaunchedNode abstraction

Ohayo, sensei! The introduction of the LaunchedNode struct effectively distinguishes between launched and unlaunched node states, enhancing code clarity and maintainability.


3-4: Addition of the exit module looks good

Ohayo, sensei! The inclusion of the exit module enhances the node's shutdown management. Good job!


132-133: 🛠️ Refactor suggestion

Update usage of deprecated stage::Sequencing

Ohayo, sensei! On lines 132-133, stage::Sequencing is marked as deprecated. Consider updating to the latest implementation to ensure compatibility and leverage improvements.

To check for available alternatives, you can run:

#!/bin/bash
# Description: Find non-deprecated sequencing stages in the `stage` module.

# Test: Search for structs in the `stage` module that are not deprecated.
rg 'pub struct' --type rust -g '*/stage/*.rs' -A 2 | grep -v '#[deprecated]'

# Expected Result: List of available structs in the `stage` module that can be used as alternatives.

94-95: 🛠️ Refactor suggestion

Update usage of deprecated SequencerConfig

Ohayo, sensei! The code on lines 94-95 uses a deprecated SequencerConfig. Updating to the recommended configuration will help maintain future compatibility and reduce potential issues.

To verify if there's a non-deprecated alternative, you can run the following script:

bin/katana/src/cli/node.rs (5)

228-229: Ohayo, sensei! The refactored node building process looks good.


238-240: Launching the node after building: Good change

Ohayo, sensei! The separation of building and launching the node improves clarity and aligns with the new node lifecycle.


241-248: Graceful shutdown implementation looks good

Ohayo, sensei! The implementation of graceful shutdown on receiving OS signals is well-handled. This ensures that the node stops properly when needed.


349-349: Updated print_intro function signature

Ohayo, sensei! Changing the print_intro function to accept address: &str enhances flexibility. Ensure that all usages of address within print_intro are updated accordingly.


234-235: ⚠️ Potential issue

Potential type mismatch when passing server_address to print_intro.

Ohayo, sensei! Since print_intro now expects an &str for the address parameter, ensure that server_address is of type &str. If node.server_config.addr() returns a SocketAddr, you may need to convert it to a string using to_string().

Run the following script to verify the return type of ServerConfig::addr():

✅ Verification successful

No Type Mismatch Found

Ohayo, sensei! Verified that server_address is a String, and passing a reference to it (&server_address) correctly matches the expected &str type for print_intro.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the return type of `ServerConfig::addr()`.

# Test: Search for the definition of `fn addr()` and check its return type.
rg 'fn addr\(&self\) ->' --type rust -A 1

# Expected output: The function returns a `String` or `&str`.

Length of output: 197

crates/katana/node/src/exit.rs Show resolved Hide resolved
crates/dojo-test-utils/src/sequencer.rs Show resolved Hide resolved
crates/dojo-test-utils/src/sequencer.rs Show resolved Hide resolved
crates/dojo-test-utils/src/sequencer.rs Show resolved Hide resolved
crates/katana/node/src/lib.rs Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
crates/katana/node/src/lib.rs (3)

82-96: Ohayo, sensei! Excellent work on the Node struct!

The Node struct nicely encapsulates all the necessary components of the node, which aligns perfectly with the PR objectives. The use of the #[must_use] attribute is a great touch to ensure that the node is actually launched.

One small suggestion to enhance the documentation:

Consider adding a brief comment for each field in the Node struct to explain its purpose. This will make it easier for other developers to understand the role of each component.

pub struct Node {
    /// Pool for managing transactions
    pub pool: TxPool,
    /// Database environment (optional)
    pub db: Option<DbEnv>,
    // ... (add comments for other fields)
}

Line range hint 155-285: Ohayo, sensei! Let's refactor this build function for better maintainability!

The build function is quite complex and handles various initializations. As the TODO comment suggests, this is a placeholder. Here are some suggestions to improve it:

  1. Break down the build function into smaller, more focused functions. For example:

    • init_executor_factory
    • init_backend
    • init_block_producer
    • init_transaction_pool
  2. Consider creating a NodeBuilder struct that encapsulates the building process. This would allow for a more flexible and testable construction process.

  3. Use the builder pattern to allow for optional configurations. This would make the initialization process more clear and less prone to errors.

Here's a sketch of how this could look:

struct NodeBuilder {
    server_config: ServerConfig,
    sequencer_config: SequencerConfig,
    starknet_config: StarknetConfig,
}

impl NodeBuilder {
    fn new(
        server_config: ServerConfig,
        sequencer_config: SequencerConfig,
        starknet_config: StarknetConfig,
    ) -> Self {
        Self { server_config, sequencer_config, starknet_config }
    }

    fn init_executor_factory(&self) -> Result<Arc<dyn ExecutorFactory>> {
        // Initialization logic for executor factory
    }

    fn init_backend(&self) -> Result<(Arc<Backend<BlockifierFactory>>, Option<DbEnv>)> {
        // Initialization logic for backend
    }

    // ... other init methods ...

    async fn build(self) -> Result<Node> {
        let executor_factory = self.init_executor_factory()?;
        let (backend, db) = self.init_backend()?;
        let block_producer = self.init_block_producer(&backend)?;
        let pool = self.init_transaction_pool(&block_producer)?;

        Ok(Node {
            db,
            pool,
            backend,
            server_config: self.server_config,
            block_producer,
            sequencer_config: self.sequencer_config,
            task_manager: TaskManager::current(),
        })
    }
}

pub async fn build(
    server_config: ServerConfig,
    sequencer_config: SequencerConfig,
    starknet_config: StarknetConfig,
) -> Result<Node> {
    NodeBuilder::new(server_config, sequencer_config, starknet_config).build().await
}

This refactoring will make the code more modular, easier to test, and more maintainable. It also addresses the TODO by providing a more structured approach to building the node. What do you think, sensei?


Line range hint 288-371: Ohayo, sensei! Let's enhance the spawn function for better modularity!

The spawn function is doing a great job setting up the RPC server, but it's handling quite a few responsibilities. Here are some suggestions to improve its structure and maintainability:

  1. Break down the function into smaller, more focused functions:

    • register_api_methods: Handle the registration of API methods based on the configuration.
    • setup_cors: Configure CORS settings.
    • build_middleware: Set up the middleware stack.
    • create_server: Create and configure the server instance.
  2. Consider creating a RpcServerBuilder struct to encapsulate the server building process. This would allow for a more flexible and testable construction process.

Here's a sketch of how this could look:

struct RpcServerBuilder<EF: ExecutorFactory> {
    node_components: (TxPool, Arc<Backend<EF>>, BlockProducer<EF>, TxValidator),
    config: ServerConfig,
}

impl<EF: ExecutorFactory> RpcServerBuilder<EF> {
    fn new(node_components: (TxPool, Arc<Backend<EF>>, BlockProducer<EF>, TxValidator), config: ServerConfig) -> Self {
        Self { node_components, config }
    }

    fn register_api_methods(&self, methods: &mut RpcModule<()>) -> Result<()> {
        // API method registration logic
    }

    fn setup_cors(&self) -> CorsLayer {
        // CORS setup logic
    }

    fn build_middleware(&self, cors: CorsLayer) -> tower::ServiceBuilder {
        // Middleware building logic
    }

    async fn create_server(&self, middleware: tower::ServiceBuilder, methods: RpcModule<()>) -> Result<ServerHandle> {
        // Server creation logic
    }

    pub async fn build(self) -> Result<RpcServer> {
        let mut methods = RpcModule::new(());
        self.register_api_methods(&mut methods)?;

        let cors = self.setup_cors();
        let middleware = self.build_middleware(cors);
        let handle = self.create_server(middleware, methods).await?;

        Ok(RpcServer { handle, addr: handle.local_addr()? })
    }
}

pub async fn spawn<EF: ExecutorFactory>(
    node_components: (TxPool, Arc<Backend<EF>>, BlockProducer<EF>, TxValidator),
    config: ServerConfig,
) -> Result<RpcServer> {
    RpcServerBuilder::new(node_components, config).build().await
}

This refactoring will make the code more modular, easier to test, and more maintainable. It separates concerns and makes the server spawning process more clear and flexible. What do you think about this approach, sensei?

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6220494 and 9f48bbf.

📒 Files selected for processing (2)
  • bin/katana/src/cli/node.rs (2 hunks)
  • crates/katana/node/src/lib.rs (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
crates/katana/node/src/lib.rs (3)

3-3: Ohayo, sensei! Nice work on the new LaunchedNode struct!

The introduction of the exit module and the LaunchedNode struct aligns perfectly with the PR objectives. This clear distinction between launched and unlaunched nodes will indeed enhance the clarity of the node's lifecycle management. Great job on improving the code structure!

Also applies to: 57-63


Line range hint 373-377: Ohayo, sensei! The RpcServer struct looks great!

The RpcServer struct is simple, clean, and does exactly what it needs to do. It encapsulates the necessary information about the RPC server (its address and handle) without any unnecessary complexity. The Debug derive is a nice touch for easier debugging and logging.

Great job on keeping this part of the code concise and focused!


65-80: ⚠️ Potential issue

Ohayo, sensei! Let's enhance the graceful shutdown process.

The LaunchedNode implementation looks solid, but there's an opportunity for improvement in the stop method. The TODO comment suggests waiting for the RPC server to stop instead of just stopping it.

To ensure a more graceful shutdown, consider implementing a mechanism to wait for the RPC server to fully stop. This could involve using a future or a channel to signal when the server has completely shut down.

bin/katana/src/cli/node.rs (3)

228-231: Ohayo, sensei! Node building process correctly updated

The change to use katana_node::build effectively separates the build and launch phases, aligning with the new abstraction. This enhances code clarity and maintainability.


240-250: Ohayo, sensei! Node launch and graceful shutdown implemented correctly

The updates correctly launch the node using node.launch() and handle OS signals for graceful shutdown. This improves node lifecycle management and aligns with asynchronous best practices.


351-351: Ohayo, sensei! Function print_intro signature updated appropriately

Updating the print_intro function to accept address: &str reflects the necessary adjustment after changing how the address is handled. This enhances flexibility in formatting and displaying the address.

Comment on lines 98 to 152
impl Node {
/// Start the node.
///
/// This method will start all the node process, running them until the node is stopped.
pub async fn launch(self) -> Result<LaunchedNode> {
// Metrics recorder must be initialized before calling any of the metrics macros, in order
// for it to be registered.

if let Some(addr) = self.server_config.metrics {
let prometheus_handle = prometheus_exporter::install_recorder("katana")?;
let mut reports = Vec::new();

if let Some(ref db) = self.db {
reports.push(Box::new(db.clone()) as Box<dyn Report>);
}

prometheus_exporter::serve(
addr,
prometheus_handle,
metrics_process::Collector::default(),
reports,
)
.await?;

info!(%addr, "Metrics endpoint started.");
}

let pool = self.pool.clone();
let backend = self.backend.clone();
let block_producer = self.block_producer.clone();
let validator = self.block_producer.validator().clone();

// --- build sequencing stage

#[allow(deprecated)]
let sequencing = stage::Sequencing::new(
pool.clone(),
backend.clone(),
self.task_manager.clone(),
block_producer.clone(),
self.sequencer_config.messaging.clone(),
);

// --- build and start the pipeline

let mut pipeline = Pipeline::new();
pipeline.add_stage(Box::new(sequencing));

self.task_manager.spawn(pipeline.into_future());

let node_components = (pool, backend, block_producer, validator);
let rpc = spawn(node_components, self.server_config.clone()).await?;

Ok(LaunchedNode { node: self, rpc })
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ohayo, sensei! The launch method looks promising!

The Node::launch method does a great job of initializing all the necessary components. However, we can enhance it further with improved error handling and logging:

  1. Consider wrapping each major step (metrics initialization, pipeline building, RPC spawning) in separate functions. This will improve readability and make error handling more granular.

  2. Add more detailed logging throughout the launch process. This will make debugging easier in case of issues.

  3. Use tracing::instrument to automatically log entry and exit of the launch method along with its parameters.

Here's a sketch of how this could look:

use tracing::{info, instrument};

impl Node {
    #[instrument(skip(self), err)]
    pub async fn launch(self) -> Result<LaunchedNode> {
        info!("Starting node launch process");

        self.init_metrics().await?;
        let pipeline = self.build_pipeline()?;
        self.start_pipeline(pipeline);
        let rpc = self.spawn_rpc().await?;

        info!("Node launch completed successfully");
        Ok(LaunchedNode { node: self, rpc })
    }

    async fn init_metrics(&self) -> Result<()> {
        // Metrics initialization logic
    }

    fn build_pipeline(&self) -> Result<Pipeline> {
        // Pipeline building logic
    }

    fn start_pipeline(&self, pipeline: Pipeline) {
        // Pipeline starting logic
    }

    async fn spawn_rpc(&self) -> Result<RpcServer> {
        // RPC spawning logic
    }
}

This structure will make the code more modular and easier to maintain, sensei!

bin/katana/src/cli/node.rs Show resolved Hide resolved
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 57.52212% with 48 lines in your changes missing coverage. Please review.

Project coverage is 68.96%. Comparing base (1dad9ce) to head (0559702).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/katana/node/src/lib.rs 67.18% 21 Missing ⚠️
crates/katana/node/src/exit.rs 0.00% 15 Missing ⚠️
bin/katana/src/cli/node.rs 0.00% 8 Missing ⚠️
crates/katana/tasks/src/manager.rs 84.21% 3 Missing ⚠️
crates/dojo-test-utils/src/sequencer.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2504      +/-   ##
==========================================
- Coverage   68.96%   68.96%   -0.01%     
==========================================
  Files         372      376       +4     
  Lines       48568    48690     +122     
==========================================
+ Hits        33496    33580      +84     
- Misses      15072    15110      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
crates/katana/node/src/lib.rs (3)

88-98: Ohayo, sensei! Excellent encapsulation of node components!

The Node struct provides a clear and comprehensive representation of all the essential components of a node. The #[must_use] attribute is a nice touch to prevent accidental instantiation without usage.

Consider adding a brief doc comment for each field in the Node struct. This would enhance the struct's self-documentation and make it easier for other developers to understand the purpose of each component.


104-148: Ohayo, sensei! The launch method looks promising!

The launch method does a great job of initializing all the necessary components and starting the node. It maintains the state abstraction by returning a LaunchedNode instance.

Consider breaking down the launch method into smaller, more focused methods. This could improve readability and make the code easier to maintain. For example:

impl Node {
    pub async fn launch(self) -> Result<LaunchedNode> {
        self.init_metrics().await?;
        self.build_and_start_pipeline().await?;
        let rpc = self.spawn_rpc().await?;
        Ok(LaunchedNode { node: self, rpc })
    }

    async fn init_metrics(&self) -> Result<()> {
        // Metrics initialization logic
    }

    async fn build_and_start_pipeline(&self) -> Result<()> {
        // Pipeline building and starting logic
    }

    async fn spawn_rpc(&self) -> Result<RpcServer> {
        // RPC spawning logic
    }
}

This structure would make the code more modular and easier to maintain, sensei!


Line range hint 162-287: Ohayo, sensei! The build function is comprehensive but complex.

The build function does a thorough job of constructing all the necessary components for the Katana node. However, its complexity and the TODO comment suggest that it might benefit from some refactoring.

Consider breaking down this function into smaller, more focused functions for each major initialization step (e.g., building the executor factory, backend, block producer, etc.). This would improve readability and maintainability.

For example:

pub async fn build(
    server_config: ServerConfig,
    sequencer_config: SequencerConfig,
    starknet_config: StarknetConfig,
) -> Result<Node> {
    let executor_factory = build_executor_factory(&starknet_config);
    let (blockchain, db) = build_blockchain(&mut starknet_config).await?;
    let backend = build_backend(blockchain, executor_factory, &starknet_config);
    let block_producer = build_block_producer(&backend, &sequencer_config);
    let pool = build_transaction_pool(&block_producer);

    // ... other initialization steps ...

    Ok(Node { ... })
}

This structure would align with the TODO comment's suggestion of implementing a dedicated class for building node components, making the code more modular and easier to maintain in the future.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9f48bbf and 0559702.

📒 Files selected for processing (2)
  • crates/katana/node/src/lib.rs (5 hunks)
  • crates/metrics/src/prometheus_exporter.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
crates/metrics/src/prometheus_exporter.rs (1)

12-13: Ohayo, sensei! LGTM, but let's verify the impact.

The change to make PrometheusHandle publicly accessible looks good and aligns with the PR objectives. It enhances the flexibility of the metrics module.

To ensure this change doesn't introduce unintended consequences, let's verify its usage:

✅ Verification successful
<!-- [monologue]
[task review_file_1/1 review_changes_1/1 verification_1/1]
Analyze the results of the PrometheusHandle usage search.
[observations]
- The shell script found two usages of PrometheusHandle outside the `prometheus_exporter.rs` file.
  - In `crates/katana/node/src/lib.rs`, it's both imported and used as a public field.
- These usages indicate that `PrometheusHandle` is being utilized in a controlled and intended manner.
[analysis]
- The public exposure of `PrometheusHandle` allows other modules to interact with Prometheus metrics as designed.
- The specific usages in `katana/node/src/lib.rs` suggest that the handle is integrated where necessary without widespread or unintended access.
- No evidence of misuse or excessive exposure that could lead to potential issues.
[actions]
- Conclude that the change to make `PrometheusHandle` public is appropriate and does not introduce any unintended consequences.
- Approve the changes as they align with the PR's objectives and maintain codebase integrity.
-->

Ohayo, sensei! Verified that PrometheusHandle is used appropriately outside the prometheus_exporter.rs module. The changes are safe and align with our objectives.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any new usages of PrometheusHandle outside this module

# Test: Search for new PrometheusHandle usages
rg --type rust -g '!crates/metrics/src/prometheus_exporter.rs' 'PrometheusHandle'

Length of output: 243

crates/katana/node/src/lib.rs (1)

3-3: Ohayo, sensei! Nice abstraction for the launched node state!

The new exit module and LaunchedNode struct provide a clear separation between a node that has been launched and one that hasn't. This abstraction enhances the code's readability and maintainability.

Also applies to: 60-64

Comment on lines +70 to +80
pub async fn stop(&self) -> Result<()> {
// TODO: wait for the rpc server to stop instead of just stopping it.
self.rpc.handle.stop()?;
self.node.task_manager.shutdown().await;
Ok(())
}

/// Returns a future which resolves only when the node has stopped.
pub fn stopped(&self) -> NodeStoppedFuture<'_> {
NodeStoppedFuture::new(self)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo, sensei! Great implementation of node shutdown methods!

The stop and stopped methods provide a clean way to manage the node's lifecycle. However, there's room for improvement:

The TODO comment on line 71 suggests waiting for the RPC server to stop instead of just stopping it. This would ensure a more graceful shutdown. Consider implementing this enhancement to improve the robustness of the shutdown process.

Would you like assistance in implementing the logic to wait for the RPC server to fully stop, sensei?

@kariy kariy merged commit 3e51d5d into main Oct 8, 2024
14 of 15 checks passed
@kariy kariy deleted the katana/node branch October 8, 2024 04:25
steebchen added a commit that referenced this pull request Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant