refactor(katana): rename simulation flags for consistency #2581
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes to the transaction execution handling in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
crates/katana/pool/src/validation/stateful.rs (1)
146-147: Ohayo! Consider adding tests for the flag behavior changes, sensei!While the changes look good, it would be beneficial to ensure that the new flag behavior is covered by tests, especially the interaction between
skip_validate,account_validation(), andfee()methods in the validation logic.Would you like me to help generate test cases to verify this behavior?
crates/katana/executor/src/implementation/blockifier/mod.rs (1)
81-81: Verify naming consistency in StarknetVMProcessorWhile most fields have been renamed to use "execution", the field
simulation_flagsin StarknetVMProcessor still retains the old terminology despite being of typeExecutionFlags.Consider applying this change for complete consistency:
- simulation_flags: ExecutionFlags, + execution_flags: ExecutionFlags,Remember to update the constructor assignment as well:
- Self { block_context, state, transactions, simulation_flags, stats: Default::default() } + Self { block_context, state, transactions, execution_flags: simulation_flags, stats: Default::default() }Also applies to: 90-90
crates/katana/node/src/lib.rs (1)
172-174: Ohayo! The ExecutionFlags initialization looks clean, sensei!The builder pattern implementation is elegant. Consider adding a doc comment to explain the significance of these flags for future maintainers.
+ // Initialize execution flags based on dev config settings let execution_flags = ExecutionFlags::new() .with_account_validation(config.dev.account_validation) .with_fee(config.dev.fee);crates/katana/executor/tests/fixtures/mod.rs (1)
252-253: Ohayo sensei! Consider making the flag names more intuitive.The double negation in
!skip_validateand!skip_fee_transfermight be confusing. Consider renaming the parameters to match the positive intent of the flag methods:-pub fn flags( - #[default(false)] skip_validate: bool, - #[default(false)] skip_fee_transfer: bool, -) -> ExecutionFlags { - ExecutionFlags::new().with_account_validation(!skip_validate).with_fee(!skip_fee_transfer) +pub fn flags( + #[default(true)] enable_validation: bool, + #[default(true)] enable_fee: bool, +) -> ExecutionFlags { + ExecutionFlags::new().with_account_validation(enable_validation).with_fee(enable_fee)crates/katana/rpc/rpc/src/starknet/trace.rs (1)
75-77: Consider improving variable naming for clarity, sensei!The
should_skip_feevariable is negated when passed towith_fee(!should_skip_fee), which might be slightly confusing. Consider renaming it toshould_charge_feeto match the positive logic used in the builder method:-let should_skip_fee = !simulation_flags.contains(&SimulationFlag::SkipFeeCharge) - && self.inner.backend.executor_factory.execution_flags().fee(); +let should_charge_fee = !simulation_flags.contains(&SimulationFlag::SkipFeeCharge) + && self.inner.backend.executor_factory.execution_flags().fee(); let flags = katana_executor::ExecutionFlags::new() .with_account_validation(should_validate) - .with_fee(!should_skip_fee); + .with_fee(should_charge_fee);crates/katana/rpc/rpc/src/starknet/read.rs (1)
485-492: Clean implementation using builder pattern!The code effectively uses the new
ExecutionFlagsbuilder pattern to configure validation and nonce checking settings. The comments clearly explain why nonce checking is disabled during fee estimation.However, consider adding a brief comment explaining why
account_validationis set based onshould_validate:let flags = katana_executor::ExecutionFlags::new() + // Enable validation only if not explicitly skipped and supported by node .with_account_validation(should_validate) .with_nonce_check(false);crates/katana/executor/src/implementation/blockifier/utils.rs (1)
77-81: Ohayo, sensei! Consider documenting the nonce check behavior.The comment explains an important limitation about the nonce check behavior in Blockifier. Consider moving this documentation to the
ExecutionFlagstype definition for better visibility.crates/katana/executor/src/abstraction/mod.rs (2)
24-32: Consider derivingCopyforExecutionFlags, senseiSince
ExecutionFlagscontains onlyboolfields, derivingCopyallows for easier copying without needing to clone.Apply this diff to derive
Copy:#[derive(Debug, Clone)] +#[derive(Copy)] pub struct ExecutionFlags { /// Determine whether to perform the transaction sender's account validation logic. account_validation: bool, /// Determine whether to perform fee related checks and operations ie., fee transfer. fee: bool, /// Determine whether to perform transaction's sender nonce check. nonce_check: bool, }
44-60: Add#[must_use]to builder methods, senseiTo prevent accidental misuse, consider adding
#[must_use]to the builder methods to ensure the returned values are used.Apply this diff to add
#[must_use]attributes:+ #[must_use] pub fn with_account_validation(mut self, enable: bool) -> Self { self.account_validation = enable; self } + #[must_use] pub fn with_fee(mut self, enable: bool) -> Self { self.fee = enable; self } + #[must_use] pub fn with_nonce_check(mut self, enable: bool) -> Self { self.nonce_check = enable; self }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
- crates/katana/executor/benches/concurrent.rs (3 hunks)
- crates/katana/executor/benches/execution.rs (3 hunks)
- crates/katana/executor/src/abstraction/executor.rs (3 hunks)
- crates/katana/executor/src/abstraction/mod.rs (2 hunks)
- crates/katana/executor/src/implementation/blockifier/mod.rs (8 hunks)
- crates/katana/executor/src/implementation/blockifier/utils.rs (1 hunks)
- crates/katana/executor/src/implementation/noop.rs (4 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (3 hunks)
- crates/katana/executor/tests/simulate.rs (4 hunks)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/pool/src/validation/stateful.rs (4 hunks)
- crates/katana/rpc/rpc/src/starknet/mod.rs (1 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (1 hunks)
- crates/katana/rpc/rpc/src/starknet/trace.rs (1 hunks)
🔇 Additional comments (39)
crates/katana/executor/benches/execution.rs (4)
6-6: Ohayo! Import statement looks perfect, sensei!The updated import aligns well with the renaming refactor.
22-22: Clean flag initialization, sensei!The ExecutionFlags instantiation maintains the same pattern while adopting the new type name.
33-33: Parameter type update looks solid, sensei!The function signature maintains proper reference semantics while adopting the new type name.
Line range hint
6-33: Verify complete migration to ExecutionFlagsLet's ensure we haven't missed any occurrences of the old type name.
crates/katana/executor/src/abstraction/executor.rs (3)
9-10: Ohayo! Clean import organization, sensei! ✨The addition of
ExecutionFlagsto the import list is well-organized and properly grouped with related types.
33-33: Clean trait method update, sensei! 🎯The return type change from
&SimulationFlagto&ExecutionFlagsaligns perfectly with the refactoring objective while maintaining the same semantic meaning.
64-64: Excellent consistency in method signatures, sensei! 🌟The parameter type changes from
SimulationFlagtoExecutionFlagsin bothsimulate()andestimate_fee()methods are consistently applied, maintaining a clean and unified API design.Also applies to: 71-71
crates/katana/executor/tests/simulate.rs (5)
5-5: LGTM! Import statement updated correctly.The import statement has been updated to use
ExecutionFlagsinstead ofSimulationFlag, maintaining consistency with the refactoring.
22-24: Ohayo sensei! Test cases look good with improved clarity.The test case definitions have been updated to use
ExecutionFlagswith more descriptive flag configurations. The new.with_account_validation(false)is more explicit than the previous.skip_validate().
26-26: Test case for no signature validation is properly maintained.The panic test case for transactions without signatures has been correctly updated to use the new flag system while maintaining the expected behavior.
94-94: Blockifier test integration maintained correctly.The blockifier-specific test implementation has been properly updated to use the new
ExecutionFlagstype.
32-32: Function signatures updated consistently.The parameter types in both the test template and implementation function have been updated to use
ExecutionFlags.Let's verify that all usages of the old flag type have been updated:
Also applies to: 42-42
crates/katana/executor/benches/concurrent.rs (4)
12-12: Ohayo! Import statement looks perfect, sensei!The update to use
ExecutionFlagsaligns well with the renaming initiative.
31-31: Excellent API improvement, sensei!The new
with_account_validation(false)is more explicit and self-documenting compared to the previousskip_validate(). This enhances code readability while maintaining the same functionality.
43-43: Parameter type update looks good, sensei!The function signature has been properly updated to use
ExecutionFlags, maintaining consistency with the new type system.
Line range hint
12-43: Verify complete migration of flag usage, sensei!Let's ensure all instances of the old flag type have been properly migrated in the benchmark code.
✅ Verification successful
Ohayo! All flag usages are properly aligned, sensei!
The verification shows:
SimulationFlagis only used in RPC-related code for API types and request handlingExecutionFlagsis consistently used across executor benchmarks, tests, and node execution- The benchmark's usage of
ExecutionFlags::new().with_account_validation(false)aligns with other test cases and execution patterns🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to SimulationFlag rg "SimulationFlag" crates/katana/ # Verify consistent usage of ExecutionFlags rg "ExecutionFlags::" crates/katana/Length of output: 2777
crates/katana/executor/src/implementation/noop.rs (3)
13-14: Ohayo! Import changes look perfect, sensei!The updated imports correctly reflect the transition from
SimulationFlagtoExecutionFlags.
22-22: Field type update looks great, sensei!The
execution_flagsfield type has been properly updated to use the newExecutionFlagstype.
58-59: Method signatures updated consistently, but let's verify usage!The method signatures have been properly updated to use
ExecutionFlags. Let's verify that all callers have been updated accordingly.Also applies to: 72-72, 82-82
✅ Verification successful
The previous results show SimulationFlag is still present in RPC-related code, but we need to verify the executor implementation specifically. These commands will help us understand the complete context of the ExecutionFlags usage and verify if the changes are consistent within the executor module.
All changes are properly aligned with ExecutionFlags, sensei!
The verification shows that:
- The
execution_flagsmethod innoop.rscorrectly returns&ExecutionFlags- All callers have been updated to use
ExecutionFlagsconsistently- The remaining
SimulationFlagreferences are in RPC types which are separate from the executor implementation and serve as API typesThe method signature changes are correct and all usages are consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that there are no remaining references to SimulationFlag # and all callers use ExecutionFlags # Test 1: Check for any remaining SimulationFlag references echo "Checking for remaining SimulationFlag references..." rg "SimulationFlag" # Test 2: Verify ExecutionFlags usage in method calls echo "Verifying ExecutionFlags usage in method calls..." rg -A 2 "simulate.*flags:|estimate_fee.*flags:"Length of output: 1834
Script:
#!/bin/bash # Let's check for ExecutionFlags usage patterns to verify the changes echo "Checking ExecutionFlags usage..." rg "ExecutionFlags" -A 2 # Also check for execution_flags method calls echo "Checking execution_flags method calls..." rg "execution_flags\(" -A 2 # Let's also check the specific file content echo "Checking the specific file content..." cat crates/katana/executor/src/implementation/noop.rsLength of output: 19824
crates/katana/pool/src/validation/stateful.rs (4)
15-15: Ohayo! Import change looks good, sensei!The transition from
SimulationFlagtoExecutionFlagsin the imports aligns well with the PR's objective of improving naming consistency.
38-38: Clean field type update, sensei!The field type update in the
Innerstruct maintains consistency with the newExecutionFlagstype.
47-47: Constructor parameter update looks solid!The parameter type change in the
new()method properly propagates theExecutionFlagstype through the initialization chain.
146-147: Verify the semantic equivalence of the flag checks, sensei!The changes from direct flag access to method calls (
account_validation()andfee()) appear to be part of the refactoring. However, we should verify that these new methods maintain the same semantic meaning as the previousskip_validateandskip_fee_transferflags.Let's check the implementation of these methods:
✅ Verification successful
These commands will help us understand the implementation and usage of ExecutionFlags better. The previous ast-grep pattern might have been too specific, so let's try a broader search approach to find the relevant code.
The flag checks maintain semantic equivalence, sensei! ✨
The implementation shows that:
account_validation()is a direct getter for the internalaccount_validationboolean fieldfee()is a direct getter for the internalfeeboolean field- Both fields default to
trueand can be toggled via builder methods- The usage patterns across the codebase confirm consistent behavior
The refactoring from direct field access to method calls maintains identical semantics while providing better encapsulation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the ExecutionFlags implementation to verify the semantic equivalence ast-grep --pattern 'impl ExecutionFlags { $$$ pub fn account_validation($$$) -> bool { $$$ } pub fn fee($$$) -> bool { $$$ } $$$ }'Length of output: 156
Script:
#!/bin/bash # Let's try a broader search for ExecutionFlags implementations rg -U "impl ExecutionFlags" -A 20Length of output: 1615
Script:
#!/bin/bash # Let's also look for the struct definition rg "struct ExecutionFlags" -A 10Length of output: 949
Script:
#!/bin/bash # Search for any usage patterns of these flags rg "account_validation|fee\(\)" -A 2 -B 2Length of output: 16213
crates/katana/executor/src/implementation/blockifier/mod.rs (2)
25-26: Ohayo sensei! The imports look good!The updated imports correctly reflect the type rename from
SimulationFlagtoExecutionFlags, maintaining consistency with the broader refactoring effort.
35-35: Clean and consistent renaming in BlockifierFactory, sensei!The renaming from
simulation_flagstoflagsin the struct field and corresponding updates in the constructor and accessor methods are well-aligned with the new terminology.Also applies to: 40-40, 71-71
crates/katana/node/src/lib.rs (2)
28-28: Ohayo sensei! Import change looks good!The update from
SimulationFlagtoExecutionFlagsaligns perfectly with the PR's objective of improving naming consistency.
176-176: Looking good, but let's verify the BlockifierFactory usage, sensei!The BlockifierFactory creation with the new ExecutionFlags is correct. Let's verify other usages to ensure consistency.
✅ Verification successful
Ohayo! All BlockifierFactory usages are consistent, sensei!
The verification shows all instances of
BlockifierFactory::new()are correctly using both the config environment and execution flags parameters:
- Main node implementation in
katana/node/src/lib.rs- Benchmarking code in
katana/executor/benches/concurrent.rs- Test fixtures in
katana/executor/tests/fixtures/mod.rs🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BlockifierFactory usage patterns to ensure consistent ExecutionFlags usage rg -A 3 "BlockifierFactory::new\("Length of output: 790
crates/katana/executor/tests/fixtures/mod.rs (2)
5-5: Ohayo sensei! The import looks good!The import statement correctly reflects the type rename from
SimulationFlagtoExecutionFlags.
268-268: Ohayo sensei! The blockifier module changes look good!The changes correctly implement the type rename while maintaining the existing behavior with flags enabled by default.
Also applies to: 273-273
crates/katana/rpc/rpc/src/starknet/trace.rs (2)
67-68: Ohayo! The flag handling logic looks good, sensei!The changes maintain the correct behavior while improving clarity by explicitly checking both the simulation flags and the executor's configuration.
Also applies to: 72-73
67-77: Verify consistent flag usage across the codebase, sensei!Let's ensure all usages of
SimulationFlagare being updated to use the newExecutionFlagstype consistently.✅ Verification successful
Ohayo! The flag usage is consistent across the codebase, sensei!
The verification shows:
SimulationFlagis properly type-aliased fromstarknet::core::types- Both
trace.rsandread.rsuseExecutionFlagsconsistently with similar patterns:
- Both create flags with
ExecutionFlags::new()- Both apply validation flags appropriately
- The fee handling is consistent with the simulation flags
The changes in the review are properly aligned with the rest of the codebase's usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining SimulationFlag usages that might need updating echo "Checking for remaining SimulationFlag usages..." rg --type rust "SimulationFlag" -l echo "Checking for new ExecutionFlags usages..." rg --type rust "ExecutionFlags" -l echo "Checking for potential missed imports..." rg --type rust "use.*SimulationFlag|use.*ExecutionFlags" -lLength of output: 1594
Script:
#!/bin/bash # Let's check the actual implementation of SimulationFlag and ExecutionFlags usage in these files echo "=== Checking SimulationFlag implementation ===" rg --type rust -A 5 "SimulationFlag" crates/katana/rpc/rpc-types/src/lib.rs echo -e "\n=== Checking ExecutionFlags usage in trace.rs ===" rg --type rust -A 10 "ExecutionFlags" crates/katana/rpc/rpc/src/starknet/trace.rs echo -e "\n=== Checking ExecutionFlags usage in read.rs ===" rg --type rust -A 10 "ExecutionFlags" crates/katana/rpc/rpc/src/starknet/read.rs echo -e "\n=== Checking imports in read.rs ===" rg --type rust "use.*ExecutionFlags" crates/katana/rpc/rpc/src/starknet/read.rsLength of output: 1840
crates/katana/rpc/rpc/src/starknet/mod.rs (1)
97-97: Ohayo! The flag type rename looks good, sensei!The change from
SimulationFlagtoExecutionFlagsaligns well with the PR's objective of making flag naming more consistent across the codebase. The method implementation correctly uses the renamed type.crates/katana/rpc/rpc/src/starknet/read.rs (1)
482-484: Ohayo! Clear and improved validation logic, sensei!The validation check now explicitly considers both the user's request (
skip_validate) and the node's configuration (account_validation()), making the intent more apparent.crates/katana/executor/src/implementation/blockifier/utils.rs (3)
59-59: LGTM! Import changes look good.The import change from
SimulationFlagtoExecutionFlagsis consistent with the PR objectives.
66-66: LGTM! Function signature updates are consistent.The parameter type changes in both
transactandtransact_innerfunctions fromSimulationFlagtoExecutionFlagsmaintain API consistency.Also applies to: 72-72
75-82: Verify the flag access method changes.The changes from direct field access to method calls (
account_validation(),fee(),nonce_check()) look good. However, let's verify that these methods are properly implemented in theExecutionFlagstype.✅ Verification successful
The flag access methods are properly implemented, sensei! ✨
The verification shows that
ExecutionFlagsstruct incrates/katana/executor/src/abstraction/mod.rshas all the required fields and their corresponding getter methods:
account_validation()returns theaccount_validationfieldfee()returns thefeefieldnonce_check()returns thenonce_checkfieldThe implementation is clean and matches the usage in the reviewed code perfectly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of ExecutionFlags methods # Test: Search for the ExecutionFlags implementation ast-grep --pattern 'impl ExecutionFlags { $$$ fn account_validation($$$) -> $$$ $$$ fn fee($$$) -> $$$ $$$ fn nonce_check($$$) -> $$$ $$$ }'Length of output: 156
Script:
#!/bin/bash # Let's try a broader search for ExecutionFlags implementations rg -A 10 "impl ExecutionFlags" # Also search for the struct definition to understand its fields rg "struct ExecutionFlags" # Backup: search for these specific method names rg "fn (account_validation|fee|nonce_check)"Length of output: 1393
Script:
#!/bin/bash # Let's see the complete ExecutionFlags struct and implementation rg -B 5 -A 20 "pub struct ExecutionFlags" crates/katana/executor/src/abstraction/mod.rsLength of output: 963
crates/katana/executor/src/abstraction/mod.rs (3)
33-36: Looks good, sensei!The
Defaultimplementation correctly initializes all flags totrue.
62-75: Getter methods are well-implemented, senseiThe getter methods correctly return the values of their respective flags.
116-145: Excellent enhancements toExecutionResult, senseiThe new methods provide clear and convenient ways to create and inspect execution results.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2581 +/- ##
==========================================
+ Coverage 69.48% 69.64% +0.15%
==========================================
Files 401 401
Lines 50817 50759 -58
==========================================
+ Hits 35310 35350 +40
+ Misses 15507 15409 -98 ☔ View full report in Codecov by Sentry. |
also removes some unused flags. the flags were introduced back in #1561 when we were trying to integrate both blockifier and starknet_in_rust. but some of the flags (ie
skip_executeandskip_max_fee) is starknet_in_rust specific. but now that we no longer need to accomodate for starknet-in-rust due to its deprecation #2200, better to just remove it to keep things simple and clean.also i decided to rename the fields and omit the
skip_*prefix as it gets kinda confusing sometimes, esp when we need to determine the execution flag based on the rpc flags as well ie indojo/crates/katana/rpc/rpc/src/starknet/read.rs
Lines 479 to 495 in 17f2564