refactor(katana-tasks): separate tasks spawning responsibility from TaskManager
#2514
Conversation
WalkthroughOhayo! This pull request introduces several modifications primarily focused on 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: 1
🧹 Outside diff range and nitpick comments (12)
crates/katana/pipeline/src/lib.rs (1)
49-52: Excellent logging enhancements, sensei!The modifications to the logging statements are spot-on:
- Specifying the "pipeline" target for the stage execution log improves log categorization.
- Adding a period at the end of the message enhances readability.
- The new log message for pipeline completion provides better visibility into the execution flow.
These changes will make our logs more informative and easier to parse. Dojo approved!
For even better consistency, consider adding the stage ID to the completion log as well:
- info!(target: "pipeline", "Pipeline finished."); + info!(target: "pipeline", "Pipeline finished. Total stages: {}.", self.stages.len());bin/katana/src/cli/node.rs (2)
Line range hint
441-484: Sugoi work on enhancing theprint_introfunction, sensei!The addition of JSON logging and the new helper functions
print_genesis_contractsandprint_genesis_accountsgreatly improve the clarity and structure of the genesis state output. This change makes it easier for users and automated tools to parse and understand the initial state of the blockchain.One small suggestion for consistency:
Consider using the
serde_json::to_string_prettyfunction for the JSON output to make it more readable when logged. Here's a quick example:- info!( - target: LOG_TARGET, - "{}", - serde_json::json!({ - "accounts": accounts.map(|a| serde_json::json!(a)).collect::<Vec<_>>(), - "seed": format!("{}", seed), - "address": format!("{address}"), - }) - ) + info!( + target: LOG_TARGET, + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "accounts": accounts.map(|a| serde_json::json!(a)).collect::<Vec<_>>(), + "seed": format!("{}", seed), + "address": format!("{address}"), + })).unwrap() + )This will make the JSON output more human-readable when logged.
Line range hint
486-557: Domo arigato for the new helper functions, sensei!The
print_genesis_contractsandprint_genesis_accountsfunctions greatly improve the clarity and structure of the genesis state output. The tabular format makes it easy for users to understand the initial state of the blockchain.One small suggestion for consistency:
Consider using a consistent style for displaying addresses and hashes. Currently, some are displayed with the
{:#064x}format specifier, while others use the default{}format. For consistency, you might want to use the same format for all addresses and hashes. Here's an example:- println!( - r" - | Contract | Fee Token - | Address | {} - | Class Hash | {:#064x}", - genesis.fee_token.address, genesis.fee_token.class_hash, - ); + println!( + r" + | Contract | Fee Token + | Address | {:#064x} + | Class Hash | {:#064x}", + genesis.fee_token.address, genesis.fee_token.class_hash, + );Apply this change throughout both functions for a consistent display of all addresses and hashes.
crates/katana/pipeline/src/stage/sequencing.rs (1)
50-51: Ohayo, sensei! Consider simplifying task handling whenmessaging_configisNone.Currently, when
messaging_configisNone, a task is spawned that waits indefinitely onfuture::pending::<()>(). You might consider not spawning a task in this case or handling it differently to avoid unnecessary resource usage.crates/katana/tasks/src/task.rs (2)
88-88: Ohayo sensei, please ensure task name is correctly logged even whennameisNoneSince
namecan beNone, consider handling this in the log statement to avoid loggingtask = None.
130-131: Consider handlingNonetask names more explicitly, senseiWhen the task name is
None, using an empty string may make logs less informative. Consider providing a default name or indicating when a task name is not provided.crates/katana/tasks/src/lib.rs (4)
25-25: Reminder: Replace withTaskSpawnerinmanager.rsOhayo sensei! There's a
TODOcomment indicating that this code should be replaced withTaskSpawnerinmanager.rs. Would you like me to assist in implementing this change or open a GitHub issue to track it?
26-28: Correct grammatical errors in documentation commentsOhayo sensei! I noticed some typos in the documentation:
- "This is simple wrapper" should be "This is a simple wrapper".
- "a tokio runtime" should be "the Tokio runtime".
- "a tokio's runtime" should be "Tokio's runtime".
Apply this diff to fix the typos:
/// A task spawner for spawning tasks on a tokio runtime. This is simple wrapper around a tokio's -/// runtime [Handle] to easily spawn tasks on the runtime. +/// runtime [Handle] to easily spawn tasks on the runtime. +/// +/// This is a simple wrapper around the Tokio runtime. It uses Tokio's +/// runtime [Handle] to easily spawn tasks on the runtime.
Line range hint
35-58: Consider merging multipleimplblocks forTokioTaskSpawnerOhayo sensei! I noticed that
TokioTaskSpawnerhas multipleimplblocks. For better readability and maintainability, consider merging them into a singleimplblock.
Line range hint
77-79: Fix typo in documentation commentOhayo sensei! There's a small typo in the documentation: "spawing" should be "spawning".
Apply this diff to correct the typo:
/// For spawing blocking IO-bound tasks, use [TokioTaskSpawner::spawn_blocking] instead. -/// For spawing blocking IO-bound tasks, use [TokioTaskSpawner::spawn_blocking] instead. +/// For spawning blocking IO-bound tasks, use [TokioTaskSpawner::spawn_blocking] instead.crates/katana/tasks/src/manager.rs (2)
211-211: Accessing internal fields in tests may break encapsulation.In the test,
assert_eq!(manager.inner.tracker.len(), 3);directly accessesmanager.inner.tracker. To maintain encapsulation, consider adding a public method or accessor inTaskManagerto expose the required information for testing purposes.
217-217: Ohayo, sensei! Prefer using public methods to check cancellation status in tests.The assertion
!manager.inner.on_cancel.is_cancelled()accesses theon_canceltoken directly. For better encapsulation, add a method toTaskManagerlikeis_cancelled()that returns the cancellation status, and use it in the test.
📜 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 (8)
- bin/katana/src/cli/node.rs (1 hunks)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/pipeline/Cargo.toml (1 hunks)
- crates/katana/pipeline/src/lib.rs (3 hunks)
- crates/katana/pipeline/src/stage/sequencing.rs (4 hunks)
- crates/katana/tasks/src/lib.rs (1 hunks)
- crates/katana/tasks/src/manager.rs (8 hunks)
- crates/katana/tasks/src/task.rs (6 hunks)
🧰 Additional context used
🔇 Additional comments (24)
crates/katana/pipeline/Cargo.toml (1)
18-18: Ohayo! Nice addition of Tokio, sensei!The inclusion of Tokio as a workspace dependency is a solid choice. This asynchronous runtime will likely enhance our task management capabilities, aligning perfectly with our mission to separate task spawning responsibilities.
crates/katana/pipeline/src/lib.rs (3)
9-9: Ohayo, sensei! Nice addition to the tracing imports!Adding the
errormacro alongsideinfois a great move. It enhances our error handling capabilities, allowing us to log errors more effectively. This change aligns well with the new error logging implemented later in the file.
62-66: Ohayo! Impressive error handling upgrade, sensei!The addition of
inspect_errin theinto_futureimplementation is a masterful touch:
- It provides specific error logging for pipeline failures, enhancing our ability to debug issues.
- The use of
inspect_erris a clean and efficient way to add logging without altering the future's result.- Including the error details in the log message will be invaluable for troubleshooting.
This change significantly improves the robustness of our pipeline execution. Well done!
Line range hint
1-87: Ohayo, sensei! Overall assessment of the Pipeline enhancementsThe changes made to the
Pipelinestruct and its associated methods are excellent. They significantly improve logging and error handling, which aligns perfectly with the PR objectives of enhancing task management. Here's a summary of the improvements:
- Enhanced import statements for better tracing capabilities.
- Improved logging in the
runmethod for clearer execution tracking.- Robust error handling in the
into_futureimplementation.These modifications will make the pipeline more maintainable, easier to debug, and more informative during execution. Great work on refining the Katana project's core functionality!
bin/katana/src/cli/node.rs (1)
259-262: Ohayo! Nice touch on the logging configuration, sensei!The updated default log filter now includes "tasks=debug" at the beginning, which aligns well with the PR's focus on enhancing task management. This change will provide more detailed logs for task-related operations, making it easier to debug and monitor the new task spawning system.
crates/katana/pipeline/src/stage/sequencing.rs (6)
11-12: Ohayo, sensei! Imports for task handling are correctly added.The addition of
TaskHandleandTaskSpawnerimports is appropriate for the new task spawning functionality.
22-22: Fieldtask_spawnercorrectly replacestask_manager.Updating the struct to use
task_spawneraligns with the refactoring goals to separate task spawning responsibilities.
31-35: Constructor updated to acceptTaskSpawner.The
newfunction now correctly acceptstask_spawnerand initializes the struct accordingly.
38-51:run_messagingmethod correctly returnsTaskHandle<()>.The method now returns
TaskHandle<()>, allowing for proper task management and monitoring. The use oftask_spawnerto spawn theMessagingTaskis well-implemented.
55-61:run_block_productionmethod appropriately spawns the block production task.Changing
run_block_productionto returnTaskHandle<()>and usingtask_spawnerto spawn the task aligns with the refactoring objectives.
73-90: Execution logic effectively manages task lifecycles.The
executemethod now usestokio::select!to monitor bothmessagingandblock_productiontasks. This ensures that if either task finishes unexpectedly, an error is logged, and appropriate actions can be taken.crates/katana/tasks/src/task.rs (5)
36-36: Ohayo sensei! UpdatingTaskBuilderto useTaskSpawneris appropriateThis change aligns with the refactoring objectives to separate task spawning responsibilities.
80-80: Ohayo! Updated destructuring ofselfis appropriate, senseiThis change correctly reflects the updated fields in
TaskBuilder.
85-85: Adjusted cancellation token retrieval fromspawneris appropriateThis modification aligns with the refactoring to use
TaskSpawner, sensei.
103-103: Update to usespawner.spawn(fut)is appropriate, senseiThis change correctly uses the
TaskSpawnerto spawn the future.
48-49: EnsureTaskBuilder::newis updated throughout the codebase, senseiSince
TaskBuilder::newnow takesTaskSpawnerinstead ofTaskManager, please make sure all instantiations are updated accordingly.Run the following script to verify all calls to
TaskBuilder::newuseTaskSpawner:crates/katana/tasks/src/manager.rs (6)
65-67: Ohayo, sensei! Great job on introducingTaskSpawnerfor better separation of concerns.The new method
task_spawnerand theTaskSpawnerstruct effectively separate task spawning responsibilities fromTaskManager. This aligns well with the PR objectives to resolve issues related to shared cancellation tokens and enhances the modularity of the task management system.
145-145: Ensure thread safety when tracking futures across spawners.Using
self.inner.tracker.track_future(task);withinTaskSpawneris appropriate. Confirm thatTaskTrackeris thread-safe and can handle concurrent tracking from multipleTaskSpawnerinstances without any race conditions or data inconsistencies.
169-170: Ohayo, sensei! Confirm the cancellation behavior inTaskManager'sDropimplementation.Calling
self.inner.on_cancel.cancel();in theDropimplementation ensures that all ongoing tasks are cancelled when theTaskManageris dropped. Verify that this behavior is intentional and that droppingTaskManagerprematurely won't lead to unintended task cancellations elsewhere in the system.
245-245: Ohayo, sensei! Verify that tasks respect the graceful shutdown signal.When spawning a task with
graceful_shutdown(), ensure that all other tasks are cancelled appropriately and that the shutdown process waits for the graceful task to complete. Confirm that resources are cleaned up correctly and no tasks linger after shutdown.
261-261: Ensure panics in critical tasks trigger the intended shutdown behavior.In
critical_task_graceful_shutdown_on_panicked, a critical task intentionally panics. Verify that theTaskManagerhandles this panic by initiating a graceful shutdown without causing unwanted side effects or leaving tasks in an inconsistent state.Run the following script to confirm panic handling:
31-31: Ohayo, sensei! Verify the removal of theClonetrait fromTaskManager.The
Clonetrait has been removed fromTaskManager(#[derive(Debug, Clone)]→#[derive(Debug)]). Ensure that no parts of the codebase rely on cloningTaskManager. If cloning is still required elsewhere, consider whetherTaskSpawner, which implementsClone, can serve as a replacement.Run the following script to check for any cloning of
TaskManager:✅ Verification successful
Ohayo, sensei! It seems the initial check didn't find any cloning of
TaskManager, but to be sure, let's dive a bit deeper. 🧐
Ohayo, sensei! 🎉
Cloning ofTaskManagerhas been successfully removed, andTaskSpawneris now being used as its replacement throughout the codebase. All checks confirm that no parts of the code rely on cloningTaskManager.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `TaskManager` cloning. # Expected result: There should be no instances where `TaskManager` is cloned. rg 'TaskManager.*\.clone\(\)'Length of output: 31
Script:
#!/bin/bash # Description: Search for manual implementations of the `Clone` trait for `TaskManager`. # Expected result: There should be no manual `Clone` implementations for `TaskManager`. rg 'impl\s+Clone\s+for\s+TaskManager'Length of output: 39
Script:
#!/bin/bash # Description: Search for any indirect cloning of `TaskManager` through other structs or wrappers. # Expected result: There should be no indirect cloning of `TaskManager`. rg '.*clone\(\).*TaskManager'Length of output: 31
Script:
#!/bin/bash # Description: Verify if `TaskSpawner` is used as a replacement for cloning `TaskManager`. # Expected result: Instances where `TaskSpawner` is used instead of cloning `TaskManager`. rg 'TaskSpawner'Length of output: 3272
crates/katana/node/src/lib.rs (2)
135-135: Ohayo sensei! Good use oftask_spawner()for improved task management.Switching to
self.task_manager.task_spawner()enhances the separation of responsibilities by delegating task spawning to theTaskSpawner. This aligns perfectly with the refactoring goals and improves the code's clarity.
145-150: Ohayo sensei! Task spawning with builder pattern enhances configurability.Utilizing the builder pattern with
build_task().critical().name("Pipeline").spawn(pipeline.into_future())increases the configurability and readability of task spawning. This approach makes it clear which tasks are critical and aids in monitoring and debugging.
|
tests failing caused by not displaying the actual port address that has been binded by the rpc server. but instead display the raw port specified from the config. this is an issue in the case where port number is set to 0 (ie when requesting for random port). as the node-bindings relies on the address displayed in the startup log. fixed in #2518 |
kariy
force-pushed
the
katana/tasks-spawner
branch
from
0d77872 to
7858a28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
crates/katana/tasks/src/task.rs (1)
Line range hint
1-165: Ohayo! Overall refactoring looks stellar, sensei!The changes consistently implement the separation of task spawning responsibility from
TaskManagertoTaskSpawner. This new structure should effectively resolve the issue with unintended task cancellations when aTaskManagerinstance is dropped.The addition of debug logging for tasks with graceful shutdown enhances observability, which is a nice touch.
One minor suggestion to consider:
Consider adding a brief comment explaining the purpose of
TaskSpawnerand its relationship toTaskManagerat the top of the file. This would provide helpful context for future developers working on this code.crates/katana/tasks/src/manager.rs (2)
51-57: Ohayo! Nice implementation of the new structure, sensei!The
newmethod correctly initializes theinnerfield with anArc<Inner>, and thetask_spawnermethod provides a clean way to create aTaskSpawner. These changes align well with the refactoring objectives.A small suggestion: Consider adding a comment in the
task_spawnermethod to explain that it creates a newTaskSpawnerinstance each time it's called, if that's the intended behavior.Also applies to: 65-67
111-136: Ohayo! Excellent introduction of TaskSpawner, sensei!The new
TaskSpawnerstruct and its methods align perfectly with the PR objectives. It provides a clean API for task management and spawning, separating these responsibilities fromTaskManager.A small suggestion: Consider adding a brief comment to the
cancellation_tokenmethod explaining when and why a user might need to access it directly.crates/katana/node/src/lib.rs (1)
145-150: Ohayo, pipeline sensei! Impressive task spawning upgrade.The new implementation for spawning the pipeline task is a significant improvement. It provides more granular control over the task creation process, allowing for properties like criticality and name to be set. This aligns well with the PR's goal of enhancing task management.
One small suggestion to consider:
Consider extracting the task name "Pipeline" into a constant at the top of the file. This would improve maintainability if the name needs to be referenced elsewhere or changed in the future. For example:
const PIPELINE_TASK_NAME: &str = "Pipeline"; // Then use it in the task spawning: .name(PIPELINE_TASK_NAME)What do you think, sensei? Would you like me to prepare a small PR for this change?
crates/katana/tasks/src/lib.rs (4)
25-25: Ohayo! Remember to address the TODO commentThere's a
TODOindicating thatTokioTaskSpawnershould be replaced withTaskSpawnerinmanager.rs. To keep the codebase clean and prevent technical debt, please consider resolving this.Do you need assistance implementing this change, or would you like me to open a new GitHub issue to track this task, sensei?
Line range hint
27-29: Correct grammatical errors in documentationIn the documentation for
TokioTaskSpawner, there are some grammatical errors:
- "This is simple wrapper around a tokio's runtime [Handle]" should be "This is a simple wrapper around Tokio's runtime [Handle]".
Apply this diff to fix the grammatical errors:
-/// A task spawner for spawning tasks on a tokio runtime. This is simple wrapper around a tokio's -/// runtime [Handle] to easily spawn tasks on the runtime. +/// A task spawner for spawning tasks on a Tokio runtime. This is a simple wrapper around Tokio's +/// runtime [Handle] to easily spawn tasks on the runtime.
Line range hint
37-54: Consider merging multipleimplblocks forTokioTaskSpawnerCurrently,
TokioTaskSpawnerhas two separateimplblocks. While Rust allows multipleimplblocks, merging them can improve readability and maintainability.
Line range hint
78-83: Fix typo in documentationIn the
BlockingTaskPooldocumentation, there's a typo:
- "For spawing blocking IO-bound tasks..." should be "For spawning blocking IO-bound tasks..."
Apply this diff to correct the typo:
-/// CPU-bound tasks. For spawing blocking IO-bound tasks, use [TokioTaskSpawner::spawn_blocking] +/// CPU-bound tasks. For spawning blocking IO-bound tasks, use [TokioTaskSpawner::spawn_blocking]
📜 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 (9)
- bin/katana/src/cli/node.rs (1 hunks)
- crates/katana/node-bindings/src/lib.rs (1 hunks)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/pipeline/Cargo.toml (1 hunks)
- crates/katana/pipeline/src/lib.rs (3 hunks)
- crates/katana/pipeline/src/stage/sequencing.rs (4 hunks)
- crates/katana/tasks/src/lib.rs (1 hunks)
- crates/katana/tasks/src/manager.rs (8 hunks)
- crates/katana/tasks/src/task.rs (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- bin/katana/src/cli/node.rs
- crates/katana/pipeline/Cargo.toml
- crates/katana/pipeline/src/lib.rs
- crates/katana/pipeline/src/stage/sequencing.rs
🧰 Additional context used
🔇 Additional comments (13)
crates/katana/tasks/src/task.rs (4)
9-9: Ohayo! New imports look good, sensei!The addition of
debugfromtracingandTaskSpawneraligns well with the refactoring objectives. These imports support the new logging functionality and the transition fromTaskManagertoTaskSpawner.Also applies to: 12-12
36-36: TaskBuilder update looks sharp, sensei!The replacement of
TaskManagerwithTaskSpawnerin theTaskBuilderstruct and itsnewmethod aligns perfectly with the refactoring goals. This change effectively separates the task spawning responsibility, enhancing the overall architecture.Also applies to: 48-49
80-80: Ohayo! TaskBuilder::spawn method is looking mighty fine, sensei!The updates to the
spawnmethod are spot-on:
- Using
spawnerinstead ofmanagermaintains consistency with the new architecture.- Obtaining the cancellation token from
spawneraligns with the refactoring goals.- The new debug logging enhances observability for tasks with graceful shutdown.
- Task spawning through
spawnercompletes the transition to the new structure.These changes effectively implement the separation of task spawning responsibility.
Also applies to: 85-85, 88-88, 103-103
130-131: CriticalTaskBuilder changes are on point, sensei!The updates to
CriticalTaskBuilderare well-executed:
- Separating the task name retrieval enhances code readability.
- Obtaining the cancellation token from
spawneraligns perfectly with the newTaskSpawnerstructure.These modifications successfully integrate the
CriticalTaskBuilderwith the refactored task management system.crates/katana/tasks/src/manager.rs (6)
22-30: Ohayo! Documentation update looks great, sensei!The updated documentation for
TaskManagerclearly explains the new task spawning mechanism usingTaskSpawner. This change aligns well with the refactoring objectives and provides clear guidance for users.
31-34: Excellent refactoring, sensei!The
TaskManagerstruct has been streamlined to use anArc<Inner>, which should improve shared access to the manager's state. The removal of theClonederive is appropriate sinceArcalready provides the necessary sharing mechanism.
36-45: Ohayo! Great job on improving encapsulation, sensei!The introduction of the
Innerstruct is a solid design choice. It nicely encapsulates the internal state ofTaskManager, separating it from the public interface. This should make the code more maintainable and easier to reason about.
Line range hint
72-108: Ohayo! Consistent updates to methods, sensei!The changes to
wait_for_shutdown,shutdown,handle, andwaitmethods are well-implemented. They correctly access the fields through the newinnerstruct, maintaining proper encapsulation. This consistency helps in keeping the code clean and easy to understand.
145-146: Ohayo! Nice update to task tracking, sensei!The change in the
spawn_innermethod to useself.inner.trackerfor tracking the task is spot on. It ensures that tasks are properly managed using the encapsulatedtracker, maintaining consistency with the new structure.
169-170: Ohayo! Great updates to Drop and tests, sensei!The addition of a trace log in the
Dropimplementation forTaskManageris a nice touch for debugging. The test cases have been correctly updated to use the newTaskSpawnerAPI, which is crucial for ensuring the refactored code works as expected.Regarding the past comment about potential hangs in the
task_with_graceful_shutdowntest:The infinite loops in the spawned tasks still don't check for cancellation, which could potentially lead to hangs if the shutdown mechanism fails. Consider adding cancellation checks within these loops to ensure graceful termination.
You could modify the loops like this:
spawner.build_task().spawn(async { loop { + if spawner.cancellation_token().is_cancelled() { + break; + } time::sleep(Duration::from_secs(1)).await; } });This change would make the tests more robust and prevent potential hangs.
Also applies to: 204-261
crates/katana/node/src/lib.rs (2)
135-137: Ohayo, task management sensei! Excellent refactor.The change from
self.task_manager.clone()toself.task_manager.task_spawner()is a great improvement. It aligns perfectly with the PR objective of separating task spawning responsibilities. This focused approach to task management should lead to better control over task lifecycles and improved overall system stability.
Line range hint
1-450: Ohayo, Katana sensei! Overall assessment of the refactor.The changes implemented in this file successfully address the main objectives of the PR. The separation of task spawning responsibilities from
TaskManagertoTaskSpawneris well executed. This refactoring should resolve the issue with task cancellation mentioned in the PR objectives, where dropping aTaskManagerinstance was causing unintended cancellations of tasks in other managers.The new implementation provides better control over task creation and management, which should lead to improved stability and maintainability of the system. The changes are consistent and well-integrated into the existing codebase.
Great work, sensei! This refactor is a significant step forward for the Katana project.
crates/katana/node-bindings/src/lib.rs (1)
625-625: Ohayo, sensei! The code looks good here.Initializing Katana with
Katana::new().spawn();is appropriate and correctly spawns the instance with default settings.
| // try to connect as a provider | ||
| let provider = JsonRpcClient::new(HttpTransport::new(dbg!(katana.endpoint_url()))); | ||
| assert!(provider.chain_id().await.is_ok()) |
There was a problem hiding this comment.
Ohayo, sensei! Consider removing the dbg! macro for cleaner test output.
The dbg! macro is useful for debugging but can clutter test outputs with unnecessary information. It's best to remove it before merging to keep the test output clean.
Apply this diff to remove the dbg! macro:
-let provider = JsonRpcClient::new(HttpTransport::new(dbg!(katana.endpoint_url())));
+let provider = JsonRpcClient::new(HttpTransport::new(katana.endpoint_url()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // try to connect as a provider | |
| let provider = JsonRpcClient::new(HttpTransport::new(dbg!(katana.endpoint_url()))); | |
| assert!(provider.chain_id().await.is_ok()) | |
| // try to connect as a provider | |
| let provider = JsonRpcClient::new(HttpTransport::new(katana.endpoint_url())); | |
| assert!(provider.chain_id().await.is_ok()) |
the idea is to separate task management into two components;
TaskManagerandTaskSpawner.TaskSpawneris derived from aTaskManagerand thus can only be created from aTaskManagerand is used to spawn any tasks to its associated manager.the issue that prompted this refactor is when due to the
Dropimplementation ofTaskManager, where it will cancel all ongoing tasks. becauseTaskManagerwas cloned around, the underlying cancellation token is the same across all instances ofTaskManager, and thus when one instance is dropped, it promptly cancelled all tasks of all manager.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor