feat(sozo): provider health check #2745
Conversation
WalkthroughThe pull request introduces significant enhancements to error handling and control flow across several files, particularly focusing on the health check of the provider. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (3)
bin/sozo/src/commands/migrate.rs (1)
144-150: Ohayo! Let's make the error message more user-friendly, sensei! 🎋While the error handling is good, we can make the message more helpful by providing specific troubleshooting steps:
if chain_id_result.is_err() { let error_info = - format!("Unhealthy provider {:?}, please check your configuration.", provider); + format!("Unable to connect to provider {:?}. Please ensure:\n\ + 1. The provider service is running\n\ + 2. The RPC URL is correct in your configuration\n\ + 3. Your network connection is stable", provider); return Err(anyhow::anyhow!(error_info)); }bin/sozo/src/utils.rs (2)
118-124: Ohayo! Let's enhance the error message here too, sensei! 🍜Similar to the previous file, we can make the error message more helpful:
if spec_version_result.is_err() { let error_info = - format!("Unhealthy provider {:?}, please check your configuration.", provider); + format!("Failed to retrieve spec version from provider {:?}. Please verify:\n\ + 1. The provider service is running and healthy\n\ + 2. The provider supports spec version {}\n\ + 3. Your configuration is correct", provider, RPC_SPEC_VERSION); return Err(anyhow::anyhow!(error_info)); }
Line range hint
118-150: Overall implementation feedback, sensei! 🎋The changes successfully implement health checks in both files with consistent error handling patterns. This addresses the core issue of providing better prompts when the Sozo service is unhealthy. The implementation is solid, and with the suggested error message improvements, it will provide an even better user experience.
Consider creating a shared error handling module to maintain consistency in provider health check messages across the codebase. This would help standardize error messages and make future maintenance easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/sozo/src/commands/migrate.rs(1 hunks)bin/sozo/src/utils.rs(1 hunks)
🔇 Additional comments (1)
bin/sozo/src/utils.rs (1)
Line range hint 267-309: Excellent test coverage for version compatibility, sensei! 🎯
The test suite thoroughly covers various version compatibility scenarios, including the specific backward compatibility case for versions 0.6 and 0.7.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
==========================================
- Coverage 56.00% 55.99% -0.02%
==========================================
Files 434 435 +1
Lines 55090 55102 +12
==========================================
+ Hits 30854 30855 +1
- Misses 24236 24247 +11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
In sozo, we have multiple places where the provider is used (migrate, execute, and even the inspect command if the diff is taken from the chain).
In consequences, it would be better to factorise the code into the dojo/utils crate and use the same in torii and here. 👍
Don't hesitate if you have any question.
fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
crates/dojo/utils/src/utils.rs (1)
8-15: Consider enhancing the success path loggingWhile the trace logging is good, it could be more informative for debugging purposes.
Consider adding more context in the success case:
trace!( latest_block = ?block, + block_number = ?block.block_number, + timestamp = ?block.timestamp, "Provider health check." );crates/torii/core/src/utils.rs (1)
Line range hint
1-400: Ohayo sensei! The removal of health_check_provider is a good architectural decision.Moving the health check functionality to
dojo_utils::utilsimproves code organization by centralizing this common functionality in a shared location.crates/torii/core/src/engine.rs (1)
Line range hint
391-395: Enhance error logging for better user experience.While the health check implementation is good, we could provide more specific error information to help users diagnose issues.
Consider enhancing the error logging:
- error!(target: LOG_TARGET,"Provider health check failed during engine start"); + error!( + target: LOG_TARGET, + error = %e, + "Provider health check failed during engine start. Please ensure the provider is running and accessible" + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
bin/sozo/src/commands/migrate.rs(3 hunks)bin/sozo/src/commands/mod.rs(2 hunks)bin/sozo/src/utils.rs(3 hunks)crates/dojo/utils/src/lib.rs(1 hunks)crates/dojo/utils/src/utils.rs(1 hunks)crates/torii/core/Cargo.toml(1 hunks)crates/torii/core/src/engine.rs(1 hunks)crates/torii/core/src/utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/sozo/src/utils.rs
- bin/sozo/src/commands/migrate.rs
🔇 Additional comments (6)
crates/dojo/utils/src/lib.rs (1)
17-17: Ohayo! LGTM - Clean module addition, sensei!
The new public utils module is well-placed and follows Rust's module organization conventions.
crates/dojo/utils/src/utils.rs (1)
5-7: Ohayo! Well-structured function signature, sensei!
The generic implementation with appropriate trait bounds (Provider + Sync + std::fmt::Debug) allows for flexible provider types while ensuring necessary functionality.
crates/torii/core/Cargo.toml (1)
21-21: Ohayo! Clean dependency addition, sensei!
The dojo-utils dependency is correctly added with workspace inheritance, maintaining consistent versioning across the project.
bin/sozo/src/commands/mod.rs (2)
37-38: Ohayo sensei! Good addition of LOG_TARGET constant.
Adding a consistent logging target improves log organization and filtering capabilities.
47-50: Documentation formatting improvement looks good!
The multi-line format improves readability of the command documentation.
crates/torii/core/src/engine.rs (1)
9-9: Import of health_check_provider looks good!
The import from dojo_utils aligns with the architectural changes.
crates/dojo/utils/src/utils.rs
Outdated
| Err(_) => { | ||
| let error_info = | ||
| format!("Unhealthy provider {:?}, please check your configuration.", provider); | ||
| Err(anyhow::anyhow!(error_info)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with more actionable messages
The current error message indicates the provider is unhealthy but could be more helpful to users.
Consider providing more specific guidance:
Err(_) => {
let error_info =
- format!("Unhealthy provider {:?}, please check your configuration.", provider);
+ format!(
+ "Provider {:?} is not responding. Please ensure:\n\
+ 1. The provider URL is correct in your configuration\n\
+ 2. The provider service is running and accessible\n\
+ 3. Your network connection is stable",
+ provider
+ );
Err(anyhow::anyhow!(error_info))
}📝 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.
| Err(_) => { | |
| let error_info = | |
| format!("Unhealthy provider {:?}, please check your configuration.", provider); | |
| Err(anyhow::anyhow!(error_info)) | |
| } | |
| } | |
| Err(_) => { | |
| let error_info = | |
| format!( | |
| "Provider {:?} is not responding. Please ensure:\n\ | |
| 1. The provider URL is correct in your configuration\n\ | |
| 2. The provider service is running and accessible\n\ | |
| 3. Your network connection is stable", | |
| provider | |
| ); | |
| Err(anyhow::anyhow!(error_info)) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
bin/sozo/src/commands/migrate.rs (1)
149-152: Enhance error message for better user experience, sensei!While the health check implementation is solid, we could make the error message more user-friendly to align with the PR's objective of providing clearer prompts.
Consider enhancing the error message:
-error!(target: LOG_TARGET,"Provider health check failed during sozo migrate."); +error!( + target: LOG_TARGET, + "Provider health check failed during sozo migrate. Please ensure that:\n\ + 1. The Sozo service is running\n\ + 2. The RPC endpoint is accessible\n\ + 3. Your network connection is stable" +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/sozo/src/commands/migrate.rs(3 hunks)bin/sozo/src/utils.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/sozo/src/utils.rs
🔇 Additional comments (2)
bin/sozo/src/commands/migrate.rs (2)
1-6: Ohayo! Clean import additions, sensei!
The new imports for Arc and health_check_provider align perfectly with the PR objectives of improving provider health checks.
148-154: Verify error scenarios, sensei!
While the implementation looks solid, let's ensure we've covered all error scenarios for a robust user experience.
✅ Verification successful
Provider health check implementation is consistent, sensei!
Based on the search results, the error handling for provider health checks is consistently implemented across the codebase:
- In
torii/core/src/engine.rs - In
sozo/src/utils.rs - In
sozo/src/commands/migrate.rs
All implementations follow the same pattern:
- Health check with proper error propagation
- Consistent error logging with
LOG_TARGET - Arc unwrapping after successful health check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other provider health check implementations to ensure consistency
rg -A 5 "health_check_provider" --type rust
# Look for other error handling patterns we might want to align with
rg -A 5 "error!\(target: LOG_TARGET" --type rust
Length of output: 18852
| let provider = Arc::try_unwrap(provider).map_err(|_| anyhow!("Failed to unwrap Arc"))?; | ||
| let chain_id = provider.chain_id().await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing Arc usage, sensei!
The current implementation wraps and immediately unwraps the provider. Since the health check is the only async operation needing the Arc, we could optimize this.
Consider this more efficient approach:
-let provider = Arc::new(provider);
-if let Err(e) = health_check_provider(provider.clone()).await {
- error!(target: LOG_TARGET,"Provider health check failed during sozo migrate.");
- return Err(e);
-}
-let provider = Arc::try_unwrap(provider).map_err(|_| anyhow!("Failed to unwrap Arc"))?;
+if let Err(e) = health_check_provider(Arc::new(provider.clone())).await {
+ error!(target: LOG_TARGET,"Provider health check failed during sozo migrate.");
+ return Err(e);
+}This approach:
- Avoids unnecessary Arc unwrapping
- Reduces potential for errors
- Maintains the same functionality
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Awesome, thank you @847850277 for the work here!
Made some minor changes, good to go!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/dojo/utils/src/provider.rs (2)
5-9: Enhance documentation with more details, sensei!The current documentation could be more helpful by including:
- What defines a "healthy" provider
- Possible error scenarios
- Usage examples
Here's a suggested improvement:
/// Check if the provider is healthy. /// -/// This function will check if the provider is healthy by getting the latest block, -/// and returns an error otherwise. +/// This function verifies provider health by attempting to fetch the latest block. +/// A provider is considered healthy if it can successfully return block information. +/// +/// # Errors +/// Returns an error if: +/// - The provider fails to connect +/// - The provider fails to retrieve the latest block +/// +/// # Example +/// ``` +/// use starknet::providers::JsonRpcClient; +/// +/// let provider = JsonRpcClient::new("http://localhost:5050"); +/// match health_check_provider(provider).await { +/// Ok(_) => println!("Provider is healthy"), +/// Err(e) => println!("Provider is unhealthy: {}", e), +/// } +/// ```
9-11: Consider these signature improvements, sensei!
- Consider taking the provider by reference to preserve caller's ownership
- The 'static bound might be unnecessarily restrictive
- Consider returning the block info on success for potential caller use
Here's a suggested improvement:
-pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( - provider: P, -) -> anyhow::Result<(), anyhow::Error> +pub async fn health_check_provider<'a, P>( + provider: &'a P, +) -> anyhow::Result<starknet::core::types::Block> +where + P: Provider + Sync + std::fmt::Debug
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
examples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
bin/sozo/src/commands/migrate.rs(3 hunks)bin/sozo/src/utils.rs(3 hunks)crates/dojo/utils/src/lib.rs(1 hunks)crates/dojo/utils/src/provider.rs(1 hunks)crates/torii/core/src/engine.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/dojo/utils/src/lib.rs
- bin/sozo/src/utils.rs
- bin/sozo/src/commands/migrate.rs
- crates/torii/core/src/engine.rs
🔇 Additional comments (1)
crates/dojo/utils/src/provider.rs (1)
1-3: Ohayo! Clean and precise imports, sensei!
The imports are well-organized and specifically targeted to the functionality needed.
| use starknet::core::types::{BlockId, BlockTag}; | ||
| use starknet::providers::Provider; | ||
| use tracing::trace; | ||
|
|
||
| /// Check if the provider is healthy. | ||
| /// | ||
| /// This function will check if the provider is healthy by getting the latest block, | ||
| /// and returns an error otherwise. | ||
| pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( | ||
| provider: P, | ||
| ) -> anyhow::Result<(), anyhow::Error> { | ||
| match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await { | ||
| Ok(block) => { | ||
| trace!( | ||
| latest_block = ?block, | ||
| "Provider health check." | ||
| ); | ||
| Ok(()) | ||
| } | ||
| Err(_) => { | ||
| let error_info = | ||
| format!("Unhealthy provider {:?}, please check your configuration.", provider); | ||
| Err(anyhow::anyhow!(error_info)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Tests needed for provider health checks, sensei!
The provider health check implementation currently lacks test coverage. No test module exists in provider.rs, and there are no dedicated test files for this functionality. Consider adding:
- Unit tests to verify successful health checks
- Tests for error scenarios with unhealthy providers
- Mock provider implementations for controlled testing
🔗 Analysis chain
Verify test coverage, sensei!
The implementation aligns well with the PR objectives, but let's ensure proper test coverage exists.
Would you like help creating integration tests for different provider scenarios?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test modules or integration tests
rg -g '*.rs' -l 'mod tests' crates/dojo/utils/src/
fd -g '*_test.rs' crates/dojo/utils/tests/
Length of output: 275
Script:
#!/bin/bash
# Let's check if there are any tests in provider.rs itself
rg -g 'provider.rs' 'mod tests' crates/dojo/utils/src/
# Let's also check if there are any test files that might test the provider functionality
fd -g '*provider*test*.rs' crates/dojo/
# Let's see if there are any usages of health_check_provider in test files
rg -l "health_check_provider" crates/dojo/
Length of output: 173
| match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await { | ||
| Ok(block) => { | ||
| trace!( | ||
| latest_block = ?block, | ||
| "Provider health check." | ||
| ); | ||
| Ok(()) | ||
| } | ||
| Err(_) => { | ||
| let error_info = | ||
| format!("Unhealthy provider {:?}, please check your configuration.", provider); | ||
| Err(anyhow::anyhow!(error_info)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging, sensei!
The implementation could be improved in several ways:
- Preserve the original error information
- Add more detailed logging
- Be cautious with provider debug information
Here's a suggested improvement:
- match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await {
- Ok(block) => {
- trace!(
- latest_block = ?block,
- "Provider health check."
- );
- Ok(())
- }
- Err(_) => {
- let error_info =
- format!("Unhealthy provider {:?}, please check your configuration.", provider);
- Err(anyhow::anyhow!(error_info))
- }
- }
+ let result = provider
+ .get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest))
+ .await;
+
+ match &result {
+ Ok(block) => {
+ trace!(
+ block_number = ?block.block_number,
+ block_hash = ?block.block_hash,
+ "Provider health check successful"
+ );
+ }
+ Err(e) => {
+ tracing::error!(
+ error = %e,
+ "Provider health check failed"
+ );
+ }
+ }
+
+ result.map_err(|e| anyhow::anyhow!(
+ "Provider health check failed: {}. Please check your configuration.",
+ e
+ ))📝 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.
| match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await { | |
| Ok(block) => { | |
| trace!( | |
| latest_block = ?block, | |
| "Provider health check." | |
| ); | |
| Ok(()) | |
| } | |
| Err(_) => { | |
| let error_info = | |
| format!("Unhealthy provider {:?}, please check your configuration.", provider); | |
| Err(anyhow::anyhow!(error_info)) | |
| } | |
| } | |
| } | |
| let result = provider | |
| .get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)) | |
| .await; | |
| match &result { | |
| Ok(block) => { | |
| trace!( | |
| block_number = ?block.block_number, | |
| block_hash = ?block.block_hash, | |
| "Provider health check successful" | |
| ); | |
| } | |
| Err(e) => { | |
| tracing::error!( | |
| error = %e, | |
| "Provider health check failed" | |
| ); | |
| } | |
| } | |
| result.map_err(|e| anyhow::anyhow!( | |
| "Provider health check failed: {}. Please check your configuration.", | |
| e | |
| )) |
Description
@glihm Hi,
PR solves the problem of providing more user-friendly prompts when Sozo is not healthy, such as when it is not started, making it easier for developers and users.
I have noticed that Sozo has already interacted with the provider, and the current handling method is to capture any abnormal information during the first call.
If you have time, please review it.
Related issue
close #2739
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
providerto expand the module interface.Bug Fixes
Documentation
Migratecommand for better readability.Chores
dojo-utilsin the project configuration.