chore(katana): de-featurize base messaging #2506
Conversation
WalkthroughOhayo, sensei! The pull request introduces significant changes across several 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/core/Cargo.toml (2)
36-36: Ohayo, sensei! LGTM with a small consideration.The removal of
optional = truefor the alloy-related dependencies is in line with the de-featurization objective. This ensures that all necessary components for base messaging are always available.However, it's worth noting that making these dependencies required might increase the package size and compilation time. If this becomes a concern, consider exploring strategies to optimize build times or package size in the future.
Also applies to: 38-42
Line range hint
52-52: Ohayo, sensei! LGTM: Messaging feature removed as planned.The removal of the
messagingfeature is exactly what we expected to see for this de-featurization PR. This change integrates the base messaging components into the core package, simplifying the codebase structure.Don't forget to update any relevant documentation that might reference the now-removed
messagingfeature to avoid confusion for other developers, sensei!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- bin/katana/Cargo.toml (1 hunks)
- bin/katana/src/cli/node.rs (0 hunks)
- crates/katana/core/Cargo.toml (1 hunks)
- crates/katana/core/src/sequencer.rs (0 hunks)
- crates/katana/core/src/service/mod.rs (0 hunks)
- crates/katana/node/Cargo.toml (1 hunks)
- crates/katana/rpc/rpc/Cargo.toml (1 hunks)
💤 Files with no reviewable changes (3)
- bin/katana/src/cli/node.rs
- crates/katana/core/src/sequencer.rs
- crates/katana/core/src/service/mod.rs
🧰 Additional context used
🔇 Additional comments (5)
crates/katana/node/Cargo.toml (1)
33-33: Ohayo, sensei! LGTM: Feature dependency removed as intended.The removal of the "messaging" feature from the "starknet-messaging" feature dependencies aligns well with the PR objective of de-featurizing base messaging. This change suggests that the messaging functionality is now part of the core package rather than an optional feature.
To ensure this change doesn't cause unintended effects, please run the following script to check for any remaining references to the "messaging" feature:
bin/katana/Cargo.toml (1)
38-38: Ohayo, sensei! The messaging feature has been successfully de-featurized.The changes in the
featuressection align well with the PR objectives:
- The
messagingfeature has been removed from the default features list.- The
messagingfeature itself has been completely removed.- A new
starknet-messagingfeature has been introduced, which depends onkatana-node/starknet-messaging.These modifications effectively separate the core messaging functionality from feature-specific implementations, streamlining the codebase as intended.
However, to ensure consistency across the project, please verify:
- That all code depending on the previous
messagingfeature has been updated accordingly.- That the new
starknet-messagingfeature is properly implemented and documented where necessary.Let's run a quick check to ensure no lingering references to the old
messagingfeature remain:Also applies to: 41-43
✅ Verification successful
Ohayo, sensei! All references to the old
messagingfeature have been successfully removed.No lingering
messagingreferences were found in the Rust files, and the Cargo.toml files properly reflect the newstarknet-messagingfeature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old messaging feature # Test: Search for "messaging" feature in Cargo.toml files echo "Searching for 'messaging' feature in Cargo.toml files:" rg --type toml 'messaging.*=.*\[' -g 'Cargo.toml' # Test: Search for "messaging" feature in Rust files echo "Searching for #[cfg(feature = \"messaging\")] in Rust files:" rg '#\[cfg\(feature\s*=\s*"messaging"\)\]' -g '*.rs'Length of output: 580
crates/katana/core/Cargo.toml (1)
18-18: Ohayo, sensei! LGTM: Dependencies are now required.The removal of
optional = trueforasync-trait,reqwest,serde, andserde_jsonaligns well with the objective of integrating base messaging components into the core package. This change ensures these essential dependencies are always available.Also applies to: 26-28
crates/katana/rpc/rpc/Cargo.toml (2)
44-44: Ohayo again, sensei! The rstest dependency is looking sharp.The modification to the rstest dependency line seems to be a simple reformatting. It's good to keep our Cargo.toml files consistent and tidy.
39-39: Ohayo, sensei! The katana-node dependency looks good now.The removal of the "messaging" feature from the katana-node dependency aligns well with our mission to de-featurize base messaging. Nice work!
Let's make sure this change is consistent across our dojo. Run this script to check:
✅ Verification successful
Ohayo, sensei! It seems the previous check didn't yield any results. Let's double-check to ensure the "messaging" feature has been fully removed.
Ohayo, sensei! Let's ensure that the "messaging" feature has been thoroughly removed from the
katana-nodedependency.
Ohayo, sensei! It looks like the previous check didn't find the
Cargo.tomlfile. Let's broaden our search to ensure the "messaging" feature is completely removed from allCargo.tomlfiles in the repository.
Ohayo, sensei! 🎉 The "messaging" feature has been successfully removed from the
katana-nodedependency across all Cargo.toml files. Everything looks great!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of "messaging" feature from katana-node dependencies # Test: Search for any remaining "messaging" features in katana-node dependencies rg --type toml 'katana-node.*features.*messaging' cratesLength of output: 58
Script:
#!/bin/bash # Description: Confirm the complete removal of the "messaging" feature from katana-node dependencies # Verify by checking the entire Cargo.toml for any mentions of "messaging" in features ast-grep --lang toml --pattern $'katana-node { $$$ }' crates | grep 'messaging'Length of output: 187
Script:
#!/bin/bash # Description: Verify the complete removal of the "messaging" feature from katana-node dependencies # Search for "messaging" feature specifically within katana-node in Cargo.toml files rg -A2 '\[dependencies\.katana-node\]' crates/Cargo.toml | rg 'features\s*=\s*\[.*"messaging".*\]'Length of output: 158
Script:
#!/bin/bash # Description: Thoroughly verify the removal of the "messaging" feature from katana-node dependencies across all Cargo.toml files # Search all Cargo.toml files for 'katana-node' dependencies with the 'messaging' feature rg '\[dependencies\.katana-node\]' -A5 --type toml | rg 'messaging'Length of output: 67
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
=======================================
Coverage 68.95% 68.96%
=======================================
Files 376 376
Lines 48690 48689 -1
=======================================
+ Hits 33576 33580 +4
+ Misses 15114 15109 -5 ☔ View full report in Codecov by Sentry. |
include the base messaging stuff as non-feature code. the
starknet-messagingis still feature-gated.Summary by CodeRabbit
New Features
TransactionMinerstruct for enhanced transaction handling.Bug Fixes
Documentation
Chores