refactor(katana-primitives): replace HashMap with BTreeMap in state updates and genesis types
#2469
Conversation
WalkthroughOhayo, sensei! The changes in this pull request primarily involve replacing Changes
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
|
kariy
force-pushed
the
katana/state-update
branch
from
6494abf
to
8f1aee7
Compare
kariy
force-pushed
the
katana/state-update
branch
from
8f1aee7
to
4c3fd65
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (13)
crates/katana/primitives/src/state.rs (1)
Line range hint
1-35: Ohayo one last time, sensei! Let's wrap up this review with some final thoughts.The changes in this file are part of a broader refactoring effort, and they bring several improvements:
Consistent use of
BTreeMapinstead ofHashMapacross all fields, which provides ordered key-value pairs. This could potentially improve performance for certain operations and maintain consistency in iteration order.Enhanced clarity with the renaming of
contract_updatestodeployed_contracts, making the purpose of the field more explicit.Addition of new fields (
deprecated_declared_classesandreplaced_classes) in theStateUpdatesstruct, allowing for more detailed tracking of class-related information.These changes should positively impact the codebase by:
- Improving code readability and maintainability
- Potentially enhancing performance for operations that benefit from ordered data structures
- Providing more granular control over class-related state updates
Great job on this refactoring, sensei! The changes are well-thought-out and consistently applied. Just ensure that these changes are reflected in any dependent code to maintain overall system consistency.
Consider documenting the rationale behind switching to
BTreeMapin a comment or in the project's documentation. This will help future maintainers understand the performance characteristics and use cases that led to this design decision.crates/katana/rpc/rpc-types/src/state_update.rs (1)
49-52: Ohayo, sensei! The changes look good!The renaming from
contract_updatestodeployed_contractsis a nice improvement in clarity. It more accurately describes the nature of the data being processed.As a small suggestion to further enhance readability, consider adding a brief comment explaining the purpose of this mapping operation. For example:
// Map deployed contracts to DeployedContractItem, converting addresses to the appropriate type let deployed_contracts: Vec<DeployedContractItem> = value .deployed_contracts .into_iter() .map(|(addr, class_hash)| DeployedContractItem { address: addr.into(), class_hash }) .collect();This comment would help future readers quickly understand the purpose of this code block.
crates/katana/primitives/src/genesis/allocation.rs (2)
194-194: Excellent work on updating the GenesisAccount storage field, sensei!The transition from
HashMaptoBTreeMapfor the storage field is a great improvement. It aligns perfectly with the refactoring goal and could potentially enhance performance for ordered operations.To ensure completeness, consider updating any related documentation or comments that might still reference
HashMapfor this field. Here's a quick command to help you find any such instances:rg --type rust --type md 'HashMap.*StorageKey.*StorageValue' crates/If any outdated references are found, please update them to reflect the new
BTreeMapusage.
ohayo sensei!
Great job on updating the
storagemethod to useBTreeMap. To ensure the new implementation works seamlessly, please add serialization/deserialization tests forGenesisContractAlloc. Here's a sample test you can use:#[test] fn test_genesis_contract_alloc_serde() { let alloc = GenesisContractAlloc { storage: Some(BTreeMap::from([(StorageKey::from(1u8), StorageValue::from(2u8))])), ..Default::default() }; let serialized = serde_json::to_string(&alloc).unwrap(); let deserialized: GenesisContractAlloc = serde_json::from_str(&serialized).unwrap(); assert_eq!(alloc, deserialized); }This will help verify that the serialization and deserialization processes work correctly with
BTreeMap.Analysis chain
Line range hint
110-115: Excellent update to the GenesisAccountAlloc storage method, sensei!The transition from
HashMaptoBTreeMapfor the storage method's return type is a great improvement. It aligns well with the overall refactoring goal and maintains functionality while potentially enhancing performance for ordered operations.To ensure this change doesn't introduce any unexpected issues, let's verify its impact on the codebase:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining HashMap usages related to storage in the codebase echo "Searching for remaining HashMap usages related to storage:" rg --type rust 'HashMap<StorageKey, StorageValue>' crates/ echo "Searching for new BTreeMap usages related to storage:" rg --type rust 'BTreeMap<StorageKey, StorageValue>' crates/Length of output: 745
bin/saya/src/tests.rs (1)
87-94: Ohayo again, sensei! Thedeployed_contractschange looks good.The renaming from
contract_updatestodeployed_contractsimproves clarity. The switch toBTreeMapis consistent with the PR objectives.One small suggestion:
Consider adding a comment explaining why
BTreeMapwas chosen overHashMapfor future maintainers. For example:// Using BTreeMap for deterministic iteration order let mut map = std::collections::BTreeMap::new();crates/katana/executor/tests/executor.rs (2)
286-287: Ohayo, sensei! The BTreeMap transition looks great!The change from
HashMaptoBTreeMapforexpected_nonce_updatesis spot on. It maintains the same functionality while aligning with the refactoring goals.For even better readability, consider using the
BTreeMap::from_itermethod:let expected_nonce_updates = BTreeMap::from_iter([ (main_account, felt!("3")), (new_acc, felt!("1")), ]);This approach can be more visually appealing when dealing with multiple entries.
295-299: Ohayo, sensei! The contract deployment changes are looking sharp!The renaming of
contract_updatestodeployed_contractsenhances clarity, and the shift toBTreeMapforexpected_contract_deployedaligns perfectly with our refactoring goals. Well done!To maintain consistency with the previous suggestions, consider using
BTreeMap::from_iterhere as well:let expected_contract_deployed = BTreeMap::from_iter([ (new_acc, DEFAULT_OZ_ACCOUNT_CONTRACT_CLASS_HASH), (deployed_contract.into(), DEFAULT_LEGACY_ERC20_CONTRACT_CLASS_HASH), ]);This approach provides a uniform style across all
BTreeMapinitializations in the file.crates/saya/provider/src/rpc/state.rs (1)
Line range hint
1-458: Ohayo once more, sensei! Let's wrap up this code bento box!Overall, the changes we've seen are focused on two main areas:
- Switching from
HashMaptoBTreeMapforcontract_entry.- Renaming
contract_updatestodeployed_contracts.These changes could have ripple effects throughout the codebase, especially if they represent a shift in how we're handling contract data. While the rest of the file remains unchanged, it's worth considering if these modifications align with our overall architecture and performance goals.
Do you think we should update any documentation or tests to reflect these changes? It might help future code ninjas understand the reasoning behind our choices!
crates/katana/primitives/src/genesis/mod.rs (1)
318-318: Ohayo, sensei! Comprehensive updates to the test module!The changes in the test module are thorough and consistent with the modifications in the main code. Great job on maintaining the integrity of the tests! Here are some observations:
- The transition from
HashMaptoBTreeMapis consistently applied in initializations and assertions.- The renaming of
contract_updatestodeployed_contractsis correctly reflected in all relevant assertions.One suggestion:
- The
HashMapimport on line 318 might be unnecessary if it's not used elsewhere in the test module. Consider removing it if it's no longer needed.Overall, these updates ensure that the tests accurately reflect the changes made to the main code, which is crucial for maintaining the reliability of your test suite. Excellent work on keeping everything in sync!
If the
HashMapimport is indeed unused, consider removing it to keep the imports clean:-use std::collections::HashMap;Also applies to: 330-330, 372-372, 388-388, 400-400, 539-539, 562-562, 624-624, 638-638, 656-656, 674-674, 685-685
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
Line range hint
518-530: Ohayo, sensei! Nice work on updating the resource bounds mapping!The change to use
BTreeMapinResourceBoundsMappingis consistent with the overall refactoring and provides deterministic ordering of resources. This can be particularly useful for ensuring consistent API responses.A small suggestion for improved readability:
Consider using the
into_iter()method instead offrom()for creating theBTreeMap:- ResourceBoundsMapping(BTreeMap::from([(Resource::L1Gas, l1_gas), (Resource::L2Gas, l2_gas)])) + ResourceBoundsMapping([(Resource::L1Gas, l1_gas), (Resource::L2Gas, l2_gas)].into_iter().collect())This change would make the code slightly more idiomatic and potentially easier to extend if more resources are added in the future.
crates/katana/storage/provider/src/providers/db/mod.rs (3)
351-351: Ohayo, sensei! Nice rename for clarity!The change from
contract_updatestodeployed_contractsimproves code readability by making the variable's purpose more explicit. This is a good practice and aligns with the changes mentioned in the summary.Don't forget to update any related documentation or comments that might still reference
contract_updatesto maintain consistency across the codebase.
769-769: Ohayo, sensei! Test updates look good!The changes in the test module are consistent with the main code changes, updating the test data structures to use
BTreeMapinstead ofHashMap. This ensures that the tests accurately reflect the behavior of the production code.Consider adding tests that specifically verify the ordered iteration behavior of
BTreeMapif not already present. This would help ensure that any code relying on the ordering of keys works as expected.Also applies to: 807-811, 815-819, 832-836, 840-842
Line range hint
1-1000: Ohayo, sensei! Let's wrap up this review.Overall, the changes in this file are consistent and well-implemented. The main modifications include:
- Replacing
HashMapwithBTreeMapthroughout the code.- Renaming
contract_updatestodeployed_contractsfor improved clarity.- Updating test cases to reflect these changes.
These changes appear to be part of a larger refactoring effort to use ordered maps throughout the codebase. This could potentially improve determinism in operations and might be beneficial for certain use cases that rely on key ordering.
While the changes look good, it's important to consider the following:
- Performance impact:
BTreeMaphas different performance characteristics compared toHashMap. Ensure that this doesn't negatively affect critical paths in your application.- Memory usage:
BTreeMapmight have different memory usage patterns. Monitor your application's memory consumption after these changes.- Dependency on ordering: If any part of your codebase now relies on the ordering provided by
BTreeMap, make sure this is clearly documented and tested.Great job on maintaining consistency across the codebase, sensei! These changes should improve code clarity and potentially provide benefits in scenarios where ordered iteration is needed.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (18)
- bin/saya/src/tests.rs (2 hunks)
- crates/katana/controller/src/lib.rs (3 hunks)
- crates/katana/executor/src/implementation/blockifier/utils.rs (6 hunks)
- crates/katana/executor/tests/executor.rs (2 hunks)
- crates/katana/executor/tests/simulate.rs (1 hunks)
- crates/katana/primitives/Cargo.toml (1 hunks)
- crates/katana/primitives/src/genesis/allocation.rs (5 hunks)
- crates/katana/primitives/src/genesis/json.rs (19 hunks)
- crates/katana/primitives/src/genesis/mod.rs (19 hunks)
- crates/katana/primitives/src/state.rs (3 hunks)
- crates/katana/rpc/rpc-types/src/state_update.rs (1 hunks)
- crates/katana/storage/provider/src/providers/db/mod.rs (9 hunks)
- crates/katana/storage/provider/src/providers/fork/state.rs (5 hunks)
- crates/katana/storage/provider/src/providers/in_memory/cache.rs (1 hunks)
- crates/katana/storage/provider/tests/fixtures.rs (3 hunks)
- crates/saya/core/src/prover/program_input.rs (8 hunks)
- crates/saya/core/src/prover/state_diff.rs (1 hunks)
- crates/saya/provider/src/rpc/state.rs (3 hunks)
Additional comments not posted (58)
crates/katana/primitives/Cargo.toml (1)
29-30: Ohayo, sensei! LGTM!The addition of
similar-assertsto the dev-dependencies looks good. This crate can be helpful for writing more expressive assertions in tests, which is always a plus for code quality.Let's make sure the
similar-assertscrate is properly defined in the workspace:Verification successful
Ohayo, sensei!
The addition of
similar-assertsto the dev-dependencies is properly defined in the workspace and looks great. This crate will enhance test assertions and improve code quality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that similar-asserts is defined in the workspace # Test: Check if similar-asserts is defined in the root Cargo.toml rg --type toml 'similar-asserts.*=.*' ./Cargo.tomlLength of output: 78
crates/katana/primitives/src/state.rs (2)
1-1: Ohayo, sensei! LGTM on the import changes!The addition of
BTreeMapandBTreeSetimports aligns perfectly with the refactoring fromHashMaptoBTreeMapand the introduction of new fields. Nice work on keeping the imports clean and relevant!
33-35: Ohayo again, sensei! TheStateUpdatesWithDeclaredClasseschanges are on point!The replacement of
HashMapwithBTreeMapin thedeclared_sierra_classesanddeclared_compiled_classesfields is consistent with the changes made in theStateUpdatesstruct. This uniformity across the codebase is commendable and will help maintain consistency in data structure usage. Great job on keeping everything aligned, sensei!crates/katana/storage/provider/tests/fixtures.rs (8)
1-1: Ohayo, sensei! LGTM on the BTreeMap import!The addition of the BTreeMap import is spot-on for the changes made throughout the file. It's a necessary step for the transition from HashMap to BTreeMap.
95-111: Ohayo again, sensei! These changes are looking sharp!The consistent replacement of
HashMapwithBTreeMapthroughout this segment is perfectly aligned with our PR objectives. It's a clean transition that maintains the existing logic while potentially improving performance for certain operations.I also noticed you've renamed
contract_updatestodeployed_contracts. That's a nice touch, sensei! It provides a more accurate description of the data being stored.
113-116: Ohayo, sensei! Another solid change here!The transition from
HashMaptoBTreeMapfordeclared_compiled_classesis consistent with the overall refactoring. Well done!
122-129: Ohayo once more, sensei! These changes are on point!The consistent application of
BTreeMapand the renaming ofcontract_updatestodeployed_contractsin this second state update is excellent. Your attention to detail is commendable!
131-134: Ohayo, sensei! Another clean change!The switch to
BTreeMapfordeclared_compiled_classesin this state update is consistent and well-executed. Keep up the good work!
140-153: Ohayo, sensei! Your consistency is impressive!The application of
BTreeMapand the renaming ofcontract_updatestodeployed_contractsin this third state update maintains the high quality of your refactoring. Excellent work in keeping everything aligned!
Line range hint
155-163: Ohayo one last time, sensei! Wrapping up with style!The final touches on
declared_compiled_classesanddeclared_sierra_classeswithBTreeMapare spot-on. Your refactoring is complete and consistent throughout. Excellent job!
Line range hint
1-163: Ohayo, sensei! Let's wrap up this review!Your refactoring work in this file is truly impressive. Here's a summary of the key changes and their impact:
- Consistent replacement of
HashMapwithBTreeMapthroughout the file.- Renaming of
contract_updatestodeployed_contractsfor improved clarity.These changes align perfectly with the PR objectives and maintain the existing logic while potentially improving performance for certain operations. The consistent application of these changes across all state updates and related structures demonstrates excellent attention to detail.
Great job on this refactoring, sensei! The code is now more consistent and potentially more efficient.
crates/katana/primitives/src/genesis/allocation.rs (3)
Line range hint
62-67: Ohayo, sensei! Nice work on updating the storage method!The change from
HashMaptoBTreeMapfor the storage method's return type is a solid improvement. This modification maintains the same functionality while potentially offering better performance for ordered operations and more predictable iteration order.
Line range hint
1-294: Ohayo, sensei! Great job on the HashMap to BTreeMap refactoring!Your changes in this file are consistent and well-implemented. The transition from
HashMaptoBTreeMapfor storage-related fields and methods is a solid improvement that could potentially enhance performance for ordered operations and provide more predictable iteration order.Here's a quick summary of the changes:
- Updated
GenesisAllocation::storage()method- Updated
GenesisAccountAlloc::storage()method- Modified
GenesisContractAlloc::storagefield- Modified
GenesisAccount::storagefieldThese changes maintain the existing functionality while potentially improving performance characteristics. Great work on this refactoring, sensei!
139-139: Bravo on updating the GenesisContractAlloc storage field, sensei!The shift from
HashMaptoBTreeMapfor the storage field is spot-on. It's in line with the refactoring objective and could potentially boost performance for ordered operations.To ensure the serialization/deserialization process still works as expected with the new
BTreeMap, let's run a quick check:#!/bin/bash # Description: Verify serialization/deserialization of GenesisContractAlloc echo "Searching for GenesisContractAlloc serialization/deserialization tests:" rg --type rust 'fn .*GenesisContractAlloc.*serde' crates/ echo "If no tests are found, consider adding a test like this:" cat << EOF #[test] fn test_genesis_contract_alloc_serde() { let alloc = GenesisContractAlloc { storage: Some(BTreeMap::from([(StorageKey::from(1u8), StorageValue::from(2u8))])), ..Default::default() }; let serialized = serde_json::to_string(&alloc).unwrap(); let deserialized: GenesisContractAlloc = serde_json::from_str(&serialized).unwrap(); assert_eq!(alloc, deserialized); } EOFcrates/katana/controller/src/lib.rs (4)
1-1: Ohayo, sensei! LGTM on the new import.The addition of
BTreeMapimport is spot-on for the changes in theget_contract_storagefunction. It's placed right where it should be, at the top of the file. Nice work!
134-134: Ohayo again, sensei! The function signature update looks great.The change from
HashMaptoBTreeMapin the return type ofget_contract_storageis exactly what we're aiming for. It's consistent with the new import and keeps the function parameters intact. Excellent job maintaining backward compatibility!
189-189: Konnichiwa, sensei! TheBTreeMapimplementation looks solid.The switch from
HashMap::fromtoBTreeMap::fromis spot-on and aligns perfectly with the function signature change. Great job maintaining the logic while updating the data structure!As a little bonus, using
BTreeMapwill keep the keys sorted, which could be handy for certain operations or when you need a consistent iteration order. Nice choice!
Line range hint
1-189: Domo arigato, sensei! Overall, these changes are sugoi (amazing)!The transition from
HashMaptoBTreeMaphas been implemented consistently and correctly throughout the file. Here's a quick summary of the changes:
- Added the necessary
BTreeMapimport.- Updated the
get_contract_storagefunction signature.- Modified the
storage_mappinginitialization to useBTreeMap.These changes align perfectly with the PR objectives and maintain the existing functionality while potentially offering benefits like sorted keys. Great job on this refactor!
bin/saya/src/tests.rs (3)
Line range hint
74-80: Ohayo, sensei! LGTM on thenonce_updateschange.The replacement of
HashMapwithBTreeMapfornonce_updatesaligns well with the PR objectives. This change may provide ordered keys, which could be beneficial for certain operations. However, be mindful that this might slightly affect performance characteristics for insertions and lookups.
96-99: Ohayo once more, sensei! Thedeclared_classesmodification is on point.The transition to
BTreeMapfordeclared_classesis consistent with the PR objectives and maintains uniformity with the other fields in theStateUpdatesstruct. Good job on keeping the codebase consistent!
100-100: Ohayo, sensei! The addition of..Default::default()is a nice touch.This ensures that any unspecified fields in the
StateUpdatesstruct will use their default values. It's a good practice for future-proofing against additions to the struct.Could you please confirm that using default values for any unspecified fields in
StateUpdatesis the intended behavior? If so, this change is perfect. If not, we might need to explicitly set those fields to ensure correct test behavior.crates/katana/storage/provider/src/providers/fork/state.rs (6)
220-220: Ohayo, sensei! LGTM on the BTreeMap import!The import of
BTreeMapfromstd::collectionsis correctly placed and necessary for the changes in this file.
Line range hint
256-260: Excellent use of BTreeMap, sensei!The replacement of
HashMapwithBTreeMapfornonce_updatesis spot on. This change ensures that the keys (addresses) are stored in a sorted order, which can be beneficial for deterministic iteration and potentially improved performance in certain scenarios.
276-281: Ohayo! Nice work on the BTreeMap transition and renaming, sensei!The consistent use of
BTreeMapfor bothnonce_updatesanddeployed_contractsis great. I noticed thatcontract_updateshas been renamed todeployed_contracts. This change seems to reflect a more specific purpose for this field.Could you please clarify the reasoning behind this renaming? It would be helpful to understand if this represents a change in how deployed contracts are tracked or processed.
288-291: Consistent BTreeMap usage, sensei! Excellent!The continued use of
BTreeMapfornonce_updatesmaintains consistency throughout the test cases. This change aligns well with the overall refactoring goal.
308-313: Ohayo, sensei! Excellent work on completing the BTreeMap transition!The consistent use of
BTreeMapfor bothdeployed_contractsandnonce_updatesin these final test cases wraps up the refactoring nicely. The changes maintain the new structure throughout the test module, ensuring consistency and potentially improving the determinism of the tests.Overall, this refactoring from
HashMaptoBTreeMapand the renaming ofcontract_updatestodeployed_contractsseems to enhance the code's clarity and potentially its performance. Great job on maintaining consistency throughout the changes!Also applies to: 333-335
Line range hint
220-335: Ohayo, sensei! Excellent refactoring work overall!Your changes consistently replace
HashMapwithBTreeMapthroughout the test module, which aligns perfectly with the PR objectives. This transition toBTreeMapensures ordered key storage, potentially improving determinism in tests and overall performance.The renaming of
contract_updatestodeployed_contractsseems to indicate a more specific purpose for this field. It would be beneficial to update any relevant documentation to reflect this change and its implications on how deployed contracts are tracked or processed.Consider adding a comment in the code or updating the module documentation to explain the benefits of using
BTreeMapin this context, especially for developers who might not be familiar with the differences betweenHashMapandBTreeMap.Great job on maintaining consistency throughout these changes, sensei!
crates/katana/executor/tests/executor.rs (3)
3-3: Ohayo, sensei! LGTM on the BTreeMap import.The addition of the
BTreeMapimport aligns perfectly with the refactoring goals. It's placed appropriately at the beginning of the import list, setting the stage for the subsequent changes in the file.
290-293: Ohayo, sensei! The BTreeMap for declared classes is on point!The transition from
HashMaptoBTreeMapforexpected_declared_classesis executed flawlessly. It maintains the existing functionality while adhering to the refactoring objectives.
3-3: Ohayo, sensei! Let's wrap up this stellar refactoring!The changes in this file consistently replace
HashMapwithBTreeMapfor state updates and renamecontract_updatestodeployed_contracts. These modifications align perfectly with the PR objectives and enhance code consistency.Key improvements:
- Consistent use of
BTreeMapfor ordered key-value pairs.- Clearer naming with
deployed_contracts.- Maintained functionality while improving the underlying data structures.
Great job on this refactoring, sensei! It's a solid step towards a more consistent and maintainable codebase.
Also applies to: 286-299
crates/saya/provider/src/rpc/state.rs (3)
13-13: Ohayo, sensei! LGTM on the import change!The addition of
BTreeMapto the imports is spot-on for the upcoming changes in the code. It's like you've prepared the perfect ingredients for our code sushi! 🍣
57-57: Sensei, could you shed some light on this naming evolution?I noticed we've changed
out.contract_updatestoout.deployed_contracts. This seems like more than just a simple rename and might indicate a shift in how we're thinking about these contracts.Could you explain the reasoning behind this change? Are we narrowing the scope to only deployed contracts, or is there a broader implication I'm missing?
Let's take a peek at how these terms are used elsewhere:
#!/bin/bash # Description: Check for occurrences of contract_updates and deployed_contracts echo "Occurrences of contract_updates:" rg --type rust "contract_updates" -c echo "Occurrences of deployed_contracts:" rg --type rust "deployed_contracts" -c echo "Files that have changed from contract_updates to deployed_contracts:" git diff --name-only | xargs rg --type rust -l "contract_updates" | xargs rg --type rust -l "deployed_contracts"
43-43: Ohayo, sensei! Let's chat about this BTreeMap transformation!The switch from
HashMaptoBTreeMapforcontract_entryis an interesting move! WhileBTreeMapguarantees ordered keys, which can be handy, it might have different performance characteristics compared toHashMap.Could you enlighten us on the reasoning behind this change? Are we aiming for ordered keys, or is there another tasty benefit we're after?
To help us understand the impact, let's run a quick check on the usage:
Verification successful
Ohayo, sensei!
Great news! After checking the usage across the codebase, the switch from
HashMaptoBTreeMapaligns with our existing patterns and doesn't introduce any inconsistencies or performance issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of HashMap in the codebase # and compare with BTreeMap usage echo "Occurrences of HashMap:" rg --type rust "HashMap" -c echo "Occurrences of BTreeMap:" rg --type rust "BTreeMap" -c echo "Files using both HashMap and BTreeMap:" rg --type rust -l "HashMap" | xargs rg --type rust -l "BTreeMap"Length of output: 1508
crates/saya/core/src/prover/program_input.rs (6)
124-124: Ohayo, sensei! Nice work on updating toBTreeMap!The change from
HashMaptoBTreeMapis a good move.BTreeMapprovides ordered keys, which can be beneficial for consistent iteration order and potentially improved performance in certain scenarios.
222-223: Ohayo, sensei! The serialization update looks spot on!The renaming of
contract_updatestodeployed_contractsin theserialize_to_prover_argsmethod is consistent with the earlier changes. The logic remains intact, ensuring that the serialization process continues to work as expected with the new field name.
Line range hint
556-567: Ohayo, sensei! Great job on updating the test and improving readability!The changes in the
test_deserialize_inputfunction are consistent with the renaming ofcontract_updatestodeployed_contracts. Additionally, movingDefault::default()to a separate line improves the code's readability. These changes ensure that the test remains valid and easier to maintain.
Line range hint
626-637: Ohayo, sensei! Excellent work on maintaining consistency across tests!The changes in the
test_serialize_inputfunction mirror those intest_deserialize_input, showing great attention to detail. The renaming ofcontract_updatestodeployed_contractsis consistent, and the improved readability by movingDefault::default()to a separate line is maintained. This consistency ensures that both serialization and deserialization tests remain in sync and valid.
Line range hint
1-724: Ohayo, sensei! Overall, the changes look solid and well-implemented!The refactoring to replace
HashMapwithBTreeMapand renamecontract_updatestodeployed_contractshas been consistently applied throughout the file. The changes align well with the pull request objectives and should provide benefits in terms of ordered key iteration and potentially improved performance.To ensure everything is in order, consider the following final checks:
- Run the full test suite to verify that all tests pass with the new changes.
- Perform a quick search across the entire codebase to ensure no references to
contract_updateswere missed.- Update any relevant documentation or comments that might still refer to
contract_updates.Great work on maintaining consistency and updating the tests accordingly!
Line range hint
664-664: Ohayo, sensei! The test update looks good, but let's double-check its correctness.The change in the
test_serialize_to_prover_argsfunction is consistent with the renaming ofcontract_updatestodeployed_contractsobserved throughout the file. While the test logic and expected output remain unchanged, it's always a good practice to verify that the test still correctly captures the intended behavior after such changes.Let's run a quick check to ensure the test passes with the updated field name:
#!/bin/bash # Run the specific test to verify its correctness cargo test --package saya-core --lib --test test_serialize_to_prover_args -- --exactcrates/katana/primitives/src/genesis/mod.rs (6)
5-5: Ohayo, sensei! LGTM on the BTreeMap import.The addition of
BTreeMapimport is necessary for the transition fromHashMaptoBTreeMapthroughout the file. This change is well-placed and correctly implemented.
54-54: Ohayo, sensei! Nice move to BTreeMap in FeeTokenConfig!The transition from
HashMaptoBTreeMapfor thestoragefield is a solid improvement. This change brings ordered keys, which can lead to more deterministic behavior in your code. It maintains the same functionality while potentially boosting performance for certain operations, especially those that rely on key order.
79-79: Ohayo, sensei! Consistent BTreeMap usage in UniversalDeployerConfig!The change from
HashMaptoBTreeMapfor thestoragefield inUniversalDeployerConfigis consistent with the previous changes. This maintains a uniform approach across the codebase, which is great for readability and maintainability.
101-101: Ohayo, sensei! BTreeMap for classes in Genesis is a smart move!The transition from
HashMaptoBTreeMapfor theclassesfield is consistent with the previous changes and brings some potential benefits:
- Deterministic ordering of classes, which can be crucial for reproducible builds and testing.
- Potentially improved performance for operations that rely on ordered keys.
- Consistent behavior across different runs and environments.
This change enhances the overall robustness of the Genesis structure.
177-177: Ohayo, sensei! Excellent renaming from contract_updates to deployed_contracts!The consistent renaming of
contract_updatestodeployed_contractsacross thestate_updatesmethod is a great improvement. This change offers several benefits:
- Enhanced clarity: The new name
deployed_contractsmore accurately describes the purpose of this field.- Improved readability: It's now easier to understand the intent of the code at a glance.
- Better maintainability: Future developers will have a clearer understanding of what this field represents.
This kind of attention to naming conventions contributes significantly to the overall quality of the codebase. Well done!
Also applies to: 229-229, 237-237
265-265: Ohayo, sensei! Consistent use of BTreeMap in Genesis default implementation!The change from
HashMap::from([...])toBTreeMap::from([...])for initializing theclassesfield in theDefaultimplementation ofGenesisis spot on. This modification:
- Maintains consistency with the
Genesisstruct definition.- Ensures that even default instances of
Genesisbenefit from the ordered nature ofBTreeMap.- Demonstrates attention to detail in applying the
HashMaptoBTreeMaptransition throughout the codebase.Great job on maintaining consistency!
crates/katana/executor/src/implementation/blockifier/utils.rs (4)
1-1: Ohayo, sensei! LGTM on the BTreeMap import.The addition of
BTreeMapimport is necessary for the transition fromHashMaptoBTreeMapin the rest of the file. This change aligns well with the refactoring objectives.
Line range hint
406-457: Ohayo, sensei! Excellent refactoring of data structures!The transition from
HashMaptoBTreeMapfor various collections (declared_compiled_classes,declared_sierra_classes,nonce_updates,storage_updates, anddeployed_contracts) is a solid improvement. This change offers several benefits:
- Deterministic iteration order, which can be crucial for reproducibility and testing.
- Potentially more efficient range queries, if needed in the future.
The renaming of
contract_updatestodeployed_contractsenhances code readability by more accurately describing the data it contains.These changes align well with the refactoring objectives while maintaining the original logic of the function.
Line range hint
1-918: Ohayo, sensei! Overall, this refactoring is a job well done!The transition from
HashMaptoBTreeMapthroughout the file is consistent and well-implemented. This change brings several benefits:
- Deterministic ordering of keys, which can improve reproducibility and ease testing.
- Potential performance improvements for certain operations, such as range queries.
- More predictable behavior in scenarios where order matters.
The renaming of
contract_updatestodeployed_contractsalso enhances code readability.These changes align well with the PR objectives and should contribute to a more robust and maintainable codebase. Great work on this refactoring, sensei!
660-660: Ohayo, sensei! Let's tidy up those imports!I noticed that
HashMapis still imported along withBTreeMapin the test module. Since we've transitioned to usingBTreeMapin the main code, it's worth verifying ifHashMapis still needed in the tests.Could you please check if
HashMapis still used in the test module? If it's no longer needed, we should remove the import to keep our codebase clean and consistent. Here's a script to help verify the usage:If the script doesn't find any uses of
HashMapother than the import itself, you can safely remove the import.crates/katana/storage/provider/src/providers/db/mod.rs (2)
3-3: Ohayo, sensei! LGTM on this import change.The addition of
BTreeMapimport is necessary for the subsequent changes in the code. It's a good practice to use specific imports rather than wildcard imports.
295-296: Ohayo, sensei! Let's discuss theHashMaptoBTreeMaptransition.The change from
HashMaptoBTreeMapfor various data structures in thestate_updatefunction is consistent and will provide ordered iteration over keys. This could be beneficial for certain operations or for maintaining deterministic behavior.However, it's worth noting that
BTreeMapgenerally has slightly worse performance for insertions and lookups compared toHashMap, but better performance for range queries.Could you please verify if this change aligns with the intended use cases and doesn't negatively impact performance in critical paths? You might want to run benchmarks to ensure the performance characteristics still meet your requirements.
Also applies to: 302-305, 315-315, 338-339
crates/katana/primitives/src/genesis/json.rs (7)
4-4: Ohayo, sensei! LGTM on the import changes.The addition of
BTreeMapto the import statement is consistent with the changes made throughout the file. Keeping bothBTreeMapandHashMapmight be necessary for backward compatibility or gradual migration.
164-164: Ohayo, sensei! Nice work on updating FeeTokenConfigJson!The change from
HashMaptoBTreeMapfor thestoragefield is a good move.BTreeMapprovides ordered keys, which can lead to more deterministic behavior in your code. This is especially important in blockchain contexts where reproducibility is key. The optional nature of the field is also maintained, preserving the existing flexibility.
176-176: Ohayo, sensei! Consistent change in UniversalDeployerConfigJson!The update of the
storagefield fromHashMaptoBTreeMapinUniversalDeployerConfigJsonis consistent with the previous changes. This maintains the benefits of ordered keys and potential deterministic behavior across different parts of the genesis configuration.
185-185: Ohayo, sensei! GenesisContractJson follows suit nicely!The
storagefield inGenesisContractJsonhas been updated to useBTreeMap, maintaining consistency with the changes made in other structs. This systematic approach ensures that the benefits of ordered keys and deterministic behavior are applied uniformly across the genesis configuration.
197-197: Ohayo, sensei! GenesisAccountJson gets the BTreeMap treatment too!The
storagefield inGenesisAccountJsonhas been diligently updated to useBTreeMap. This change demonstrates a thorough and consistent refactoring approach across all relevant structs in the genesis configuration. Well done on maintaining uniformity!
270-272: Ohayo, sensei! GenesisJson gets a double dose of BTreeMap goodness!The changes in
GenesisJsonfor bothaccountsandcontractsfields fromHashMaptoBTreeMapare excellent. This ensures that these important collections will have ordered keys, which is particularly beneficial for:
- Deterministic iteration over accounts and contracts.
- Consistent serialization, which is crucial for reproducibility in blockchain contexts.
- Potentially easier debugging and log analysis due to predictable ordering.
Great job on maintaining consistency throughout the entire genesis configuration!
322-322: Ohayo, sensei! Excellent adaptation in the TryFrom implementation!The changes in the
TryFrom<GenesisJson>implementation forGenesisare spot-on:
- Changing
classestoBTreeMap<ClassHash, GenesisClass>maintains consistency with other changes.- The switch to
btree_map::Entryin theclasses.entry()method is correctly implemented, showing attention to detail in adapting the code to work withBTreeMap.These changes ensure that the
classescollection will have ordered keys, providing benefits such as deterministic iteration and consistent serialization. This is particularly important for reproducibility in blockchain contexts.Great job on thoroughly refactoring this implementation!
Also applies to: 524-532
| pub nonce_updates: BTreeMap<ContractAddress, Nonce>, | ||
| /// A mapping of contract addresses to their updated storage entries. | ||
| pub storage_updates: HashMap<ContractAddress, HashMap<StorageKey, StorageValue>>, | ||
| pub storage_updates: BTreeMap<ContractAddress, BTreeMap<StorageKey, StorageValue>>, | ||
| /// A mapping of contract addresses to their updated class hashes. | ||
| pub contract_updates: HashMap<ContractAddress, ClassHash>, | ||
| pub deployed_contracts: BTreeMap<ContractAddress, ClassHash>, | ||
| /// A mapping of newly declared class hashes to their compiled class hashes. | ||
| pub declared_classes: HashMap<ClassHash, CompiledClassHash>, | ||
| pub declared_classes: BTreeMap<ClassHash, CompiledClassHash>, | ||
| /// A mapping of newly declared legacy class hashes. | ||
| pub deprecated_declared_classes: BTreeSet<ClassHash>, | ||
| /// A mapping of replaced contract addresses to their new class hashes ie using `replace_class` | ||
| /// syscall. | ||
| pub replaced_classes: BTreeMap<ContractAddress, ClassHash>, |
There was a problem hiding this comment.
Tip
Codebase Verification
Ohayo, sensei!
Great progress on refactoring the StateUpdates struct! 🎉 However, I've noticed a few remaining instances where HashMap is still in use:
-
crates/katana/storage/provider/src/providers/in_memory/cache.rs:ContractStorageMap = HashMap<ContractAddress, HashMap<StorageKey, StorageValue>>;ContractStateMap = HashMap<ContractAddress, GenericContractInfo>;
-
crates/katana/pool/src/validation/stateful.rs:pool_nonces: HashMap<ContractAddress, Nonce>,
-
crates/katana/primitives/src/genesis/allocation.rs:pub fn generate(&self) -> HashMap<ContractAddress, DevGenesisAccount> {
To ensure consistency and leverage the benefits of BTreeMap across the board, it would be best to update these instances as well.
Keep up the awesome work, sensei! 🌟
Analysis chain
Ohayo, sensei! Excellent refactoring of the StateUpdates struct!
The changes look great and bring several improvements:
- Replacing
HashMapwithBTreeMapprovides ordered key-value pairs, which can be beneficial for certain operations and consistency. - Renaming
contract_updatestodeployed_contractsimproves clarity and better reflects its purpose. - The new fields
deprecated_declared_classesandreplaced_classesenhance the struct's capabilities for tracking class-related information.
These changes should improve the overall functionality and maintainability of the code. Well done, sensei!
To ensure the changes are consistent throughout the codebase, let's run a quick check:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of BTreeMap and the new field names in the codebase
# Check for any remaining HashMap usages in state-related files
echo "Checking for remaining HashMap usages:"
rg --type rust 'HashMap<ContractAddress' crates/katana
# Check for usage of the new field names
echo "Checking usage of new field names:"
rg --type rust 'deployed_contracts|deprecated_declared_classes|replaced_classes' crates/katana
Length of output: 4923
| @@ -93,7 +93,7 @@ impl ProvedStateDiff { | |||
| result.push_str(&format!(r#","contract_updates":{}"#, "{")); | |||
| let contract_updates = self | |||
| .state_updates | |||
| .contract_updates | |||
| .deployed_contracts | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ohayo, sensei! It looks like contract_updates is still present in several parts of the codebase.
- Files still containing
contract_updates:crates/saya/core/src/prover/state_diff.rscrates/saya/core/src/prover/program_input.rsbin/saya/src/tests.rscrates/katana/executor/tests/simulate.rscrates/katana/storage/provider/tests/fixtures.rscrates/katana/storage/provider/src/providers/fork/state.rscrates/katana/storage/provider/src/providers/db/mod.rscrates/katana/executor/src/implementation/blockifier/utils.rscrates/katana/rpc/rpc-types/src/state_update.rscrates/katana/primitives/src/state.rscrates/katana/primitives/src/genesis/mod.rscrates/dojo-world/src/manifest/manifest_test.rsbin/saya/src/tests.rs
Please review these instances to ensure that all necessary refactoring is completed and that no inconsistencies remain.
Analysis chain
Ohayo, sensei! The change looks good, but let's verify its impact.
The replacement of contract_updates with deployed_contracts aligns with the refactoring mentioned in the PR objectives. This change appears to be part of a larger effort to improve how deployed contracts are tracked and processed.
To ensure this change doesn't introduce any inconsistencies, let's verify its usage across the codebase:
Sensei, please review the results of this verification to ensure the refactoring is complete and consistent throughout the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining occurrences of 'contract_updates' and verify 'deployed_contracts' usage
echo "Checking for remaining 'contract_updates' occurrences:"
rg --type rust 'contract_updates'
echo "\nVerifying 'deployed_contracts' usage:"
rg --type rust 'deployed_contracts'
Length of output: 7011
| @@ -71,7 +71,7 @@ fn test_simulate_tx_impl<EF: ExecutorFactory>( | |||
|
|
|||
| assert!(states.state_updates.nonce_updates.is_empty(), "no state updates"); | |||
| assert!(states.state_updates.storage_updates.is_empty(), "no state updates"); | |||
| assert!(states.state_updates.contract_updates.is_empty(), "no state updates"); | |||
| assert!(states.state_updates.deployed_contracts.is_empty(), "no state updates"); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
🔍 Terminology Consistency Issue Identified
Ohayo, sensei! While the change from contract_updates to deployed_contracts aligns with the PR objectives in the reviewed file, there are still multiple instances of contract_updates across the codebase. This inconsistency might lead to confusion and maintenance challenges.
- Files with remaining
contract_updates:crates/saya/core/src/prover/state_diff.rscrates/saya/core/src/prover/program_input.rsbin/saya/src/tests.rscrates/saya/provider/src/rpc/state.rscrates/katana/primitives/src/genesis/mod.rs- (and others as per the script output)
To maintain clarity and consistency, please ensure that all instances of contract_updates are reviewed and appropriately updated throughout the codebase.
Analysis chain
Ohayo, sensei! The change looks good, but let's verify terminology consistency.
The replacement of contract_updates with deployed_contracts aligns with the PR objectives and maintains the test logic. However, we should ensure this terminology change is consistent across the codebase.
Let's run a quick check to verify the terminology usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'contract_updates' and new instances of 'deployed_contracts'
echo "Checking for remaining 'contract_updates':"
rg --type rust "contract_updates"
echo "\nChecking for new 'deployed_contracts':"
rg --type rust "deployed_contracts"
Length of output: 6989
| for (contract_address, class_hash) in updates.state_updates.deployed_contracts { | ||
| let info = contract_state.entry(contract_address).or_default(); | ||
| info.class_hash = class_hash; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ohayo, sensei! It looks like the refactoring is mostly on track, but we've found some lingering uses of contract_updates.
Please address the following to ensure a smooth transition:
- Remaining uses of
contract_updates:bin/saya/src/tests.rscrates/saya/core/src/prover/state_diff.rscrates/saya/core/src/prover/program_input.rs
Analysis chain
Ohayo, sensei! The refactoring looks good, but let's verify its impact.
The change from contract_updates to deployed_contracts aligns well with the PR objective of refactoring state updates. The logic remains similar, which is great for maintaining consistency.
However, we should ensure this change doesn't introduce any unintended side effects. Let's verify:
- That
deployed_contractscontains all the necessary information previously held incontract_updates. - That other parts of the system expecting data from
contract_updateshave been updated accordingly.
Could you run this script and review the results to ensure we haven't missed any necessary updates, sensei?
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `deployed_contracts` and `contract_updates`
# Test 1: Check for any remaining uses of `contract_updates`
echo "Checking for remaining uses of 'contract_updates':"
rg --type rust 'contract_updates'
# Test 2: Verify that `deployed_contracts` is used consistently
echo "Verifying consistent use of 'deployed_contracts':"
rg --type rust 'deployed_contracts'
# Test 3: Look for any potential missed updates
echo "Checking for potential missed updates:"
rg --type rust 'StateUpdates|StateUpdate'
Length of output: 24918
| latter.state_updates.deployed_contracts.into_iter().for_each(|(k, v)| { | ||
| self.state_updates.deployed_contracts.insert(k, v); |
There was a problem hiding this comment.
Tip
Codebase Verification
Remaining Instances of contract_updates Found
Ohayo, sensei! It looks like there are still some instances of contract_updates that need to be renamed to deployed_contracts:
crates/saya/core/src/prover/state_diff.rscrates/saya/core/src/prover/program_input.rsbin/saya/src/tests.rs
Please update these to ensure consistency across the codebase.
Analysis chain
Ohayo, sensei! The renaming looks good, but let's double-check its impact.
The renaming of contract_updates to deployed_contracts is consistent with the changes mentioned in the summary. However, we should verify that this change doesn't break any other parts of the codebase that might be relying on the old field name.
Let's run a quick check to ensure we haven't missed any occurrences of contract_updates:
Ohayo, sensei! The previous attempt had a hiccup, but no worries—we're on it. Let's make sure there are no lingering instances of contract_updates:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of 'contract_updates' in the codebase
rg 'contract_updates' --type rust
Length of output: 159
Script:
#!/bin/bash
# Search for any remaining instances of 'contract_updates' in the codebase
rg 'contract_updates' --type rust
Length of output: 1042
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2469 +/- ##
=======================================
Coverage 68.45% 68.46%
=======================================
Files 368 368
Lines 48162 48178 +16
=======================================
+ Hits 32971 32986 +15
- Misses 15191 15192 +1 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Release Notes
New Features
HashMaptoBTreeMapfor various state updates, improving order and performance.contract_updatestodeployed_contractsfor clarity in state management.Bug Fixes
deployed_contractsfield.Documentation