feat(katana): add builder pattern for katana config #2653
Conversation
WalkthroughOhayo, sensei! The pull request introduces a Changes
Assessment against linked issues
Ohayo, sensei! The changes comprehensively address the objectives outlined in issue #2648, providing a robust and flexible configuration builder for the Katana node. 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: 1
🧹 Outside diff range and nitpick comments (4)
crates/dojo/test-utils/src/sequencer.rs (1)
8-9: Consider being more specific with imports.Ohayo! While the ApiKind import is nicely specific, the wildcard import
katana_node::config::*might expose more symbols than needed. Consider explicitly importing justConfigBuilderand other required types.-pub use katana_node::config::*; +use katana_node::config::{Config, ConfigBuilder};bin/katana/src/cli/node.rs (1)
285-295: Excellent implementation of the builder pattern, sensei!The implementation follows best practices by:
- Breaking down configuration into logical components
- Making the configuration process more intuitive
- Maintaining a clear separation of concerns
This change significantly improves maintainability and makes the configuration process more flexible for users.
Consider adding validation methods in the builder to ensure configuration consistency before the final build step. This could prevent invalid combinations of settings early in the configuration process.
crates/katana/node/src/config/mod.rs (2)
216-245: Ohayo, sensei! Avoid redundant assignments and cloning inmessaging_*methodsIn the
messaging_*methods, the fields are being assigned twice, and cloning of parameters is unnecessary. You can simplify the code by initializing withDefault::default()and setting the fields afterward, avoiding redundant assignments and cloning.For example, update the
messaging_chainmethod:pub fn messaging_chain(mut self, chain: String) -> Self { self.config - .messaging - .get_or_insert(MessagingConfig { chain, ..Default::default() }) - .chain = chain.clone(); + .messaging + .get_or_insert_with(Default::default) + .chain = chain; self }Apply similar changes to other
messaging_*methods.
163-169: Ohayo, sensei! Simplifyfork_blockmethod initialization withDefault::default()In the
fork_blockmethod, initializingForkingConfigwith an empty URL usingUrl::from_str("").unwrap()is less ideal. Consider usingDefault::default()for cleaner initialization.Apply this diff:
pub fn fork_block(mut self, block: Option<BlockHashOrNumber>) -> Self { self.config .forking - .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) + .get_or_insert_with(Default::default) .block = block; self }This improves code clarity and avoids potential issues with invalid URLs.
📜 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 (6)
bin/katana/src/cli/node.rs(5 hunks)crates/dojo/test-utils/src/sequencer.rs(2 hunks)crates/katana/core/Cargo.toml(0 hunks)crates/katana/core/src/constants.rs(1 hunks)crates/katana/node/src/config/execution.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- crates/katana/node/src/config/execution.rs
🔇 Additional comments (7)
crates/katana/core/src/constants.rs (2)
12-12: Ohayo! Nice optimization using const instead of lazy_static!
This change improves performance by moving the initialization to compile-time and reduces runtime overhead. The code is also more maintainable without the lazy_static machinery.
12-12: Verify ContractAddress const-compatibility
Let's verify that ContractAddress is suitable for const initialization and that all its components support compile-time construction.
crates/dojo/test-utils/src/sequencer.rs (2)
114-120: Implementation aligns well with PR objectives.
Ohayo sensei! The builder pattern implementation effectively addresses the goals from issue #2648:
- ✅ Simplifies configuration setup
- ✅ Allows individual value configuration
- ✅ Clean and maintainable code structure
The method chaining approach is particularly elegant and readable.
114-120:
Function signature inconsistency with behavior.
Ohayo sensei! The implementation looks clean, but there's a discrepancy between the function name/signature and its behavior:
- The function name
get_default_test_configsuggests it returns aConfig, but it returns a built configuration - The AI summary indicates this should return
Config, notConfigBuilder
Consider this fix:
-pub fn get_default_test_config(sequencing: SequencingConfig) -> Config {
- ConfigBuilder::new()
- .dev_fee(false)
- .chain_id(ChainId::SEPOLIA)
- .genesis_sequencer_address(DEFAULT_SEQUENCER_ADDRESS)
- .rpc_apis(HashSet::from([ApiKind::Starknet, ApiKind::Dev, ApiKind::Saya, ApiKind::Torii]))
- .sequencing(sequencing)
- .build()
+pub fn get_default_test_config(sequencing: SequencingConfig) -> Config {
+ ConfigBuilder::new()
+ .dev_fee(false)
+ .chain_id(ChainId::SEPOLIA)
+ .genesis_sequencer_address(DEFAULT_SEQUENCER_ADDRESS)
+ .rpc_apis(HashSet::from([ApiKind::Starknet, ApiKind::Dev, ApiKind::Saya, ApiKind::Torii]))
+ .sequencing(sequencing)
+ .build()
+ .expect("Failed to build default test config")
}Let's verify the usage of this function across the codebase:
bin/katana/src/cli/node.rs (3)
34-34: Ohayo sensei! Import change looks good!
The import change aligns perfectly with the introduction of the builder pattern.
328-328: Clean and direct constant usage, sensei!
The direct usage of DEFAULT_SEQUENCER_ADDRESS simplifies the code and improves readability.
536-536: Test assertions properly updated, sensei!
The test cases have been correctly updated to verify the new configuration behavior with DEFAULT_SEQUENCER_ADDRESS.
Also applies to: 562-562
crates/katana/node/src/config/mod.rs
Outdated
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { sender_address, ..Default::default() }) | ||
| .sender_address = sender_address.clone(); | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_private_key(mut self, private_key: String) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { private_key, ..Default::default() }) | ||
| .private_key = private_key.clone(); | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_interval(mut self, interval: u64) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { interval, ..Default::default() }) | ||
| .interval = interval; | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging_from_block(mut self, from_block: u64) -> Self { | ||
| self.config | ||
| .messaging | ||
| .get_or_insert(MessagingConfig { from_block, ..Default::default() }) | ||
| .from_block = from_block; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing_block_time(mut self, block_time: Option<u64>) -> Self { | ||
| self.config.sequencing.block_time = block_time; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing_no_mining(mut self, no_mining: bool) -> Self { | ||
| self.config.sequencing.no_mining = no_mining; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_fee(mut self, fee: bool) -> Self { | ||
| self.config.dev.fee = fee; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_account_validation(mut self, validation: bool) -> Self { | ||
| self.config.dev.account_validation = validation; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev_fixed_gas_prices(mut self, gas_prices: Option<FixedL1GasPriceConfig>) -> Self { | ||
| self.config.dev.fixed_gas_prices = gas_prices; | ||
| self | ||
| } | ||
|
|
||
| pub fn chain(mut self, chain: ChainSpec) -> Self { | ||
| self.config.chain = chain; | ||
| self | ||
| } | ||
|
|
||
| pub fn db(mut self, db: DbConfig) -> Self { | ||
| self.config.db = db; | ||
| self | ||
| } | ||
|
|
||
| pub fn rpc(mut self, rpc: RpcConfig) -> Self { | ||
| self.config.rpc = rpc; | ||
| self | ||
| } | ||
|
|
||
| pub fn metrics(mut self, metrics: Option<MetricsConfig>) -> Self { | ||
| self.config.metrics = metrics; | ||
| self | ||
| } | ||
|
|
||
| pub fn execution(mut self, execution: ExecutionConfig) -> Self { | ||
| self.config.execution = execution; | ||
| self | ||
| } | ||
|
|
||
| pub fn messaging(mut self, messaging: Option<MessagingConfig>) -> Self { | ||
| self.config.messaging = messaging; | ||
| self | ||
| } | ||
|
|
||
| pub fn sequencing(mut self, sequencing: SequencingConfig) -> Self { | ||
| self.config.sequencing = sequencing; | ||
| self | ||
| } | ||
|
|
||
| pub fn dev(mut self, dev: DevConfig) -> Self { | ||
| self.config.dev = dev; | ||
| self | ||
| } | ||
|
|
||
| pub fn build(self) -> Config { | ||
| self.config | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider changing method signatures to use &mut self and return &mut Self for idiomatic builder pattern
In Rust, it's idiomatic for builder pattern methods to take &mut self and return &mut Self. This allows for method chaining without consuming self at each step, improving efficiency by avoiding unnecessary copies.
Apply this diff to update the method signatures:
- pub fn chain_id(mut self, chain_id: ChainId) -> Self {
+ pub fn chain_id(&mut self, chain_id: ChainId) -> &mut Self {
self.config.chain.id = chain_id;
self
}
- pub fn genesis_parent_hash(mut self, parent_hash: BlockHash) -> Self {
+ pub fn genesis_parent_hash(&mut self, parent_hash: BlockHash) -> &mut Self {
self.config.chain.genesis.parent_hash = parent_hash;
self
}
// ... Apply similar changes to all other builder methodsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/katana/node/src/config/mod.rs (3)
75-78: Add documentation for ConfigBuilder struct.Ohayo, sensei! Consider adding documentation comments to explain the purpose and usage of the ConfigBuilder struct, similar to how the Config struct is documented.
Add this documentation:
+/// Builder for creating node configurations. +/// +/// Provides a fluent interface for constructing a `Config` instance, +/// allowing users to set only the values they want to customize +/// while keeping defaults for others. #[derive(Default, Debug)] pub struct ConfigBuilder { config: Config, }
216-270: Optimize messaging methods by removing unnecessary clones.The messaging methods contain unnecessary clone() calls since the values are already being moved into the config.
Here's an example fix for the messaging_chain method (apply similar changes to other messaging methods):
pub fn messaging_chain(&mut self, chain: String) -> &mut Self { self.config .messaging - .get_or_insert(MessagingConfig { chain, ..Default::default() }) - .chain = chain.clone(); + .get_or_insert(MessagingConfig { chain: chain.clone(), ..Default::default() }) + .chain = chain; self }
337-339: Consider adding validation in build method.The current implementation doesn't validate if required configuration fields are set before building the final config.
Consider adding a validate() method that checks for required fields and valid combinations of settings. For example:
- Ensure chain ID is set
- Validate that fork block is not set without fork URL
- Verify that messaging configuration has all required fields when enabled
bin/katana/src/cli/node.rs (1)
329-329: Consider adding a comment explaining the sequencer address assignment.While the code is correct, it would be helpful to add a brief comment explaining why we're setting the default sequencer address here, especially since this is part of the genesis configuration.
+ // Set the default sequencer address if no custom genesis is provided chain_spec.genesis.sequencer_address = DEFAULT_SEQUENCER_ADDRESS;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/katana/src/cli/node.rs(5 hunks)crates/katana/node/src/config/mod.rs(2 hunks)
🔇 Additional comments (3)
bin/katana/src/cli/node.rs (3)
34-34: LGTM! Clean import update.
The import change from Config to ConfigBuilder aligns with the PR objective of implementing the builder pattern.
285-296: Excellent implementation of the builder pattern, sensei!
The implementation follows the builder pattern best practices:
- Clear method chaining
- Logical grouping of configuration components
- Final
build()call to construct the config
The code is more readable and maintainable compared to direct struct initialization.
537-537: LGTM! Test assertions properly updated.
The test assertions have been correctly updated to verify the sequencer address configuration in both default and custom scenarios.
Also applies to: 563-563
| pub fn build(&mut self) -> Config { | ||
| self.config.clone() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize build method to avoid unnecessary cloning.
The build method clones the entire config unnecessarily. Since this is the final step in the builder pattern, we can take ownership of the config.
Apply this change:
-pub fn build(&mut self) -> Config {
- self.config.clone()
+pub fn build(self) -> Config {
+ self.config
}📝 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.
| pub fn build(&mut self) -> Config { | |
| self.config.clone() | |
| } | |
| pub fn build(self) -> Config { | |
| self.config | |
| } |
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | ||
| self.config | ||
| .forking | ||
| .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) | ||
| .block = block; | ||
| self | ||
| } |
There was a problem hiding this comment.
Improve fork_block implementation.
The empty URL string used as default in fork_block is not a meaningful default and could cause issues.
Consider requiring the URL to be set before the block can be set:
-pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self {
- self.config
- .forking
- .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None })
- .block = block;
- self
+pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self {
+ if let Some(forking) = &mut self.config.forking {
+ forking.block = block;
+ } else {
+ panic!("Fork URL must be set before setting fork block");
+ }
+ self
}📝 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.
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | |
| self.config | |
| .forking | |
| .get_or_insert(ForkingConfig { url: Url::from_str("").unwrap(), block: None }) | |
| .block = block; | |
| self | |
| } | |
| pub fn fork_block(&mut self, block: Option<BlockHashOrNumber>) -> &mut Self { | |
| if let Some(forking) = &mut self.config.forking { | |
| forking.block = block; | |
| } else { | |
| panic!("Fork URL must be set before setting fork block"); | |
| } | |
| self | |
| } |
There was a problem hiding this comment.
Thank you @edisontim for the work here!
@kariy that's a lot of functions and things to adjust if one of the config type changes, but the code simplification is actually pretty great, wdyt?
I feel like having a builder for each config may be a better approach, keeping the high level function @edisontim did?
let rpc = RpcConfigBuilder::new()
.port(0)
.addr(DEFAULT_RPC)
.max_connections(DEFAULT_MAXCONN);
let config = ConfigBuilder::new()
.rpc(rpc.build())
.dev(DevConfigBuild::new().fee(false))
.build();
|
@edisontim you would have to rebase due to latest changes. 👍 |
glihm
changed the title
I agree with the second point, I think it's a lot of clutter in one file. However for the first point it's the same if you split it in different configs no? The advantage of keeping it as one struct though as @kariy raised is that you only need one import for your config, in the case you split it you'll need to import multiple builders Not sure abt this one |
I see, let's wait @kariy input here to be sure we're all aligned then. :) |
|
@edisontim this may require a little rebase if you have a chance before @kariy review. 🫡 |
|
@edisontim if you have a chance to rebase, would be perfect man. 🙏 @kariy in a chance you have, happy to have your inputs on the proposed changes. 🫡 |
I've merged main into my branch, the chain parameter of the config has changed to be an |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/katana/node/src/config/mod.rs (4)
10-14: Ohayo sensei! Good to see these imports for building your new config structures.Please verify if all imported items are necessary; if any are unused, consider removing them to keep the module tidy.
101-107: Ohayo sensei!fork_blockmethod is also well structured.If there's any scenario where the
urlis still empty, consider adding validations. Otherwise, looks good.
124-127: Ohayo sensei!rpc_apismethod sets the modules list in one shot.If advanced module filtering is ever needed, consider adding a method to selectively enable or disable modules. For now, this is fine.
217-223: Ohayo sensei!messaging_from_block.No immediate suggestions, but you could confirm that
from_blockis valid (not negative or the like) if relevant.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/dojo/test-utils/src/sequencer.rs(2 hunks)crates/katana/cli/src/args.rs(3 hunks)crates/katana/core/Cargo.toml(0 hunks)crates/katana/core/src/constants.rs(1 hunks)crates/katana/node/src/config/metrics.rs(1 hunks)crates/katana/node/src/config/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/core/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/core/src/constants.rs
🔇 Additional comments (50)
crates/katana/node/src/config/metrics.rs (1)
24-28: Ohayo sensei! Great addition of theDefaultimplementation forMetricsConfig.This ensures that code depending on default values can safely construct a
MetricsConfigwithout specifying anything. Looks clean and concise—no changes needed here.crates/katana/cli/src/args.rs (3)
245-245: Ohayo sensei! Good approach wrappingDEFAULT_SEQUENCER_ADDRESSin aContractAddress.This move helps maintain type safety. Double-check that other references to the default sequencer address align with this updated type usage.
426-426: Ohayo sensei! Nice test coverage.Ensuring the chain spec shares the correct
sequencer_addresshelps avoid confusion. Keep up the thorough testing approach!
453-453: Ohayo sensei! Consistent validation ofsequencer_address.Having another confirmation test is a great way to catch accidental regressions. This is a well-structured test—great work!
crates/dojo/test-utils/src/sequencer.rs (1)
118-128: Ohayo sensei! Concise switch to a builder pattern inget_default_test_config.Switching from direct
Configconstruction to the newConfigBuilderis a neat improvement for clarity and extensibility. Just double-check that any future additions also flow neatly through these chained methods.crates/katana/node/src/config/mod.rs (45)
16-16: Ohayo sensei! Helpful import for the developer config.Keep an eye on whether all fields in
FixedL1GasPriceConfigremain relevant as your builder pattern evolves.
20-21: Ohayo sensei! Great to see theBlockHashOrNumberandGasPricesused inside the builder.This approach can simplify forking and gas price configuration logic. Nicely done.
23-28: Ohayo sensei! Imports for class and genesis data look consistent.If you plan on expanding genesis definitions, ensure they remain consistent with the chain spec and builder usage.
30-31: Ohayo sensei! IncludingRpcConfigandRpcModulesListis essential.Just keep in mind that future modules might need additional logic in the builder if they require specialized defaults.
80-80: Ohayo sensei! Empty line.No functional impact. Looks fine.
81-85: Ohayo sensei! DefiningConfigBuilderis a strong step for the builder pattern.Marking it
#[derive(Default, Debug)]plus storing aConfiginternally is an idiomatic approach.
86-90: Ohayo sensei!new()returningSelf::default()is neat.This standard practice keeps code DRY. No further suggestions here.
91-95: Ohayo sensei!db_dirmethod sets up the DB config path nicely.Method-chaining with
&mut Selfis idiomatic for Rust builders. Perfect approach.
96-99: Ohayo sensei!fork_urlensures the forking is initialized if absent.Re-initializing the
ForkingConfigwith a default block if needed is a practical approach. Good job.
109-112: Ohayo sensei!rpc_addrmethod is straightforward.Keeping the assignment logic minimal is the right approach. No concerns.
114-117: Ohayo sensei!rpc_portpairs well with the address.This helps ensure clarity on separate settings for
addrvs.port. Looks solid.
119-122: Ohayo sensei!rpc_max_connectionsis crucial for resource-limiting.The method is clean and ensures a single place to set the property.
129-132: Ohayo sensei! Settingrpc_cors_originshere is straightforward.Be mindful of security implications if you do any wildcard origins.
134-137: Ohayo sensei!rpc_max_event_page_sizehelps prevent large queries from exhausting resources.Perfect alignment with recommended best practices for pagination control.
139-142: Ohayo sensei! Similarly,rpc_max_proof_keysensures proof retrieval has an upper bound.This keeps the node resilient under heavy request loads.
144-147: Ohayo sensei!metrics_addruses a get-or-insert approach.Neatly handles the
Option<MetricsConfig>scenario. Great consistency.
149-152: Ohayo sensei!metrics_portmirrors the previous pattern.This consistent approach is good for maintainability.
154-157: Ohayo sensei!execution_invocation_max_steps—straightforward.No additional logic is needed unless you plan to validate step ranges.
159-162: Ohayo sensei!execution_validation_max_steps—same feedback as above.Simple and direct method. Good.
164-167: Ohayo sensei!execution_max_recursion_depthhandles deeper call sequences.If you eventually need a limit, enforce it in the method or the caller. Presently, it seems fine.
169-175: Ohayo sensei!messaging_chainshows the same pattern.Nullable or optional fields always need a keep-or-insert approach, as done here.
177-183: Ohayo sensei!messaging_rpc_url—consistent approach with.get_or_insert().Staying uniform across all messaging methods is a good practice.
185-191: Ohayo sensei!messaging_contract_address.Looking good. Consider a type-safe approach if you can parse it as Address or something domain-specific, but this is fine if the usage is purely string-based.
193-199: Ohayo sensei!messaging_sender_address.Same logic as above. Looks consistent with how the rest of the messaging fields are handled.
201-207: Ohayo sensei!messaging_private_key.Map out if there's a standard format or parsing logic for this key. If it’s purely string-based, that’s acceptable.
209-215: Ohayo sensei!messaging_interval.Simple numeric assignment. Good. Consider a sensible default if none is provided.
225-228: Ohayo sensei!sequencing_block_time.Great direct assignment. The optional usage is consistent with the rest of the config.
230-233: Ohayo sensei!sequencing_no_mining.Clear and to the point. Perfect for toggling this behavior.
235-238: Ohayo sensei!dev_fee.Toggle a boolean in a single call—makes usage super straightforward. Great job.
240-243: Ohayo sensei!dev_account_validation.Another simple boolean toggle. Good for controlling dev mode features.
245-248: Ohayo sensei!dev_fixed_gas_prices.Flexible approach letting you optionally specify these prices. Good clarity for advanced dev scenarios.
250-260: Ohayo sensei!dev_fixed_l1_gas_price_config_gas_pricesets nested fields nicely.Ensures setting partial or all fields is easy. This is well done.
262-275: Ohayo sensei!dev_fixed_l1_gas_price_config_data_gas_priceis also consistent.Keeping the pattern uniform for data gas prices. Great for clarity.
277-280: Ohayo sensei!l1_provider_urlis just a single setter.If you expand usage of the L1 provider, you might add more validation or checks. For now, good to go.
282-285: Ohayo sensei!.chain()method accepts aChainSpecand wraps it in anArc.Neat approach preventing unnecessary clones.
287-290: Ohayo sensei!.db()method is straightforward.Clean layering of a DB-specific struct. Approving this approach.
292-295: Ohayo sensei!.forking()method.A simple setter for the entire optional fork config—makes sense.
297-300: Ohayo sensei!.rpc()method sets the entireRpcConfig.Remember, you have more granular setters too, so watch for potential overlap. No biggie.
302-305: Ohayo sensei!.metrics()method for the entireOption<MetricsConfig>.Consistent with your approach for forking. Good job.
307-310: Ohayo sensei!.execution()method.Allows direct override of execution config. This stays flexible.
312-315: Ohayo sensei!.messaging()method.Aligns well with the other optional config sets.
317-320: Ohayo sensei!.sequencing()method.Completes the coverage for each top-level config struct.
322-325: Ohayo sensei!.dev()method.Enables quickly switching out the dev config. Looks good.
327-329: Ohayo sensei! Usingself.config.clone()inbuild()reintroduces overhead.This matches the earlier suggestion from the previous review about returning the owned value. Tagging as a duplicate:
Applying this diff can remove the unnecessary clone:
-pub fn build(&mut self) -> Config { - self.config.clone() +pub fn build(self) -> Config { + self.config }
330-330: Ohayo sensei! End of the builder struct—no further changes here.Everything else looks consistent. Nicely done overall!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/test-utils/src/sequencer.rs (1)
120-127: Ohayo! Consider extracting magic numbers into constants, sensei.While the builder pattern implementation is clean and readable, the magic numbers (100) for
rpc_max_event_page_sizeandrpc_max_proof_keysshould be extracted into named constants to improve maintainability and documentation.+ const DEFAULT_TEST_MAX_PAGE_SIZE: u32 = 100; + const DEFAULT_TEST_MAX_PROOF_KEYS: u32 = 100; ConfigBuilder::new() .dev_fee(false) .chain(chain_spec) .rpc_apis(RpcModulesList::all()) - .rpc_max_event_page_size(Some(100)) - .rpc_max_proof_keys(Some(100)) + .rpc_max_event_page_size(Some(DEFAULT_TEST_MAX_PAGE_SIZE)) + .rpc_max_proof_keys(Some(DEFAULT_TEST_MAX_PROOF_KEYS)) .sequencing(sequencing) .build()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/dojo/test-utils/src/sequencer.rs(1 hunks)crates/katana/cli/src/args.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/cli/src/args.rs
🔇 Additional comments (2)
crates/dojo/test-utils/src/sequencer.rs (2)
117-119: Ohayo! Chain spec initialization looks good, sensei!The ChainSpec initialization with SEPOLIA chain ID and proper ContractAddress wrapping for the sequencer_address follows best practices.
117-127: Verify test coverage and impact on existing tests.The transition to ConfigBuilder pattern looks good but we should verify its impact on existing tests.
Let's check for test files that might be affected:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the test coverage and usage patterns of both Config and ConfigBuilder in test files. This will help us assess the impact of the transition to the builder pattern.
Let's examine the actual usage patterns of Config in test files to understand if they're using direct construction or if they're already aligned with the builder pattern. This will help us determine if there might be any compatibility issues.
Ohayo sensei! Let's dig deeper to see how the Config is being used in test files and if there are any direct instantiations that might be affected by the builder pattern transition.
Ohayo sensei! After analyzing the codebase, I can now provide a conclusive response about the ConfigBuilder pattern transition.
The transition to ConfigBuilder pattern is safe and doesn't impact existing tests
The search results show that:
- No test files are directly instantiating the old Config pattern
- The
get_default_test_configfunction is not found in the codebase- The Config usages found in test files are unrelated to the configuration being modified (they are different types like
RunProfilerConfig,TestRunConfig,ProjectConfig, etc.)The changes in
sequencer.rsare self-contained and the builder pattern implementation provides a cleaner, more maintainable way to handle configuration without breaking existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that use get_default_test_config rg -l "get_default_test_config" --type rust --glob "**/*test*.rs"Length of output: 65
Script:
#!/bin/bash # Let's try a broader search for test files using Config rg -l "Config" --type rust --glob "**/*test*.rs" # Also search for direct usage of ConfigBuilder in tests rg -l "ConfigBuilder" --type rust --glob "**/*test*.rs" # Let's also check the actual implementation of get_default_test_config ast-grep --pattern 'fn get_default_test_config'Length of output: 432
Script:
#!/bin/bash # Let's check how Config is used in these test files rg "Config" -A 5 -B 5 --type rust --glob "**/*test*.rs" # Also check if there are any test utilities that might be using the old configuration pattern rg "Config" -A 5 -B 5 --type rust --glob "**/test_utils.rs"Length of output: 35645
Script:
#!/bin/bash # Let's check for any test files that might be using the Config directly rg "Config::new|Config \{" -A 5 -B 5 --type rust --glob "**/*test*.rs" # Also check for any test files that might be using the ConfigBuilder rg "ConfigBuilder::" -A 5 -B 5 --type rust --glob "**/*test*.rs" # Let's also check the actual implementation of get_default_test_config to see its usage ast-grep --pattern 'fn get_default_test_config'Length of output: 5156
Description
Adds a
ConfigBuilderbuilder pattern to theConfigstruct of KatanaRelated issue
Closes #2648
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
Release Notes:
New Features
ConfigBuilderto enhance configuration flexibility and readability.MetricsConfigadded for easier initialization.Improvements
Refactoring
lazy_staticdependency.