-
Notifications
You must be signed in to change notification settings - Fork 172
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
katana: include starknet
api as default
#2510
Conversation
WalkthroughOhayo, sensei! The changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant RpcConfig
RpcConfig->>+Default: Initialize
Default->>RpcConfig: Set apis = HashSet::from([ApiKind::Starknet])
Default->>RpcConfig: Set allowed_origins = None
RpcConfig-->>-Default: Configuration Complete
🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/node/src/config/rpc.rs (1)
44-44
: Ohayo again, sensei! Consider these enhancementsWhile the change is solid, here are a couple of suggestions to further improve the code:
Add a comment explaining why Starknet is included by default. This would provide context for future maintainers.
Consider creating a constant or a function to provide the default APIs. This could be beneficial if more APIs are added in the future or if the default set needs to be used elsewhere in the codebase.
Here's an example of how you might implement these suggestions:
/// The default set of APIs enabled for the RPC server. /// Starknet is included by default as it's the primary API for most users. pub const DEFAULT_RPC_APIS: HashSet<ApiKind> = HashSet::from([ApiKind::Starknet]); impl Default for RpcConfig { fn default() -> Self { Self { // ... other fields ... apis: DEFAULT_RPC_APIS.clone(), } } }What do you think, sensei? Would these changes be valuable additions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/katana/node/src/config/rpc.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
crates/katana/node/src/config/rpc.rs (1)
40-44
: Ohayo, sensei! LGTM: Default inclusion of Starknet APIThe change to include
ApiKind::Starknet
in the defaultapis
set is a sensible improvement. It aligns well with the PR objective and provides a more convenient out-of-the-box experience for users, as the Starknet API is likely to be the most commonly used.This modification ensures that the Starknet API is available by default without requiring additional configuration, which should streamline the setup process for many users.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2510 +/- ##
=======================================
Coverage 67.86% 67.86%
=======================================
Files 389 389
Lines 50271 50271
=======================================
Hits 34117 34117
Misses 16154 16154 ☔ View full report in Codecov by Sentry. |
ref #2508
Summary by CodeRabbit
New Features
Starknet
API.Bug Fixes
apis
field is now correctly initialized, enhancing the overall functionality of the RPC configuration.