chore(katana): move syncing specific component to /sync dir
#2789
Conversation
WalkthroughOhayo, sensei! This pull request involves significant restructuring of the Changes
Possibly related PRs
🪧 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/sync/stage/src/blocks.rs (2)
Line range hint
43-59: Consider handling rate limiting more gracefullyIn the
fetch_blocks_with_retryfunction, you are retrying when encountering rate limiting errors. However, setting amin_delayof 9 seconds may not be sufficient if the rate limit window is longer.Consider implementing the following improvements:
- Use the
Retry-Afterheader from the response to determine the appropriate delay.- Implement a maximum number of retries to prevent infinite loops.
- Log the number of retries for better observability.
Line range hint
201-262: Avoid panics with safe error handling inextract_block_dataIn the
extract_block_datafunction, usingexpect("valid u128")can cause a panic if the conversion fails, which is undesirable in production code.Please consider handling these potential errors gracefully by propagating them using the
?operator and defining appropriate error variants.Apply this diff to handle conversions safely:
fn to_gas_prices(prices: ResourcePrice) -> GasPrices { GasPrices { - eth: prices.price_in_fri.to_u128().expect("valid u128"), - strk: prices.price_in_fri.to_u128().expect("valid u128"), + eth: prices.price_in_fri.to_u128().ok_or_else(|| Error::InvalidPriceConversion)?, + strk: prices.price_in_fri.to_u128().ok_or_else(|| Error::InvalidPriceConversion)?, } } // Update fee conversions similarly... // Add a new error variant #[derive(Debug, thiserror::Error)] pub enum Error { // existing variants... + #[error("Failed to convert price to u128")] + InvalidPriceConversion, }
📜 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)
Cargo.toml(3 hunks)crates/katana/node/Cargo.toml(1 hunks)crates/katana/node/src/full/mod.rs(1 hunks)crates/katana/node/src/lib.rs(1 hunks)crates/katana/sync/pipeline/Cargo.toml(1 hunks)crates/katana/sync/pipeline/src/lib.rs(3 hunks)crates/katana/sync/stage/Cargo.toml(2 hunks)crates/katana/sync/stage/src/blocks.rs(1 hunks)crates/katana/sync/stage/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/node/src/lib.rs
🔇 Additional comments (15)
crates/katana/sync/stage/src/blocks.rs (1)
278-278: Verify the correct import path for Stage and StageExecutionInput
At line 278, the import statement:
use crate::{Stage, StageExecutionInput};Ensure that the Stage trait and StageExecutionInput struct are correctly defined and accessible from this path, especially after the recent restructuring of modules.
To confirm the correct paths, please run:
✅ Verification successful
Import path is correct, sensei! ✨
The import statement is perfectly valid! Both Stage trait and StageExecutionInput struct are defined in the same crate's lib.rs file (crates/katana/sync/stage/src/lib.rs), making them accessible via the crate path from blocks.rs which is in the same package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and path of `Stage` and `StageExecutionInput`
fd -p crates/katana -e rs -x grep -Hn 'pub (struct|trait) (Stage|StageExecutionInput)' {}
Length of output: 93
Script:
#!/bin/bash
# Let's try a different approach to find the trait and struct definitions
# First, let's search for Stage trait definition
ast-grep --pattern 'trait Stage'
# Then, let's search for StageExecutionInput struct
ast-grep --pattern 'struct StageExecutionInput'
# Also search with ripgrep for more context
rg "trait Stage|struct StageExecutionInput" -A 2
Length of output: 935
Script:
#!/bin/bash
# Let's also check the mod.rs files in the sync directory for exports
fd -p "crates/katana/sync" -e rs -x grep -Hn "pub (use|mod)" {}
Length of output: 65
Script:
#!/bin/bash
# Let's check the actual file's directory structure
fd -p "crates/katana/sync/stage" -t f -e rs
Length of output: 201
crates/katana/sync/pipeline/Cargo.toml (3)
1-7: Package metadata is correctly configured
The Cargo.toml file accurately sets the package metadata with workspace configurations, ensuring consistency across the project.
19-19: Dev-dependency async-trait is correctly included
Including async-trait as a dev-dependency is appropriate for development and testing purposes.
9-17: Verify workspace dependencies
The dependencies are set to use workspace versions, which helps maintain version consistency. However, please verify that all these crates are included in the workspace and properly configured.
Run the following script to confirm the presence of dependencies in the workspace:
✅ Verification successful
The previous script had an issue with the search path. Let's check the workspace configuration and actual crate locations to verify the dependencies properly.
All workspace dependencies are properly configured, sensei!
After checking the workspace Cargo.toml, I can confirm that all the dependencies are correctly configured:
katana-primitivesis atcrates/katana/primitiveskatana-provideris atcrates/katana/storage/providerkatana-stageis atcrates/katana/sync/stagefutures,thiserror,tokio, andtracingare all properly defined in the[workspace.dependencies]section
The workspace versioning is working as expected, ohayo! 🎋
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that all workspace dependencies exist
dependencies=("katana-primitives" "katana-provider" "katana-stage" "futures" "thiserror" "tokio" "tracing")
for dep in "${dependencies[@]}"; do
if ! fd -p crates/katana -t d "$dep"; then
echo "Dependency $dep not found in workspace"
fi
done
Length of output: 2000
Script:
#!/bin/bash
# Let's check the workspace root's Cargo.toml for workspace members and dependencies
cat Cargo.toml
# Also check if these crates exist in different locations
fd -t d "katana-primitives|katana-provider|katana-stage"
Length of output: 9435
crates/katana/sync/stage/Cargo.toml (2)
14-14: Confirm feature flags for katana-provider
The katana-provider dependency has been simplified to use workspace = true. Ensure that any previously required features, such as "test-utils", are still enabled where necessary.
4-4: Ensure all references reflect the package name change to katana-stage
The package name has been updated to katana-stage. Verify that all other crates and modules have updated their dependencies to reflect this change.
Please run the following script to find any lingering references to the old name:
crates/katana/sync/stage/src/lib.rs (3)
1-1: Unused crate dependencies warning is properly configured
The attribute #![cfg_attr(not(test), warn(unused_crate_dependencies))] will help keep the crate lean by warning about unused dependencies.
Line range hint 25-41: Comprehensive error handling in Error enum
The Error enum now includes variants for different stages and any other errors, improving error granularity.
Line range hint 43-51: Stage trait is well-defined
The Stage trait provides a clear interface with an id method and an asynchronous execute method, which will facilitate the implementation of various stages.
crates/katana/node/Cargo.toml (1)
17-17: Ohayo! LGTM - Dependency addition aligns with restructuring
The addition of katana-stage as a workspace dependency is consistent with the move of syncing components to the /sync directory.
crates/katana/node/src/full/mod.rs (1)
21-21: Ohayo sensei! Import path updated correctly
The import path has been updated to use the new katana_stage module, which aligns with the restructuring effort. The usage in pipeline construction remains unchanged, maintaining functionality while improving organization.
crates/katana/sync/pipeline/src/lib.rs (2)
9-9: Ohayo! Stage-related imports and error handling updated correctly
The changes properly reflect the migration of stage-related components to the katana_stage crate:
- Import path updated to use the new module
- Error variant correctly uses the new error type from
katana_stage
Also applies to: 25-25
187-187: Test imports aligned with new structure
The test module's imports have been properly updated to use katana_stage::StageResult, maintaining consistency with the restructuring.
Cargo.toml (2)
39-40: Ohayo sensei! The workspace member additions look good! 🎋
The new workspace members align perfectly with the PR objective of moving syncing components to the /sync directory:
"crates/katana/sync/pipeline"
"crates/katana/sync/stage"
105-105: Dependencies are properly aligned with the new structure! ⚔️
The dependency paths have been correctly updated to match the new workspace structure:
katana-pipelinenow points to"crates/katana/sync/pipeline"katana-stageis properly configured with"crates/katana/sync/stage"
Let's verify that all references to these crates are consistent:
Also applies to: 115-115
✅ Verification successful
I'll check both the actual path references in Cargo.toml and verify if the directories exist in the new structure before making a final determination.
Ohayo! Dependencies are perfectly aligned and verified, sensei! ⚔️
The verification shows:
- Both directories
crates/katana/sync/pipelineandcrates/katana/sync/stageexist in the filesystem - The paths in Cargo.toml are correctly configured and consistent
- No old path references were found in any Cargo.toml files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no old paths are referenced in other Cargo.toml files
# Search for any references to old pipeline path
rg -l "katana/pipeline\"" -g "Cargo.toml"
# Search for references to new paths to ensure they exist
rg -l "katana/sync/(pipeline|stage)" -g "Cargo.toml"
Length of output: 104
Script:
#!/bin/bash
# Let's verify the actual content of the Cargo.toml to see all path references
cat Cargo.toml | grep -A 1 -B 1 "katana.*pipeline\|katana.*stage"
# Also check if the directories actually exist
fd --type d "pipeline|stage" crates/katana/sync/
Length of output: 632
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2789 +/- ##
==========================================
- Coverage 55.99% 55.99% -0.01%
==========================================
Files 435 435
Lines 55109 55109
==========================================
- Hits 30861 30856 -5
- Misses 24248 24253 +5 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Downloaderstruct for enhanced block downloading capabilities.katanaproject.Bug Fixes
Chores