refactor(katana): improve node configurations #2508
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications across multiple files in the Katana project, primarily focusing on the removal of the Changes
Possibly related issues
Possibly related PRs
Suggested labels
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: 10
🧹 Outside diff range and nitpick comments (18)
crates/katana/node/src/config/dev.rs (1)
1-5: Ohayo, sensei! TheDevConfigstructure looks sharp!The overall structure of
DevConfigis well-defined and includes appropriate derive macros. However, to enhance clarity and maintainability, consider adding documentation comments to explain the purpose of thefeeandvalidatefields.Here's a suggestion to improve the code:
#[derive(Debug, Clone, Default)] pub struct DevConfig { + /// Indicates whether fees should be applied in development mode. pub fee: bool, + /// Determines if validation should be performed in development mode. pub validate: bool, }This addition will make it easier for other developers to understand the purpose of these configuration options, sensei!
crates/katana/node/src/config/metrics.rs (1)
3-4: Excellent struct definition, sensei!The
MetricsConfigstruct is well-defined with appropriate derive macros. TheDebugtrait will be handy for logging, andCloneprovides flexibility in usage.One small suggestion to consider:
You might want to add the
PartialEqderive macro. This can be useful for comparing configurations, especially in tests. Here's how it would look:- #[derive(Debug, Clone)] + #[derive(Debug, Clone, PartialEq)] pub struct MetricsConfig {crates/katana/node/src/config/db.rs (1)
1-6: Ohayo, sensei! TheDbConfigstructure looks sharp!The new
DbConfigstruct is a clean and simple way to handle database configuration. The use ofOption<PathBuf>for thedirfield is a wise choice, allowing for flexibility in specifying the database directory.A few suggestions to make it even better:
Consider adding documentation comments to explain the purpose of the struct and its field. This will help other developers understand how to use it.
Think about whether any additional fields might be useful for database configuration, such as connection pool size or timeout settings.
Here's an example of how you could add documentation:
/// Configuration for the database used by the Katana node. #[derive(Debug, Clone, Default)] pub struct DbConfig { /// The directory where the database files will be stored. /// If None, a default directory will be used. pub dir: Option<PathBuf>, }What do you think, sensei? Would you like me to propose these changes in a separate PR?
crates/katana/primitives/src/chain_spec.rs (2)
8-15: Ohayo again, sensei! The ChainSpec struct looks solid!The
ChainSpecstruct is well-defined with appropriate public fields foridandgenesis. DerivingDebugandCloneis a good choice for this structure.Consider adding doc comments for the
idandgenesisfields to provide more context about their purpose and usage.
17-30: Ohayo once more, sensei! The Default implementation is looking sharp!The
Defaultimplementation forChainSpecprovides a solid foundation for development setups. Great job using theDevAllocationsGeneratorfor flexible account generation!Consider adding error handling for the
ChainId::parse("KATANA")call. While it's unlikely to fail with a hardcoded string, it's generally good practice to handle potential errors explicitly.- let id = ChainId::parse("KATANA").unwrap(); + let id = ChainId::parse("KATANA").expect("Failed to parse default chain ID");This change would provide a more informative error message if parsing were to fail for any reason.
crates/katana/node/src/config/rpc.rs (3)
9-18: Ohayo, sensei! The ApiKind enum is looking strong!The enum effectively represents the supported APIs, and the derived traits are well-chosen. The use of strum_macros is a nice touch for easy string conversion.
Consider adding a brief doc comment for each variant to explain what each API is used for. This would enhance the self-documentation of the code.
20-27: Ohayo, sensei! The RpcConfig struct is well-crafted!The struct effectively encapsulates all necessary RPC configuration options. The use of
OptionandHashSetfor respective fields is a wise choice.Consider adding doc comments for each field to explain their purpose and any constraints (e.g., valid ranges for
max_connections). This would make the struct more self-documenting.
29-45: Ohayo, sensei! The RpcConfig implementation is solid, but there's room for improvement!The
socket_addrmethod is a handy addition, and the use of constants in the Default implementation ensures consistency.Consider initializing the
apisHashSet with a default set of APIs in the Default implementation. This would provide a more useful out-of-the-box configuration. For example:impl Default for RpcConfig { fn default() -> Self { Self { // ... other fields ... apis: [ApiKind::Starknet, ApiKind::Dev].into_iter().collect(), } } }This ensures that at least some APIs are enabled by default, which might be more convenient for users.
crates/katana/rpc/rpc/tests/saya.rs (2)
23-26: Ohayo again, sensei! The changes look good, but let's make it even better!The update to use
get_default_test_configwithSequencingConfigis spot on. It aligns perfectly with the new configuration structure.For improved readability, consider extracting the configuration into a separate variable:
- let sequencer = TestSequencer::start(get_default_test_config(SequencingConfig { - no_mining: true, - ..Default::default() - })) - .await; + let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() + }); + let sequencer = TestSequencer::start(config).await;This separation makes the code more modular and easier to read. What do you think, sensei?
92-95: Ohayo once more, sensei! Great consistency here!The changes in this function mirror those in
fetch_traces_from_block, which is excellent for maintaining consistency across the codebase. Well done!For the sake of consistency with the previous suggestion, let's apply the same improvement here:
- let sequencer = TestSequencer::start(get_default_test_config(SequencingConfig { - no_mining: true, - ..Default::default() - })) - .await; + let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() + }); + let sequencer = TestSequencer::start(config).await;This change will keep both test functions consistent and improve overall readability. What are your thoughts on this, sensei?
crates/katana/rpc/rpc/tests/dev.rs (2)
1-2: Ohayo, sensei! LGTM with a small suggestion.The changes to the imports and
create_test_sequencerfunction look good. They align with the refactoring of the configuration system mentioned in the PR objectives.Consider adding a brief comment explaining the purpose of
SequencingConfig::default()to improve code readability:async fn create_test_sequencer() -> TestSequencer { + // Use default sequencing configuration for testing TestSequencer::start(get_default_test_config(SequencingConfig::default())).await }Also applies to: 8-8
Line range hint
108-129: Ohayo once more, sensei! Let's tidy up this commented-out test.The
test_set_storage_at_on_instant_modefunction is still commented out. To keep our codebase clean and prevent confusion:Consider either:
- Removing the commented-out test if it's no longer needed.
- Adding a TODO comment explaining why it's kept and when it might be useful.
For example:
+// TODO: Uncomment and update this test when instant mode is implemented (Issue #XXXX) // #[tokio::test] // async fn test_set_storage_at_on_instant_mode() { // ... (rest of the commented code) // }crates/katana/rpc/rpc/tests/torii.rs (2)
28-30: Ohayo, sensei! The configuration setup looks solid!The update to use
SequencingConfigand explicitly setno_miningtotruealigns well with the project's new configuration structure. This change enhances clarity about the test environment.Consider adding a brief comment explaining why
no_miningis set totruefor this test, to help future readers understand the test setup more quickly.
Line range hint
1-265: Ohayo, sensei! The stability of our tests is commendable!It's great to see that the core logic of both test functions has been preserved. This stability ensures that we're still thoroughly testing our transaction handling and retrieval functionality.
As an enhancement, consider adding a test case that specifically verifies the behavior with different
SequencingConfigsettings. This could help catch any regressions related to the recent configuration changes.crates/katana/rpc/rpc/src/starknet/trace.rs (1)
30-30: Ohayo, sensei! Nice refactoring of the config access.The changes to how the chain ID and execution flags are accessed look good. They align well with the PR objectives of refactoring Katana node config types. Here's a small suggestion:
Consider adding a comment explaining the reason for the
#[allow(deprecated)]attributes. This will help future maintainers understand why these are necessary.Also applies to: 69-69, 75-75
crates/katana/node/src/config/mod.rs (2)
14-24: Ohayo, sensei! Consider adding documentation comments to theConfigstruct and its fieldsAdding
///documentation comments to theConfigstruct and its fields will improve code readability and help other developers understand the configuration options available.
26-30: Ohayo, sensei! Consider adding documentation comments to theSequencingConfigstruct and its fieldsAdding
///documentation comments to theSequencingConfigstruct and its fields will enhance clarity about the purpose of each configuration option.crates/katana/node/src/lib.rs (1)
165-165: Ohayo sensei! Update the function documentation to reflect changesThe
buildfunction now accepts a singleConfigparameter. Please update the function's documentation to reflect this change for clarity.Apply this diff to update the documentation:
/// Build the core Katana components from the given configurations. +/// +/// This function builds the node components using the provided unified `Config` structure.
📜 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 (31)
- bin/katana/Cargo.toml (0 hunks)
- bin/katana/src/cli/node.rs (9 hunks)
- crates/dojo-test-utils/Cargo.toml (0 hunks)
- crates/dojo-test-utils/src/sequencer.rs (4 hunks)
- crates/dojo-utils/src/tx/waiter.rs (2 hunks)
- crates/katana/core/Cargo.toml (0 hunks)
- crates/katana/core/src/backend/config.rs (2 hunks)
- crates/katana/core/src/backend/mod.rs (2 hunks)
- crates/katana/core/src/lib.rs (0 hunks)
- crates/katana/core/src/sequencer.rs (0 hunks)
- crates/katana/core/src/service/messaging/service.rs (2 hunks)
- crates/katana/node/Cargo.toml (1 hunks)
- crates/katana/node/src/config/db.rs (1 hunks)
- crates/katana/node/src/config/dev.rs (1 hunks)
- crates/katana/node/src/config/metrics.rs (1 hunks)
- crates/katana/node/src/config/mod.rs (1 hunks)
- crates/katana/node/src/config/rpc.rs (1 hunks)
- crates/katana/node/src/lib.rs (10 hunks)
- crates/katana/primitives/src/chain_spec.rs (1 hunks)
- crates/katana/primitives/src/lib.rs (1 hunks)
- crates/katana/rpc/rpc-api/src/lib.rs (0 hunks)
- crates/katana/rpc/rpc/src/config.rs (0 hunks)
- crates/katana/rpc/rpc/src/dev.rs (1 hunks)
- crates/katana/rpc/rpc/src/lib.rs (0 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (4 hunks)
- crates/katana/rpc/rpc/src/starknet/trace.rs (2 hunks)
- crates/katana/rpc/rpc/src/starknet/write.rs (3 hunks)
- crates/katana/rpc/rpc/tests/dev.rs (1 hunks)
- crates/katana/rpc/rpc/tests/saya.rs (3 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (14 hunks)
- crates/katana/rpc/rpc/tests/torii.rs (3 hunks)
💤 Files with no reviewable changes (8)
- bin/katana/Cargo.toml
- crates/dojo-test-utils/Cargo.toml
- crates/katana/core/Cargo.toml
- crates/katana/core/src/lib.rs
- crates/katana/core/src/sequencer.rs
- crates/katana/rpc/rpc-api/src/lib.rs
- crates/katana/rpc/rpc/src/config.rs
- crates/katana/rpc/rpc/src/lib.rs
🧰 Additional context used
🔇 Additional comments (52)
crates/katana/node/src/config/metrics.rs (1)
1-1: Ohayo, sensei! The import looks sharp!The specific import of
SocketAddrfrom the standard library is clean and precise. It's exactly what we need for ourMetricsConfigstruct.crates/katana/primitives/src/lib.rs (1)
5-5: Ohayo, sensei! The new module looks sharp!The addition of the
chain_specmodule aligns perfectly with our mission to refactor the Katana node config types. It's placed in the correct alphabetical order, maintaining our code's zen-like harmony. Well done!crates/katana/primitives/src/chain_spec.rs (1)
1-6: Ohayo, sensei! The imports look sharp and focused!The imports are well-organized and relevant to the functionality being implemented in this file. Good job keeping it clean and efficient!
crates/katana/node/Cargo.toml (1)
32-33: Ohayo, sensei! These additions look sharp!The new dependencies
strumandstrum_macroshave been added with proper workspace version management. This is consistent with the existing dependency management style in the file.These crates are typically used for working with enums, providing functionality like string conversion, iteration, and more. It would be interesting to see how they're being utilized in the Katana node code. Are you planning to wield these new tools for enum-related tasks, sensei?
crates/katana/node/src/config/rpc.rs (1)
1-7: Ohayo, sensei! The imports and constants look sharp!The imports are well-chosen, and the constants provide sensible default values for the RPC configuration. Using
LOCALHOSTas the default address is a secure choice.crates/katana/rpc/rpc/src/dev.rs (1)
97-97: Ohayo, sensei! This change looks good to me.The modification aligns well with the PR objectives of refactoring Katana node config types. The
predeployed_accountsmethod now correctly accesses the genesis accounts fromself.backend.chain_spec.genesisinstead ofself.backend.config.genesis. This change reflects the restructuring of the backend's configuration management.crates/katana/rpc/rpc/src/starknet/write.rs (2)
44-44: Ohayo again, sensei! The chain ID access has been consistently updated here too.The change from
this.inner.backend.chain_idtothis.inner.backend.chain_spec.idin theadd_declare_transaction_implmethod is consistent with the previous update. This maintains the uniformity of the refactoring across different transaction types.
65-65: Ohayo once more, sensei! The chain ID access update is complete across all transaction types.The change to
this.inner.backend.chain_spec.idin theadd_deploy_account_transaction_implmethod completes the consistent update across all transaction handling methods. This refactoring aligns well with the newConfigstructure mentioned in the PR summary.To summarize the impact:
- All transaction handling methods now use
chain_spec.idinstead ofchain_id.- This change provides a more unified approach to accessing chain-related information.
- The core logic of transaction handling remains intact, minimizing the risk of introducing bugs.
Great job on maintaining consistency throughout these changes, sensei!
crates/katana/rpc/rpc/tests/saya.rs (1)
6-6: Ohayo, sensei! LGTM on the import changes!The updates to the imports align perfectly with the refactoring of the configuration structures. Nice work on keeping things consistent!
Let's make sure these changes are applied consistently across the codebase:
Also applies to: 9-9
✅ Verification successful
Ohayo, sensei! Imports are consistently updated across the codebase!
Everything looks good with the import changes. All usages of the old imports have been successfully replaced with the new ones across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new imports across the codebase # Test 1: Check for any remaining usage of the old import rg --type rust "get_default_test_starknet_config" # Test 2: Verify the usage of the new import rg --type rust "get_default_test_config" # Test 3: Check for any remaining usage of the old SequencerConfig rg --type rust "use.*SequencerConfig" # Test 4: Verify the usage of the new SequencingConfig rg --type rust "use.*SequencingConfig"Length of output: 3878
crates/katana/rpc/rpc/tests/dev.rs (1)
Line range hint
13-106: Ohayo again, sensei! The test logic looks solid.The core functionality of the tests remains unchanged, which is great. They continue to effectively verify the behavior of block timestamps in various scenarios. This consistency suggests that the refactoring hasn't introduced any unintended side effects in the testing logic.
crates/katana/rpc/rpc/tests/torii.rs (2)
7-7: Ohayo, sensei! The import changes look good!The updates to the imports are consistent with the project's refactoring efforts. The change from
get_default_test_starknet_configtoget_default_test_configand the update of theSequencerConfigimport path reflect the broader changes in the project structure.Also applies to: 10-10
183-184: Ohayo, sensei! We might need to double-check the test setup here.The change to use
SequencingConfig::default()instead of a custom configuration raises a question about the test's intent. Originally, this test was specifically for instant mining, but now it's using the default configuration.Could you please verify if the default
SequencingConfigincludes instant mining? If not, we might need to explicitly enable it to maintain the test's original purpose.To help verify this, you can run the following script:
This will help us confirm whether instant mining is enabled by default or if we need to adjust the test setup.
crates/katana/core/src/service/messaging/service.rs (2)
106-106: Ohayo again, sensei! This change is consistently awesome!The replacement of
backend.chain_idwithbackend.chain_spec.idin the Starknet mode mirrors the change in the Ethereum mode. This consistency across different blockchain implementations is praiseworthy and contributes to a more unified codebase.It's great to see this systematic update being applied uniformly. Keep up the excellent work, sensei!
88-88: Ohayo, sensei! This change looks good to me!The replacement of
backend.chain_idwithbackend.chain_spec.idseems to be a step towards a more structured approach to chain identification. It's a subtle yet important change that could improve code organization and readability.To ensure consistency, let's verify this change across the codebase:
✅ Verification successful
Ohayo, sensei! This change has been thoroughly verified and all instances of
backend.chain_idhave been successfully replaced withbackend.chain_spec.id. Great job ensuring consistency across the codebase!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of `backend.chain_id` and compare with `backend.chain_spec.id` usage echo "Instances of backend.chain_id:" rg --type rust 'backend\.chain_id' echo "\nInstances of backend.chain_spec.id:" rg --type rust 'backend\.chain_spec\.id'Length of output: 1311
crates/katana/rpc/rpc/src/starknet/trace.rs (1)
Line range hint
1-368: Ohayo again, sensei! Overall, the changes look solid.The refactoring of the config access in this file is well-executed and aligns perfectly with the PR objectives. The code structure and maintainability have been improved without introducing any major issues. Keep up the excellent work!
crates/dojo-utils/src/tx/waiter.rs (2)
304-304: Ohayo, sensei! LGTM: Simplified test setupThe changes to the
create_test_sequencerfunction look good. You've successfully simplified the test setup by usingget_default_test_config(Default::default())instead of the previous configuration. This aligns well with the PR objective of refactoring Katana node config types.Also applies to: 320-320
Line range hint
1-480: Ohayo again, sensei! Overall impact looks goodThe changes in this file are well-contained within the test setup. The removal of
SequencerConfigand the simplification of the test configuration don't affect the main functionality of theTransactionWaiteror any other parts of this file. The refactoring aligns with the PR objectives and improves the maintainability of the test code.crates/katana/rpc/rpc/tests/starknet.rs (15)
11-11: Ohayo, sensei! LGTM on the import changes.The updates to the import statements align well with the refactoring process. Good job on maintaining consistency!
Also applies to: 14-14
42-42: Ohayo again, sensei! The configuration update looks good.The change to use
get_default_test_config(SequencingConfig::default())is a solid move. It maintains the previous behavior while aligning with the new unified configuration structure. Nice work!
97-97: Ohayo once more, sensei! The legacy contract test looks ship-shape.The configuration update here mirrors the change in the previous function. Consistency is key, and you're nailing it!
155-157: Ohayo, sensei! Loving the flexibility in thedeploy_accounttest.The new configuration setup is a great improvement. It allows for custom
block_timeandfeesettings, making the test more versatile. Here's what's awesome:
- You're using
SequencingConfigto set up customblock_time.- The
config.dev.feesetting adds extra control over the test environment.- The
TestSequencer::startcall now uses this flexible configuration.This change will make it easier to test different scenarios. Excellent work on improving the test's adaptability!
Also applies to: 159-159
242-242: Ohayo, sensei! Theestimate_feetest is looking good.The configuration update here is in line with the changes we've seen in other functions. Consistency is key in refactoring, and you're maintaining it well. Keep up the good work!
286-287: Ohayo, sensei! Theconcurrent_transactions_submissionstest is now more flexible.Great job on updating the configuration setup here. By using
SequencingConfigwith a customblock_time, you've made the test more adaptable. This change allows for testing different scenarios with varying block times, which is crucial for thorough testing of concurrent transactions. Keep up the excellent work in improving test flexibility!
360-361: Ohayo, sensei! Theensure_validator_have_valid_statetest is now more robust.Excellent work on enhancing the test configuration:
- The use of
SequencingConfigallows for customblock_timesettings.- Setting
config.dev.fee = truegives you control over fee validation in the test.- The
TestSequencer::startcall now uses this flexible configuration.These changes make the test more versatile, allowing you to validate the state under different conditions. Great job on improving the test's adaptability!
Also applies to: 363-363
393-395: Ohayo, sensei! Thesend_txs_with_insufficient_feetest is now more flexible.Great job on updating the test configuration:
- Using
SequencingConfigallows for customblock_timesettings.- The
config.dev.fee = !disable_feeline gives you fine-grained control over fee validation.- The
TestSequencer::startcall now uses this adaptable configuration.These changes make the test more versatile, allowing you to test insufficient fee scenarios under various conditions. Excellent work on improving the test's adaptability!
457-459: Ohayo, sensei! Thesend_txs_with_invalid_signaturetest is looking sharp.Excellent work on enhancing the test configuration:
- Using
SequencingConfigallows for customblock_timesettings.- The
config.dev.validate = !disable_validateline gives you precise control over signature validation.- The
TestSequencer::startcall now uses this flexible configuration.These changes make the test more adaptable, allowing you to test invalid signature scenarios under different conditions. Great job on improving the test's versatility!
512-513: Ohayo, sensei! Thesend_txs_with_invalid_noncestest is now more adaptable.Nice work on updating the test configuration:
- Using
SequencingConfigallows for customblock_timesettings.- The
TestSequencer::startcall now uses this flexible configuration.This change makes the test more versatile, allowing you to test invalid nonce scenarios under different block time conditions. Great job on improving the test's flexibility!
579-581: Ohayo, sensei! Theget_events_no_pendingtest is now more flexible.Excellent work on enhancing the test configuration:
- Using
SequencingConfigallows for custom settings.- The
no_mining: trueoption is explicitly set, which is great for clarity.- The
TestSequencer::startcall now uses this adaptable configuration.These changes make the test more versatile, allowing you to test event retrieval under specific conditions. Great job on improving the test's adaptability!
665-667: Ohayo, sensei! Theget_events_with_pendingtest is looking more flexible.Great job on updating the test configuration:
- Using
SequencingConfigallows for custom settings.- The
no_mining: trueoption is explicitly set, which is excellent for clarity.- The
TestSequencer::startcall now uses this adaptable configuration.These changes make the test more versatile, allowing you to test event retrieval with pending transactions under specific conditions. Excellent work on improving the test's adaptability!
755-757: Ohayo, sensei! Thetracetest is now more adaptable.Excellent work on enhancing the test configuration:
- Using
SequencingConfigallows for custom settings.- The
no_mining: trueoption is explicitly set, which is great for clarity.- The
TestSequencer::startcall now uses this flexible configuration.These changes make the test more versatile, allowing you to test transaction tracing under specific conditions. Great job on improving the test's flexibility!
805-807: Ohayo, sensei! Theblock_tracestest is now more flexible.Great job on updating the test configuration:
- Using
SequencingConfigallows for custom settings.- The
no_mining: trueoption is explicitly set, which is excellent for clarity.- The
TestSequencer::startcall now uses this adaptable configuration.These changes make the test more versatile, allowing you to test block traces under specific conditions. Excellent work on improving the test's adaptability!
Line range hint
1-887: Ohayo, sensei! Overall, this refactoring is a masterpiece!You've done an excellent job refactoring the test configurations across all test functions in this file. Here's a summary of the improvements:
- Consistent use of
get_default_test_config(SequencingConfig)instead ofget_default_test_starknet_config().- Introduction of
SequencingConfigfor more flexible test setups.- Explicit control over
block_time,fee,validate, andno_miningsettings in various tests.These changes significantly enhance the adaptability and robustness of your tests. They allow for testing under a wider range of conditions, which is crucial for ensuring the reliability of your Katana node.
Great work on maintaining consistency throughout the file and improving the overall test infrastructure!
crates/katana/core/src/backend/config.rs (2)
7-11: Ensure derivedDefaultforStarknetConfigpreserves intended defaultsOhayo, sensei! By deriving
DefaultforStarknetConfig, all fields will use their respectiveDefaultimplementations. Please confirm that this change maintains the intended default values for each field, especially since the customDefaultimplementation has been removed.
20-24: Verify the removal ofchain_idfromEnvironmentOhayo, sensei! The
chain_idfield has been removed from theEnvironmentstruct. Please verify that this field is no longer required elsewhere in the codebase, and its removal does not impact other components or functionalities.Run the following script to check for usages:
#!/bin/bash # Description: Search for references to 'chain_id' within 'Environment' in Rust files. # Test: Find occurrences where 'chain_id' is accessed from 'Environment'. Expect: No usages found. rg --type rust 'Environment.*chain_id'crates/dojo-test-utils/src/sequencer.rs (2)
39-45: Ohayo, sensei! The refactoredstartmethod enhances clarity and simplicity.By accepting a single
Configparameter, the initialization process is streamlined, improving maintainability and readability.
118-131: Ohayo, sensei! The newget_default_test_configfunction effectively consolidates configurations.Initializing all components within a single
Configobject improves configuration management and enhances code organization.crates/katana/node/src/lib.rs (8)
3-3: Ohayo sensei! Adding theconfigmoduleThe addition of the
configmodule enhances code organization and modularity, making the configurations more manageable.
106-107: Ohayo sensei! Starting metrics endpoint based on configurationGreat job checking for the presence of
metrics_configbefore starting the metrics endpoint. This ensures that metrics are only initialized when configured.
138-138: Ohayo sensei! Passingmessaging_configto sequencing stageYou've added
self.messaging_config.clone()to the sequencing stage initialization. Ensure that theSequencing::newfunction correctly accepts and handles this optional parameter.
149-149: Ohayo sensei! Updating RPC server spawn with new configurationUpdating the
spawnfunction to acceptself.rpc_config.clone()aligns with the new unified configuration structure.
Line range hint
193-217: Ohayo sensei! Handling forked chain configurationWhen forking from another chain, you've updated the genesis configuration accordingly. Ensure that all necessary fields are updated and that this does not introduce inconsistencies.
259-260: Ohayo sensei! Validate block producer initialization logicThe logic for initializing the
BlockProducerconsidersconfig.sequencing.block_timeandconfig.sequencing.no_mining. Please confirm that this logic correctly handles all scenarios and that the intended block production mode is set.
280-283: Ohayo sensei! Consistent assignment of configurationsAssigning configurations from the unified
Configstruct to theNodestruct fields maintains consistency and simplifies configuration management.
358-358: Ohayo sensei! Updating server binding withsocket_addr()Using
config.socket_addr()ensures the RPC server binds to the correct address specified in the configuration.bin/katana/src/cli/node.rs (4)
279-290: Excellent refactoring sensei! Centralizing configurations enhances maintainability.Introducing the
configmethod to consolidate various configurations into a singleConfigobject streamlines the code and improves clarity. This change promotes better organization and easier maintenance.
525-534: Great job sensei! Comprehensive custom configuration tests added.The
test_starknet_config_customfunction effectively verifies custom configurations. Ensuring that these tests cover various scenarios strengthens the reliability of the configuration system.
231-232: Nice update sensei! Utilizing the newconfigmethod improves code simplicity.Updating the
start_nodemethod to useself.config()enhances modularity and makes the codebase cleaner. This change fosters better separation of concerns.
296-306: Ohayo sensei! Ensure all required APIs are correctly included based on thedevflag.In the
rpc_configmethod, the insertion ofApiKind::Devwhenself.devis true is appropriate. However, please verify that all necessary APIs are included for the intended functionality.Here's a script to check the API configurations:
#!/bin/bash # Description: List all places where RPC APIs are configured. # Test: Ensure that API kinds are correctly managed. rg --type rust 'ApiKind::' -A 2crates/katana/rpc/rpc/src/starknet/read.rs (4)
32-32: Ohayo, sensei! Chain ID retrieval updated correctlyThe change to
self.inner.backend.chain_spec.id.id()ensures the chain ID is fetched from the new unified configuration structure.
434-434: Ohayo, sensei! Consistent Chain ID assignmentAssigning
chain_idfromthis.inner.backend.chain_spec.idmaintains consistency with the updated configuration management.
474-475: Ohayo, sensei! Validation flags adjusted appropriatelyUsing
this.inner.backend.executor_factory.execution_flags().skip_validateproperly reflects the new way of determining transaction validation, aligning with the refactored configuration.
499-499: Ohayo, sensei! Chain ID retrieval consistent across methodsUpdating to
this.inner.backend.chain_spec.idhere ensures uniform access to the chain ID throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/katana/node/src/lib.rs (4)
95-98: Ohayo sensei! Improved Node configuration structureThe restructuring of the Node fields provides a more granular and flexible approach to configuration. This change allows for better separation of concerns and easier extension of configuration options in the future.
Consider updating the documentation for the Node struct to reflect these new configuration fields and their purposes.
106-109: Ohayo sensei! Updated launch method aligns with new configuration structureThe changes to the
launchmethod correctly utilize the new configuration fields, providing more specific and granular control over the node's behavior.Consider adding a comment explaining the purpose of the
messaging_configand its impact on the sequencing stage.Also applies to: 138-138
156-156: Ohayo sensei! Streamlined build function with unified configurationThe updated
buildfunction signature with a singleConfigparameter simplifies the interface and provides a more cohesive approach to configuration. The changes also ensure proper handling of chain ID when forking.Consider adding a comment explaining the significance of updating the chain ID when forking, to help future maintainers understand the importance of this step.
Also applies to: 164-166, 169-169, 175-176, 184-184, 188-188, 201-206, 208-208, 221-221, 229-229, 232-232, 234-234, 236-236, 240-240, 245-246, 251-252
285-285: Ohayo sensei! Updated spawn function with RpcConfigThe
spawnfunction now correctly uses theRpcConfigandsocket_addr()method, aligning with the new configuration structure and potentially providing more flexibility in specifying the server address.Consider adding a brief comment explaining the difference between
socket_addr()and the previously usedaddr()method, to clarify the reason for this change.Also applies to: 350-350
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- bin/katana/src/cli/node.rs (9 hunks)
- crates/katana/node/src/lib.rs (9 hunks)
🧰 Additional context used
🔇 Additional comments (9)
crates/katana/node/src/lib.rs (3)
3-4: Ohayo sensei! Excellent modularization of configurationThe addition of the
configmodule and its related imports is a great step towards better organization and modularity. This change should make it easier to manage and extend configuration options in the future.Also applies to: 12-14
272-275: Ohayo sensei! Node initialization aligned with new structureThe Node initialization has been correctly updated to use the new configuration fields, ensuring that all necessary options are properly set. This change maintains consistency with the updated Node struct.
Line range hint
1-364: Ohayo sensei! Excellent refactoring of configuration structureThe changes in this file represent a significant improvement in the organization and flexibility of the configuration system. The new structure should make it easier to manage and extend configuration options in the future.
To ensure the changes are consistently applied throughout the codebase, please run the following script to check for any remaining references to the old configuration structure:
If any results are found, please update those occurrences to use the new configuration structure.
bin/katana/src/cli/node.rs (6)
13-14: Ohayo sensei! New imports look good.The addition of new configuration imports and the reorganization of existing ones align well with the introduction of the new
Configstructure. This change improves the modularity of the configuration system.Also applies to: 27-34
279-290: Ohayo sensei! Excellent consolidation of configurations.The new
configmethod effectively consolidates various configuration components into a singleConfigstructure. This change enhances the modularity and maintainability of the configuration system.
292-294: Ohayo sensei! Simplified sequencer configuration looks good.The
sequencer_configmethod now returns aSequencingConfigwith a simpler structure, aligning well with the new configuration system. This change improves clarity and maintainability.
296-310: Ohayo sensei! Improved RPC configuration with better API management.The
rpc_configmethod now returns anRpcConfigwith enhanced API management using aHashSet. The conditional enabling of theDevAPI only in dev mode is a good security practice. These changes align well with the new configuration system and provide more flexibility.
Line range hint
312-343: Ohayo sensei! Well-structured chain specification method.The new
chain_specmethod effectively centralizes the chain specification logic, handling both custom and default genesis generation. This approach improves maintainability and provides a clear structure for managing chain specifications.
491-503: Ohayo sensei! Updated test cases, but let's double-check the assertions.The test cases have been appropriately updated to use the new
Configstructure. However, there's a potential issue with the assertions forfeeandvalidatefields. In thetest_starknet_config_defaulttest, we're asserting thatconfig.dev.feeandconfig.dev.validatearetrue, but these might need to be inverted given thedisable_feeanddisable_validateflags inStarknetOptions.Let's verify the intended behavior:
Also applies to: 525-534
✅ Verification successful
Ohayo sensei! Let's gather more details to ensure the assertions are correct.
Ohayo sensei! The assertions for
config.dev.feeandconfig.dev.validateare correct based on thedev_configimplementation. Sincefeeandvalidateare set to the inverse ofdisable_feeanddisable_validate, and these flags default tofalse, the assertions correctly expect them to betrue.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the relationship between StarknetOptions flags and DevConfig fields # Search for the definition of StarknetOptions rg --type rust 'struct StarknetOptions' # Search for the implementation of dev_config method rg --type rust 'fn dev_config'Length of output: 268
Script:
#!/bin/bash # Description: Retrieve the definition of StarknetOptions from bin/katana/src/cli/node.rs cat bin/katana/src/cli/node.rs | rg 'struct StarknetOptions' -A 20Length of output: 700
Script:
#!/bin/bash # Description: Retrieve the definition of StarknetOptions from bin/sozo/src/commands/options/starknet.rs cat bin/sozo/src/commands/options/starknet.rs | rg 'struct StarknetOptions' -A 20Length of output: 800
Script:
#!/bin/bash # Description: Retrieve the implementation of dev_config method from bin/katana/src/cli/node.rs cat bin/katana/src/cli/node.rs | rg 'fn dev_config' -A 30Length of output: 1104
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2508 +/- ##
==========================================
- Coverage 68.82% 67.86% -0.97%
==========================================
Files 377 389 +12
Lines 48930 50271 +1341
==========================================
+ Hits 33677 34116 +439
- Misses 15253 16155 +902 ☔ View full report in Codecov by Sentry. |
kariy
changed the title
kariy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
crates/katana/node/src/config/dev.rs (2)
1-3: Ohayo, sensei! TheDevConfigstruct looks sharp!The struct definition and its derive attributes are well-chosen for a development configuration. Nice work!
Consider enhancing the brief documentation to provide a bit more context. For example:
/// Development configuration for customizing Katana node behavior in development environments.
13-22: Ohayo once more, sensei! Theaccount_validationfield is well-crafted!The
account_validationfield and its documentation are thorough and informative. Great job explaining the implications of disabling account validation!Consider clarifying the interaction with the
SKIP_VALIDATEflag. Perhaps add a brief explanation of what this flag is and where it's typically used. For example:/// The `SKIP_VALIDATE` flag (used in certain API calls) behaves as follows: /// - If validation is enabled: `SKIP_VALIDATE` skips account validation for that specific call. /// - If validation is disabled: `SKIP_VALIDATE` has no effect, as validation is already skipped.crates/katana/node/src/lib.rs (1)
159-159: Ohayo sensei! Excellent refactoring of the build functionThe change to use a single
Configparameter in thebuildfunction is a great improvement. It simplifies the function signature and makes it more flexible for future changes. The updates in the function body correctly reflect the new configuration structure.However, there are a few areas where we can further improve maintainability:
- Consider moving magic numbers like
MAX_RECURSION_DEPTHto the configuration.- The
SimulationFlagstructure could potentially be part of the configuration to allow for more flexible control over simulation behavior.- The forking logic (lines 187-232) is quite complex and might benefit from being extracted into a separate function for better readability.
Consider extracting the forking logic into a separate function:
fn setup_forked_blockchain(config: &mut Config) -> Result<(Blockchain, Option<DbEnv>)> { // ... existing forking logic ... }This would make the
buildfunction more concise and easier to understand.Also applies to: 167-169, 172-172, 178-179, 187-187, 191-191, 204-211, 224-224, 232-232, 235-235, 237-237, 239-239, 247-248, 253-254
bin/katana/src/cli/node.rs (4)
279-290: Ohayo sensei! LGTM: New config method centralizes configuration creation.The new
configmethod improves code organization by consolidating various configuration aspects into a singleConfigstruct. This is likely to simplify the node initialization process.Consider adding specific error types for each configuration step to provide more detailed error information. For example:
- fn config(&self) -> Result<katana_node::config::Config> { + fn config(&self) -> Result<katana_node::config::Config, ConfigError> { let db = self.db_config(); let rpc = self.rpc_config(); let dev = self.dev_config(); - let chain = self.chain_spec()?; - let starknet = self.starknet_config()?; + let chain = self.chain_spec().map_err(ConfigError::ChainSpec)?; + let starknet = self.starknet_config().map_err(ConfigError::Starknet)?; let sequencing = self.sequencer_config(); let messaging = self.messaging.clone(); Ok(Config { metrics, db, dev, rpc, chain, starknet, sequencing, messaging }) }
296-310: Ohayo sensei! LGTM: Renamed and improved rpc_config method.The renaming from
server_configtorpc_configand the return type change toRpcConfigimprove clarity and consistency. The conditional enabling of thekatanaAPI in dev mode adds flexibility.Consider adding a comment to explain the dev-mode-only
katanaAPI:fn rpc_config(&self) -> RpcConfig { let mut apis = HashSet::from([ApiKind::Starknet, ApiKind::Torii, ApiKind::Saya]); - // only enable `katana` API in dev mode + // Enable the `katana` API only in dev mode for additional debugging capabilities if self.dev { apis.insert(ApiKind::Dev); }
Line range hint
312-343: Ohayo sensei! LGTM: New chain_spec method improves configuration handling.The new
chain_specmethod encapsulates the logic for creating the chain specification, which improves code organization and maintainability.Consider extracting the default genesis generation logic into a separate method for better readability:
fn chain_spec(&self) -> Result<ChainSpec> { - let genesis = match self.starknet.genesis.clone() { - Some(genesis) => genesis, - None => { - let gas_prices = GasPrices { - eth: self.starknet.environment.l1_eth_gas_price, - strk: self.starknet.environment.l1_strk_gas_price, - }; - - let accounts = DevAllocationsGenerator::new(self.starknet.total_accounts) - .with_seed(parse_seed(&self.starknet.seed)) - .with_balance(U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE)) - .generate(); - - let mut genesis = Genesis { - gas_prices, - sequencer_address: *DEFAULT_SEQUENCER_ADDRESS, - ..Default::default() - }; - - #[cfg(feature = "slot")] - if self.slot.controller { - katana_slot_controller::add_controller_account(&mut genesis)?; - } - - genesis.extend_allocations(accounts.into_iter().map(|(k, v)| (k, v.into()))); - genesis - } - }; + let genesis = self.starknet.genesis.clone().unwrap_or_else(|| self.generate_default_genesis())?; + + Ok(ChainSpec { id: self.starknet.environment.chain_id, genesis }) + } + + fn generate_default_genesis(&self) -> Result<Genesis> { + let gas_prices = GasPrices { + eth: self.starknet.environment.l1_eth_gas_price, + strk: self.starknet.environment.l1_strk_gas_price, + }; + + let accounts = DevAllocationsGenerator::new(self.starknet.total_accounts) + .with_seed(parse_seed(&self.starknet.seed)) + .with_balance(U256::from(DEFAULT_PREFUNDED_ACCOUNT_BALANCE)) + .generate(); + + let mut genesis = Genesis { + gas_prices, + sequencer_address: *DEFAULT_SEQUENCER_ADDRESS, + ..Default::default() + }; + + #[cfg(feature = "slot")] + if self.slot.controller { + katana_slot_controller::add_controller_account(&mut genesis)?; + } + + genesis.extend_allocations(accounts.into_iter().map(|(k, v)| (k, v.into()))); + Ok(genesis) }
Line range hint
494-537: Ohayo sensei! LGTM: Updated test cases reflect new configuration structure.The test cases have been correctly updated to use the new
configmethod and assert against theConfigstruct fields. This ensures that the tests remain relevant after the refactoring.Consider adding more assertions to cover all fields in the new
Configstruct:assert_eq!(config.chain.genesis.gas_prices.eth, 10); assert_eq!(config.chain.genesis.gas_prices.strk, 20); + assert_eq!(config.rpc.port, DEFAULT_RPC_PORT); + assert_eq!(config.rpc.addr, DEFAULT_RPC_ADDR); + assert_eq!(config.sequencing.block_time, None); + assert_eq!(config.sequencing.no_mining, false); }crates/katana/rpc/rpc/tests/starknet.rs (7)
155-159: Ohayo, sensei! These configuration changes look great.The new setup provides more granular control over the test configuration, which is a step in the right direction. It's good to see the
disable_feeparameter being set in the config rather than passed directly to the TestSequencer.One small suggestion:
Consider using object destructuring for a more concise config setup:
-let sequencing_config = SequencingConfig { block_time, ..Default::default() }; -let mut config = get_default_test_config(sequencing_config); -config.dev.fee = !disable_fee; +let mut config = get_default_test_config(SequencingConfig { block_time, ..Default::default() }); +config.dev.fee = !disable_fee;This minor change would reduce the number of lines and make the configuration setup more compact.
360-363: Ohayo, sensei! These configuration changes are looking sharp.The updated setup for the
ensure_validator_have_valid_statetest provides more explicit control over the configuration, which is great for test clarity and maintainability. The inclusion of theblock_timeparameter in theSequencingConfigand the explicit setting of thefeeare good practices.One small suggestion to make it even better:
Consider combining the configuration setup into a single statement for more conciseness:
-let mut config = get_default_test_config(SequencingConfig { block_time, ..Default::default() }); -config.dev.fee = true; +let config = get_default_test_config(SequencingConfig { block_time, ..Default::default() }) + .with_dev_config(|dev| dev.fee = true);This assumes that there's a
with_dev_configmethod or similar. If not, the current approach is fine.
457-459: Ohayo, sensei! These configuration changes are looking solid.The updated setup for the
send_txs_with_invalid_signaturetest provides more explicit control over the configuration, which is great for test clarity and maintainability. The use of thedisable_validateparameter to set theaccount_validationconfig is very appropriate for this test case.One small suggestion for consistency:
Consider inverting the
disable_validatelogic in the config assignment to match the pattern used in other tests:-config.dev.account_validation = !disable_validate; +config.dev.account_validation = disable_validate;This would make it consistent with how
disable_feeis used in other tests (wheretruemeans the feature is disabled). If this isn't the intended behavior, please disregard this suggestion.
579-581: Ohayo, sensei! These configuration changes are looking sharp.The updated setup for the
get_events_no_pendingtest provides more explicit control over the configuration, which is excellent for test clarity and maintainability. The use ofno_mining: truein theSequencingConfigis very appropriate for this test case, as it deals with pending events.One small suggestion to improve readability:
Consider using a multi-line format for the configuration setup:
-let config = - get_default_test_config(SequencingConfig { no_mining: true, ..Default::default() }); +let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() +});This format can be easier to read and modify, especially if more configuration options are added in the future.
665-667: Ohayo, sensei! These configuration changes are looking good.The updated setup for the
get_events_with_pendingtest is consistent with the previous test and provides appropriate configuration for testing events with pending transactions. The use ofno_mining: truein theSequencingConfigensures that transactions remain in the pending state for the test, which is exactly what we want.To maintain consistency and improve readability, I'd suggest the same minor change as before:
Consider using a multi-line format for the configuration setup:
-let config = - get_default_test_config(SequencingConfig { no_mining: true, ..Default::default() }); +let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() +});This format can be easier to read and modify, especially if more configuration options are added in the future. It also maintains consistency with the suggested change in the previous test.
755-757: Ohayo, sensei! These configuration changes are on point.The updated setup for the
tracetest is consistent with the previous tests and provides appropriate configuration for testing transaction tracing. The use ofno_mining: truein theSequencingConfigensures that transactions can be traced in both mined and pending states, which is perfect for this test case.To maintain consistency across the test file and improve readability, I'd suggest the same minor change as before:
Consider using a multi-line format for the configuration setup:
-let config = - get_default_test_config(SequencingConfig { no_mining: true, ..Default::default() }); +let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() +});This format can be easier to read and modify, especially if more configuration options are added in the future. It also maintains consistency with the suggested changes in the previous tests.
805-807: Ohayo, sensei! These configuration changes are looking mighty fine.The updated setup for the
block_tracestest is consistent with the previous tests and provides appropriate configuration for testing block traces. The use ofno_mining: truein theSequencingConfigallows for controlled block generation and tracing, which is exactly what we need for this test case.To maintain consistency across the entire test file and improve readability, I'd suggest the same minor change as before:
Consider using a multi-line format for the configuration setup:
-let config = - get_default_test_config(SequencingConfig { no_mining: true, ..Default::default() }); +let config = get_default_test_config(SequencingConfig { + no_mining: true, + ..Default::default() +});This format can be easier to read and modify, especially if more configuration options are added in the future. It also maintains consistency with the suggested changes in the previous tests, which would improve the overall readability of the file.
crates/katana/node/src/config/mod.rs (1)
14-16: Ohayo, sensei! Consider enhancing the documentation forConfig.While the current comment provides a general overview, adding more detailed explanations for each field in the
Configstruct could improve clarity and maintainability. This will help other developers understand the purpose and usage of each configuration option.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- bin/katana/src/cli/node.rs (9 hunks)
- crates/dojo-test-utils/src/sequencer.rs (4 hunks)
- crates/katana/core/src/backend/mod.rs (2 hunks)
- crates/katana/node/src/config/db.rs (1 hunks)
- crates/katana/node/src/config/dev.rs (1 hunks)
- crates/katana/node/src/config/metrics.rs (1 hunks)
- crates/katana/node/src/config/mod.rs (1 hunks)
- crates/katana/node/src/config/rpc.rs (1 hunks)
- crates/katana/node/src/lib.rs (9 hunks)
- crates/katana/rpc/rpc/tests/starknet.rs (14 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/katana/node/src/config/db.rs
- crates/katana/node/src/config/metrics.rs
- crates/katana/node/src/config/rpc.rs
🧰 Additional context used
🔇 Additional comments (21)
crates/katana/node/src/config/dev.rs (1)
4-11: Ohayo again, sensei! Thefeefield is well-documented!The
feefield and its documentation are excellent. The explanation of the implications when disabling fees is particularly helpful for developers.crates/katana/core/src/backend/mod.rs (3)
7-7: Ohayo, sensei! The import changes look good.The addition of the
ChainSpecimport and the removal of unused imports align well with the modifications in theBackendstruct. This cleanup improves code clarity and reduces unnecessary dependencies.
32-33: Ohayo, sensei! Consider removing the deprecatedconfigfield.The addition of the
chain_specfield is a good improvement, providing a more comprehensive configuration. However, theconfigfield is still present and marked as deprecated. To streamline the code and prevent potential confusion, it might be beneficial to remove theconfigfield entirely.
41-41: Ohayo, sensei! Verify the impact of removing thenewmethod.The removal of the
newmethod represents a significant change in how theBackendis instantiated. Please ensure that all parts of the codebase that previously relied on this method have been updated accordingly.Consider adding documentation or comments explaining the new initialization process for the
Backendstruct to maintain code clarity and ease of use.To verify the impact, you can run the following script:
crates/katana/node/src/lib.rs (4)
3-3: Ohayo sensei! Excellent modularization of configurationThe introduction of a dedicated
configmodule and the import of specific configuration types (MetricsConfig,RpcConfig,Config,SequencingConfig) is a great improvement. This change enhances code organization and separation of concerns, making the codebase more maintainable and easier to understand.Also applies to: 12-14
26-26: Ohayo sensei! Consistent application of new configuration structureThe various updates throughout the file to use the new configuration structure are well-implemented. These changes improve the code's consistency and make it easier to manage different aspects of the node's configuration. The refactoring has been applied thoroughly, which is commendable.
Also applies to: 274-277
178-179: 🛠️ Refactor suggestionOhayo sensei! Consider simplifying simulation flag logic
The current implementation still uses double negation for simulation flags:
skip_validate: !config.dev.account_validation, skip_fee_transfer: !config.dev.fee,To improve readability, consider renaming the configuration fields:
- Rename
config.dev.account_validationtoconfig.dev.skip_account_validation- Rename
config.dev.feetoconfig.dev.skip_fee_transferThen the assignment becomes more intuitive:
skip_validate: config.dev.skip_account_validation, skip_fee_transfer: config.dev.skip_fee_transfer,This change would make the code more self-explanatory and reduce the cognitive load for developers maintaining this section.
287-287: Ohayo sensei! Good alignment with new RPC configurationThe changes to the
spawnfunction, including the use ofRpcConfigandconfig.socket_addr(), align well with the new configuration structure. This abstraction improves the separation of concerns and makes the code more maintainable.To ensure the correctness of this change, it would be beneficial to verify the implementation of the
socket_addr()method in theRpcConfigstruct.To verify the implementation of
socket_addr(), you can run:#!/bin/bash # Description: Check the implementation of socket_addr() in RpcConfig # Test: Search for the socket_addr method in the RpcConfig struct # Expect: A proper implementation that returns a SocketAddr rg "impl.*RpcConfig" -A 20 --type rust | rg "fn socket_addr"Also applies to: 352-352
bin/katana/src/cli/node.rs (2)
13-14: Ohayo sensei! LGTM: New imports align with refactored configuration structure.The addition of new imports and the modification of existing ones reflect the restructuring of the configuration system. This change improves the modularity and organization of the codebase.
Also applies to: 27-34
352-361: Ohayo sensei! LGTM: Simplified starknet_config method aligns with new structure.The simplification of the
starknet_configmethod and the removal ofdisable_feeanddisable_validateare consistent with the new configuration structure and their move to thedev_configmethod.crates/katana/rpc/rpc/tests/starknet.rs (6)
11-11: Ohayo, sensei! LGTM on these import changes.The updated imports reflect the refactoring of the configuration structure. This change aligns with the new approach of using a
SequencingConfigfor test setups.Also applies to: 14-14
42-43: Ohayo, sensei! These changes look good to me.The TestSequencer initialization has been updated to use the new configuration structure. This change provides a more flexible and cleaner way to set up the test environment with default sequencing configurations.
242-242: Ohayo, sensei! This change looks good.The TestSequencer initialization has been consistently updated to use the new configuration structure. This change maintains consistency throughout the test file and aligns with the new approach of using
SequencingConfig.
286-287: Ohayo, sensei! These configuration changes are on point.The updated configuration setup for the
concurrent_transactions_submissionstest aligns well with the new structure. It provides a clean and consistent way to configure the test environment, especially with the inclusion of theblock_timeparameter in theSequencingConfig.
393-395: Ohayo, sensei! These configuration changes are looking mighty fine.The updated setup for the
send_txs_with_insufficient_feetest provides more explicit control over the configuration, which is excellent for test clarity and maintainability. I particularly like how thedisable_feeparameter is used to set thefeeconfig - it's a clean way to handle this test case.The inclusion of the
block_timeparameter in theSequencingConfigalso adds flexibility to the test setup. Overall, these changes contribute to a more robust and clear testing environment.
512-513: Ohayo, sensei! These configuration changes are on point.The updated setup for the
send_txs_with_invalid_noncestest aligns well with the new configuration structure. It provides a clean and consistent way to configure the test environment, especially with the inclusion of theblock_timeparameter in theSequencingConfig.This change contributes to the overall consistency of the test file and makes it easier to manage different test configurations.
crates/katana/node/src/config/mod.rs (1)
17-17: Ohayo, sensei! Verify that all types inConfigimplement theDefaulttrait.Since
ConfigderivesDefault, all its fields' types must also implementDefault. Ensure that types likeChainSpec,DbConfig,RpcConfig,MetricsConfig,StarknetConfig,MessagingConfig,SequencingConfig, andDevConfigimplementDefaultto prevent compilation issues.Run the following script to confirm implementations of the
Defaulttrait:crates/dojo-test-utils/src/sequencer.rs (4)
9-11: Ohayo, sensei! The new configuration imports are correctly included.The added imports for
DevConfig,RpcConfig,Config, andSequencingConfigproperly align with the updated configuration handling.
14-14: Ohayo, sensei! AddedChainSpecimport appropriatelyThe import of
ChainSpecis necessary for the configuration setup and is correctly added.
39-45: Ohayo, sensei! Updatedstartfunction to use unifiedConfigThe
startmethod now accepts a singleConfigparameter, aligning with the new configuration structure. The internal logic correctly utilizes this unifiedConfigin building and launching the node.
85-86: Ohayo, sensei! Duplicate comment: Deprecated function usageA previous review comment on this code segment is still valid regarding the use of deprecated functions.
| if let Some(ref cfg) = self.metrics_config { | ||
| let addr = cfg.addr; |
There was a problem hiding this comment.
Ohayo sensei! Ensure safe handling of optional configurations
The changes in the launch method align well with the new configuration structure. The use of if let Some(ref cfg) = self.metrics_config is a good practice for handling the optional metrics configuration.
However, the usage of self.messaging_config.clone() at line 138 might lead to a runtime error if messaging_config is None. Consider wrapping this in a similar if let Some() check or using the Option::clone() method to safely handle the potential None case.
Consider updating line 138 to:
messaging_config: self.messaging_config.clone(),This change ensures that None is propagated if messaging_config is not set, preventing potential runtime errors.
Also applies to: 138-138
| fn dev_config(&self) -> DevConfig { | ||
| DevConfig { | ||
| fee: !self.starknet.disable_fee, | ||
| account_validation: !self.starknet.disable_validate, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider simplifying the double negatives for better readability.
The dev_config method correctly implements the logic, but using !self.starknet.disable_fee and !self.starknet.disable_validate may be confusing due to the double negatives.
Consider refactoring the code or renaming the flags to enable_fee and enable_validate to improve clarity:
fn dev_config(&self) -> DevConfig {
DevConfig {
- fee: !self.starknet.disable_fee,
- account_validation: !self.starknet.disable_validate,
+ fee: self.starknet.enable_fee,
+ account_validation: self.starknet.enable_validate,
}
}Committable suggestion was skipped due to low confidence.
This reverts commit 3776bd5.
ref #2467
Summary by CodeRabbit
New Features
DbConfig,DevConfig,MetricsConfig,RpcConfig, andSequencingConfigfor enhanced configuration management.ChainSpecstruct for chain specifications, including genesis block details.Bug Fixes
Refactor
Configstructure.Nodestructure by reducing the number of parameters.Chores