feat(katana-runner): allow configuring chain id #2554
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new optional field 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: 0
🧹 Outside diff range and nitpick comments (4)
crates/katana/runner/tests/runner.rs (1)
27-28: Ohayo, sensei! Great improvement to thewith_asynctest!The changes to this function enhance the test coverage by verifying the default chain ID. It's a good practice to assert the expected values rather than discarding them.
One small suggestion for consistency:
Consider moving the
short_string!("KATANA")to a constant at the top of the file, similar to how "SN_SEPOLIA" is used in thecustom_chain_idtest. This would make it easier to update if needed and maintain consistency across tests.const DEFAULT_CHAIN_ID: Felt = short_string!("KATANA");Then you can use it in the assertion:
assert_eq!(id, DEFAULT_CHAIN_ID);What do you think, sensei?
crates/katana/runner/macro/src/entry.rs (1)
80-82: Ohayo, sensei! Excellent addition of thechain_idconfiguration.The new
chain_idhandling is well-implemented and follows the existing pattern for other configuration options. This change successfully fulfills the PR objective of allowing chain_id configuration.For consistency with other configuration options, consider adding a newline after the closing brace of this block. Here's a suggested change:
if let Some(value) = config.chain_id { cfg = quote_spanned! (last_stmt_start_span=> #cfg chain_id: Some(#value), ); } +This minor adjustment will maintain the consistent spacing pattern observed throughout the function.
crates/katana/runner/src/lib.rs (2)
70-71: Ohayo, sensei! Nice addition of the chain_id field!The new
chain_idfield looks good and provides flexibility for configuration. Great job usingOption<Felt>to allow for cases where a chain ID might not be specified.Consider adding a brief comment explaining the purpose and potential values of the
chain_idfield for better documentation. For example:/// The chain id to use. If None, the default chain id will be used. pub chain_id: Option<Felt>,
129-131: Ohayo once more, sensei! The chain_id setup looks great!The conditional block to set the
chain_idis well-placed and correctly implemented. It seamlessly integrates with the existing builder pattern.For consistency with other similar blocks in this method, consider using the
if letpattern:if let Some(id) = config.chain_id { builder = builder.chain_id(id); }This aligns with the style used for
block_time,messaging, anddb_dirconfigurations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/katana/runner/macro/src/config.rs (6 hunks)
- crates/katana/runner/macro/src/entry.rs (1 hunks)
- crates/katana/runner/src/lib.rs (3 hunks)
- crates/katana/runner/tests/runner.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
crates/katana/runner/tests/runner.rs (2)
2-2: Ohayo, sensei! LGTM on the new import!The
short_stringmacro import is necessary for the new test function. Good job on keeping the imports clean and relevant.
10-16: Ohayo, sensei! Excellent addition of the custom chain ID test!This new test function
custom_chain_idis well-implemented and serves the purpose of verifying the custom chain ID functionality. Here's what looks great:
- The use of
#[tokio::test(flavor = "multi_thread")]for asynchronous testing.- The
#[katana_runner::test(chain_id = short_string!("SN_SEPOLIA"))]attribute to set a custom chain ID.- Proper assertion to check if the provider's chain ID matches the expected value.
This test enhances the coverage and aligns perfectly with the PR objective of allowing configurable chain IDs.
crates/katana/runner/macro/src/entry.rs (1)
77-78: Ohayo, sensei! Nice cleanup of thelog_pathconfiguration.The removal of the trailing comma after the
log_pathconfiguration is a good stylistic improvement. It aligns with Rust's convention for the last item in a struct or enum definition. This change doesn't affect functionality but enhances code readability.crates/katana/runner/macro/src/config.rs (7)
21-21: Ohayo, sensei! Thechain_idfield looks sharp!The addition of the
chain_idfield to theConfigurationstruct is well-implemented. UsingOption<syn::Expr>provides flexibility and aligns with the existing pattern in the struct.
36-36: Ohayo again, sensei! Thechain_idinitialization is on point!The initialization of
chain_idtoNonein thenewmethod is consistent with other optional fields and ensures proper default behavior.
111-122: Ohayo once more, sensei! Theset_chain_idmethod is a true katana strike!The implementation of
set_chain_idis well-crafted:
- It follows the established pattern of other setter methods.
- It properly handles multiple set attempts with a clear error message.
- The update logic is concise and effective.
Great job maintaining consistency and robustness!
131-131: Ohayo, sensei! TheChainIdvariant adds a new move to our kata!The addition of the
ChainIdvariant to theRunnerArgenum is a necessary and well-placed change to support the newchain_idconfiguration option.
144-147: Ohayo, sensei! Thefrom_strimplementation now recognizes our new chain_id technique!The updates to the
from_strimplementation forRunnerArgare well-executed:
- The new match arm for "chain_id" is correctly added.
- The error message is properly updated to include the new option.
These changes ensure that the
chain_idoption is fully integrated into the parsing logic.
192-194: Ohayo, sensei! Thebuild_configfunction now masters the chain_id technique!The addition of the
RunnerArg::ChainIdmatch arm in thebuild_configfunction is well-implemented:
- It correctly calls the
set_chain_idmethod when theChainIdargument is encountered.- The implementation maintains consistency with the handling of other configuration options.
This change ensures that the
chain_idoption is properly processed during configuration building.
Line range hint
1-205: Ohayo, sensei! Let's wrap up this kata with a final bow!The implementation of the
chain_idconfiguration option is excellently done. All necessary components have been added or updated consistently:
- The
Configurationstruct- The
RunnerArgenum- The
from_strimplementation- The
build_configfunctionThe changes are well-integrated with the existing code, maintaining the established patterns and style. Great job on this feature addition!
crates/katana/runner/src/lib.rs (1)
87-87: Ohayo again, sensei! The default implementation looks spot on!The addition of
chain_id: Noneto theDefaultimplementation is perfect. It maintains backwards compatibility and follows the principle of least surprise. Well done!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2554 +/- ##
==========================================
+ Coverage 69.55% 69.59% +0.03%
==========================================
Files 388 388
Lines 49964 50018 +54
==========================================
+ Hits 34752 34809 +57
+ Misses 15212 15209 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
crates/katana/node-bindings/src/json.rs (1)
73-80: Ohayo, sensei! The newChainIdstruct looks sharp!The addition of the
ChainIdstruct is well-implemented and consistent with the existing code style. It's properly documented and derives the necessary traits for JSON deserialization and debugging.One small suggestion to consider:
You might want to add a
Clonederive to the struct for easier copying if needed in the future. Here's how you could modify it:-#[derive(Deserialize, Debug)] +#[derive(Deserialize, Debug, Clone)] pub struct ChainId { pub chain: String, }This change would make the struct more flexible for future use cases without affecting its current functionality.
crates/katana/primitives/src/chain.rs (1)
47-51: Ohayo, sensei! LGTM: Added Display trait implementation.The new
Displaytrait implementation forNamedChainIdlooks great! It provides a consistent way to displayNamedChainIdinstances as strings, which is very useful for logging and debugging.A small suggestion to make it even more awesome:
Consider using the
write!macro instead off.write_str()for potential future flexibility:impl std::fmt::Display for NamedChainId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self.name()) } }This change allows for easier formatting if we need to add more complex logic in the future.
crates/katana/node/src/lib.rs (2)
99-101: Ohayo, sensei! Nice addition of chain ID logging.The new logging statement improves the observability of the node startup process. It's a good practice to log such important information.
Consider adding the node's version to this log message for even better diagnostics. For example:
info!(chain = %chain, version = env!("CARGO_PKG_VERSION"), "Starting node.");
Line range hint
102-120: Sugoi metrics implementation, sensei!The addition of the Prometheus metrics server greatly enhances the node's observability. The conditional initialization and inclusion of database metrics (when available) are well thought out.
Consider adding error logging in case the metrics server fails to start. For example:
match prometheus_exporter::serve( addr, self.prometheus_handle.clone(), metrics_process::Collector::default(), reports, ).await { Ok(_) => info!(%addr, "Metrics endpoint started."), Err(e) => error!(%addr, error = %e, "Failed to start metrics endpoint."), }This will provide more visibility into potential issues during the metrics server initialization.
📜 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 (5)
- crates/katana/node-bindings/src/json.rs (1 hunks)
- crates/katana/node-bindings/src/lib.rs (9 hunks)
- crates/katana/node/src/lib.rs (1 hunks)
- crates/katana/primitives/Cargo.toml (0 hunks)
- crates/katana/primitives/src/chain.rs (2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/primitives/Cargo.toml
🧰 Additional context used
🔇 Additional comments (7)
crates/katana/node-bindings/src/json.rs (1)
Line range hint
1-80: Excellent work integrating the newChainIdstruct, sensei!The addition of the
ChainIdstruct seamlessly integrates with the existing codebase. It follows the established patterns and conventions used throughout the file, maintaining consistency and readability.The new struct enhances the JSON parsing capabilities of the module, allowing for the parsing of chain ID information from log messages. This addition appears to be well-thought-out and implemented cleanly.
crates/katana/primitives/src/chain.rs (2)
7-7: Ohayo, sensei! LGTM: Removed derive macro for Display.The removal of
#[derive(Display)]is a good change as it paves the way for a customDisplaytrait implementation. This allows for more control over howNamedChainIdis displayed as a string.
Line range hint
138-143: Ohayo, sensei! Verify compatibility with ChainId.The changes to
NamedChainIdlook compatible with the existingChainIdimplementation. However, it's worth double-checking that theDisplayimplementation forChainIdstill works as expected with the newNamedChainIddisplay logic.Could you please run the following test to ensure everything is working correctly?
crates/katana/node/src/lib.rs (1)
Line range hint
99-120: Dojo master level changes, sensei!The additions to the
launchmethod significantly enhance the node's observability. The chain ID logging and conditional Prometheus metrics server initialization are well-implemented and will greatly aid in monitoring and debugging.A couple of minor suggestions were made to further improve the implementation:
- Consider adding the node's version to the initial log message.
- Add error logging for potential failures in metrics server initialization.
Overall, these changes are a valuable addition to the codebase. Excellent work!
crates/katana/node-bindings/src/lib.rs (3)
633-648: Functionparse_chain_id_logis well-implementedThe
parse_chain_id_logfunction correctly parses the chain ID from log entries, handling both hexadecimal and Cairo short string formats effectively.
675-678: Test validates chain ID with JSON loggingThe test starting on line 675 successfully verifies that the chain ID is correctly set and retrieved when JSON logging is enabled.
746-758: Testcan_launch_katana_with_custom_chain_idconfirms custom chain ID functionalityThe added test function correctly ensures that a custom chain ID can be set for the Katana instance and that it matches the chain ID retrieved from the provider.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/node-bindings/src/lib.rs (1)
149-149: Rephrase the doc comment for clarityThe doc comment can be improved for readability. Consider rephrasing it to:
/// The string indicator used to extract the chain ID from logs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/katana/node-bindings/src/lib.rs (9 hunks)
🧰 Additional context used
🔇 Additional comments (7)
crates/katana/node-bindings/src/lib.rs (7)
18-18: Ohayo, sensei! Import looks good.The
cairo_short_string_to_feltfunction is correctly imported and will be useful for converting short strings toFelt.
522-522: Parsing logic is correctly implementedThe addition of the parsing condition for
JsonLog<json::RpcAddr>appropriately handles JSON logging for RPC addresses.
529-539: Chain ID parsing from JSON logs is well-handledThe logic for extracting the chain ID from JSON logs effectively handles both hexadecimal and Cairo short string formats.
562-564: Properly parsing chain ID from standard logsThe added condition ensures that the chain ID is correctly parsed when JSON logging is disabled.
632-647:parse_chain_id_logfunction is implemented correctlyThe function effectively cleans ANSI escape codes and parses the chain ID, handling both hex and short string formats appropriately.
674-677: Test for chain ID with JSON logging works as expectedThe test accurately verifies that a custom chain ID is correctly set and retrieved when JSON logging is enabled.
747-757: Test for custom chain ID is successfully validating functionalityThe asynchronous test ensures that setting a custom chain ID works correctly and that it can be retrieved from the provider.
Summary by CodeRabbit
New Features
chain_id, for enhanced customization in the Katana runner.chain_idin the configuration parsing logic.Bug Fixes
log_pathin configuration generation.Tests
chain_idin the provider.