feat: batch entities #2633
feat: batch entities #2633
Conversation
WalkthroughOhayo, sensei! This pull request includes significant updates across various files, primarily focusing on dependency management in 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: 15
🧹 Outside diff range and nitpick comments (32)
crates/dojo/core-cairo-test/src/tests/event/event.cairo (2)
Line range hint
1-8: Ohayo! The event structure looks solid, sensei!The
FooEventstructure is well-designed with appropriate key fields and type choices. However, consider adding documentation to explain the purpose of each field for better maintainability.Add documentation like this:
#[derive(Drop, Serde)] #[dojo::event] +/// Event emitted when foo operations occur struct FooEvent { + /// Primary key identifier (0-255) #[key] k1: u8, + /// Secondary key identifier #[key] k2: felt252, + /// First value parameter v1: u128, + /// Second value parameter v2: u32 }
Line range hint
10-16: The test coverage looks good but could be enhanced, sensei!While the basic event definition verification is solid, consider adding more test cases to ensure robust event handling:
- Test serialization/deserialization
- Verify key field handling
- Test with edge case values
Here's a suggested addition:
#[test] fn test_event_serialization() { let event = FooEvent { k1: 255_u8, k2: 'max', v1: 340282366920938463463374607431768211455_u128, v2: 4294967295_u32 }; let serialized = dojo::event::Event::<FooEvent>::serialize(@event); // Add assertions for serialized data }crates/dojo/core-cairo-test/src/lib.cairo (1)
Ohayo sensei!
The commented-out modules actually exist and should be uncommented
The
fdcommand shows that all the commented-out modules (model,benchmarks,entities,resources) have corresponding test files in the repository:
crates/dojo/core-cairo-test/src/tests/benchmarks.cairocrates/dojo/core-cairo-test/src/tests/model/model.cairocrates/dojo/core-cairo-test/src/tests/world/entities.cairocrates/dojo/core-cairo-test/src/tests/world/resources.cairoSince these test modules exist and there's no evidence of their intentional removal (no deletion commits found), they should be uncommented in
lib.cairoto ensure all tests are properly included in the test suite.🔗 Analysis chain
Line range hint
19-21: Verify status of commented-out modulesThere are several commented-out modules (
model,benchmarks,entities,resources). Let's verify if these are intentionally disabled or if they should be addressed as part of this PR.Also applies to: 47-48
Let me gather more information about these commented modules to make a proper assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if these modules exist in the main source code and if they're actively used # Search for corresponding source files echo "Searching for corresponding source files..." fd -e cairo "model|benchmarks|entities|resources" "crates/dojo/core/src" # Check if there are any references to these modules in recent commits echo "Checking recent commit history for these modules..." git log --grep="model\|benchmarks\|entities\|resources" --since="1 month ago" --pretty=format:"%h - %s" --Length of output: 576
Script:
#!/bin/bash # Check the content of the test file to see the commented modules echo "Checking the test file content..." cat "crates/dojo/core-cairo-test/src/lib.cairo" # Check if these modules exist in the test directory echo -e "\nSearching for test modules..." fd -e cairo "model|benchmarks|entities|resources" "crates/dojo/core-cairo-test/src" # Check for any recent removals of these modules echo -e "\nChecking for recent file deletions..." git log --diff-filter=D --summary --since="3 months ago" | grep -E "model|benchmarks|entities|resources"Length of output: 1739
crates/torii/core/src/processors/event_message.rs (2)
74-74: Ohayo! Consider enhancing the TODO comment.The TODO comment could be more descriptive about the expected configuration implementation. This would help future developers understand the planned changes better.
- // TODO: this must come from some torii's configuration. + // TODO: The historical flag should be configurable through Torii's configuration, + // allowing different retention policies for different environments.
75-76: Consider extracting the hardcoded value to a constant, sensei!The hardcoded
falsevalue could be moved to a module-level constant for better maintainability and documentation.pub(crate) const LOG_TARGET: &str = "torii_core::processors::event_message"; +// Default historical flag value until Torii configuration is implemented +pub(crate) const DEFAULT_HISTORICAL_FLAG: bool = false; #[derive(Default, Debug)] pub struct EventMessageProcessor;Then use it in the code:
- let historical = false; + let historical = DEFAULT_HISTORICAL_FLAG; db.set_event_message(entity, event_id, block_timestamp, historical).await?;crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (2)
71-81: Ohayo! Clean implementation for ABI inclusion.The implementation effectively ensures the Event struct is included in the ABI. However, we could make the intention even clearer with a more descriptive variable name.
Consider this minor improvement:
- fn ensure_abi(self: @ContractState, event: $type_name$) { - let _event = event; + fn ensure_abi(self: @ContractState, event: $type_name$) { + let _unused_event = event;
71-81: Ohayo sensei! Good architectural direction.The removal of historical functionality from the event system and its delegation to Torii configuration, combined with this ABI enhancement, creates a cleaner separation of concerns. This aligns well with the PR's objective of optimizing batch operations.
crates/dojo/core/src/world/errors.cairo (1)
Line range hint
1-100: Ohayo sensei! Excellent error handling patterns throughout the file!The error handling implementation demonstrates consistent patterns, clear messaging, and proper use of string formatting. The new
lengths_mismatchfunction integrates seamlessly with the existing error handling approach.Consider documenting these error patterns in a developer guide to maintain consistency as more batch operations are added.
crates/dojo/core-cairo-test/src/tests/world/storage.cairo (3)
5-30: Consider adding edge case tests, sensei!While the basic CRUD operations are well-tested, consider adding test cases for:
- Invalid addresses
- Boundary values for
aandbfields
81-128: Similar improvements needed here too, sensei!The test provides good coverage for non-copiable types, but has the same opportunities for improvement as the previous test:
- Remove redundant if/else branches
- Remove unnecessary comment about desnapping
- Consider extracting magic numbers
Additionally, great job testing both empty array and empty string as default values!
1-128: Ohayo! Overall excellent test coverage, sensei!The tests thoroughly validate the ModelStorage functionality, particularly the new batch operations capability mentioned in the PR objectives. They cover both copiable and non-copiable models, and verify the complete lifecycle of models including writing, reading, and erasing.
While there are some minor opportunities for improvement in code structure, the tests are robust and well-designed.
Consider adding performance tests to measure the efficiency gains from batch operations compared to individual operations, as this was one of the key objectives of the PR.
bin/sozo/tests/test_data/policies.json (1)
47-93: Ohayo sensei! New batch operations align well with PR objectives.The introduction of batch operations (
emit_events,set_entities,delete_entities) aligns perfectly with the PR's goal of reducing permission check overhead. These methods will help optimize performance when dealing with multiple entities simultaneously.Consider implementing rate limiting or size constraints for these batch operations to prevent potential DoS vectors from extremely large batches.
Also applies to: 99-109
crates/torii/types-test/src/contracts.cairo (2)
Line range hint
116-127: Ohayo sensei! The delete implementation needs some love!The current implementation has several potential issues:
- The subrecord ID calculation (
record_id + 1) assumes sequential IDs, which might not always be true- Missing error handling for non-existent records
- No events are emitted for deletion tracking
Consider this improved implementation:
fn delete(ref self: ContractState, record_id: u32) { let mut world = self.world(@"types_test"); + // Verify record exists before proceeding + match world.try_read_model::<Record>(record_id) { + Option::Some(record) => { + let record_sibling: RecordSibling = world.read_model(record_id); + // Read subrecord using the stored ID from the main record + let subrecord: Subrecord = world.read_model((record_id, record.subrecord_id)); - let record: Record = world.read_model(record_id); - let record_sibling: RecordSibling = world.read_model(record_id); + world.erase_model(@record); + world.erase_model(@record_sibling); + world.erase_model(@subrecord); - let subrecord_id = record_id + 1; - let subrecord: Subrecord = world.read_model((record_id, subrecord_id)); + // Emit deletion event + world.emit_event( + @RecordDeleted { record_id } + ); + }, + Option::None => { + panic!("Record not found"); + } + } - world.erase_model(@record); - world.erase_model(@record_sibling); - world.erase_model(@subrecord); }
Line range hint
116-127: Consider implementing batch operations for better performanceOhayo sensei! Since this PR aims to support batching entities and reduce permission check overhead, consider implementing batch operations for model deletions. Instead of individual
erase_modelcalls, you could:
- Collect all models to be deleted in a batch
- Implement a batch deletion operation
- Emit a single batch deletion event
This would align better with the PR's objective of reducing permission check overhead.
Would you like me to help design the batch deletion implementation?
crates/dojo/lang/src/attribute_macros/event.rs (1)
Line range hint
65-82: Robust event validation maintained, excellent work sensei!The strict validation requiring both key and non-key members is crucial for maintaining data integrity in the batch processing context. This helps ensure that events remain properly structured even when processed in batches.
Consider documenting these validation requirements in the module-level documentation to help other developers understand the event structure requirements.
crates/dojo/core-cairo-test/src/tests/helpers.cairo (1)
29-36: Ohayo sensei! The NotCopiable struct looks well-designed!The struct effectively demonstrates handling of non-copyable types in batch operations, which aligns perfectly with the PR's objectives. The choice of Array and ByteArray as fields provides good coverage for testing complex data structures.
Consider adding a comment explaining that this struct serves as a test case for batch operations with non-copyable types, to help other developers understand its purpose in the test suite.
bin/sozo/src/commands/events.rs (1)
Line range hint
350-370: Ohayo! Let's enhance the value printing for better readability, sensei!I noticed a couple of TODOs related to value printing:
- Model value implementation and printing for
StoreUpdateRecord- Pretty printing of values for
StoreUpdateMemberThese improvements would enhance the readability of the event output.
Would you like me to help implement these pretty printing features or create GitHub issues to track them?
crates/dojo/core/src/model/storage.cairo (2)
64-66: Ohayo, sensei! Optimizeread_values_from_idsfor large datasetsWhen retrieving multiple model values using
read_values_from_ids, consider the performance implications with large spans. Optimizing with asynchronous handling or batching may improve efficiency.
114-115: Ohayo, sensei! Enhance error handling inwrite_values_from_ids_testConsider adding error handling for invalid
entity_idsinwrite_values_from_ids_testto improve the robustness of your tests.crates/dojo/core/src/world/iworld.cairo (2)
133-141: Clarify 'emit_events' parameters for better understanding, senseiOhayo! Consider specifying in the documentation that
event_selectorapplies to all batched events inemit_events. This will help future developers understand that the method emits multiple events of the same type.
169-170: Unify terminology: 'indexes' or 'indices', sensei?Ohayo! To maintain consistency, please use either 'indexes' or 'indices' in both the documentation and parameter names for the
entitiesmethod. Currently, the documentation refers toindiceswhile the parameter is namedindexes.Also applies to: 172-173
crates/dojo/core/src/world/storage.cairo (5)
154-167: Ensure consistency inerase_modelsfunctionOhayo, sensei! In the
erase_modelsfunction, verifying that all models are successfully erased would improve reliability. Consider adding error handling to confirm each deletion, allowing for proper handling in cases where some models might not be found or cannot be deleted.
182-201: Check for existence before erasing inerase_models_ptrsOhayo, sensei! When erasing models using pointers, it's prudent to check if each model exists before attempting deletion. This can prevent unnecessary errors and provide clearer feedback on which models were successfully erased.
376-381: Optimizewrite_models_testwith batch processingOhayo, sensei! In
write_models_test, you're looping through models and callingwrite_model_testindividually. To improve efficiency, consider implementing batch processing similar towrite_modelsto reduce overhead during tests.
395-400: Enhanceerase_models_testwith error handlingOhayo, sensei! Incorporating error handling in
erase_models_testwould make testing more robust by ensuring that any issues during model deletion are captured and can be addressed promptly.
418-443: Simplifyerase_models_ptrs_testloopOhayo, sensei! The loop in
erase_models_ptrs_testcould be simplified for better readability by iterating directly overidswithout building an intermediate array.Here's a suggested refactor:
- let mut ids: Array<felt252> = array![]; - for p in ptrs { - ids - .append( - match p { - ModelPtr::Id(id) => *id, - ModelPtr::Keys(keys) => entity_id_from_keys(*keys), - } - ); - }; - - let world_test = dojo::world::IWorldTestDispatcher { - contract_address: self.dispatcher.contract_address - }; - - for i in ids { + let world_test = dojo::world::IWorldTestDispatcher { + contract_address: self.dispatcher.contract_address + }; + for p in ptrs { + let i = match p { + ModelPtr::Id(id) => *id, + ModelPtr::Keys(keys) => entity_id_from_keys(*keys), + }; dojo::world::IWorldTestDispatcherTrait::delete_entity_test( world_test, Model::<M>::selector(self.namespace_hash), ModelIndex::Id(i), Model::<M>::layout() ); }crates/dojo/core/src/world/world_contract.cairo (4)
800-838: Ohayo sensei! Simplify the loop inemit_eventsfunctionTo improve readability and maintain consistency, consider replacing the manual loop with a
forloop.Apply this diff to simplify the loop:
if keys.len() != values.len() { panic_with_byte_array( @errors::lengths_mismatch(@"keys", @"values", @"emit_events") ); } - let mut i = 0; - loop { - if i >= keys.len() { - break; - } - - self - .emit( - EventEmitted { - selector: event_selector, - system_address: get_caller_address(), - keys: *keys[i], - values: *values[i], - } - ); - - i += 1; - } + for i in 0..keys.len() { + self + .emit( + EventEmitted { + selector: event_selector, + system_address: get_caller_address(), + keys: *keys[i], + values: *values[i], + } + ); + }
846-856: Ohayo sensei! Consider preallocating the array inentitiesfunctionTo improve performance, you can initialize the array with the capacity of
indexes.len().Apply this diff to preallocate the array:
fn entities( self: @ContractState, model_selector: felt252, indexes: Span<ModelIndex>, layout: Layout ) -> Span<Span<felt252>> { - let mut models: Array<Span<felt252>> = array![]; + let mut models: Array<Span<felt252>> = Array::with_capacity(indexes.len()); for i in indexes { models.append(self.get_entity_internal(model_selector, *i, layout)); }; models.span() }
875-906: Ohayo sensei! Simplify the loop inset_entitiesfunctionSimilar to the previous suggestion, replacing the manual loop with a
forloop can enhance readability.Apply this diff to simplify the loop:
if indexes.len() != values.len() { panic_with_byte_array( @errors::lengths_mismatch(@"indexes", @"values", @"set_entities") ); } - let mut i = 0; - loop { - if i >= indexes.len() { - break; - } - - self.set_entity_internal(model_selector, *indexes[i], *values[i], layout); - - i += 1; - }; + for i in 0..indexes.len() { + self.set_entity_internal(model_selector, *indexes[i], *values[i], layout); + };
921-939: Ohayo sensei! Simplify the loop indelete_entitiesfunctionUpdating the loop structure can improve code clarity.
Apply this diff to simplify the loop:
if let Resource::Model((_, _)) = self.resources.read(model_selector) { self.assert_caller_permissions(model_selector, Permission::Writer); - for i in indexes { - self.delete_entity_internal(model_selector, *i, layout); - } + for i in indexes { + self.delete_entity_internal(model_selector, *i, layout); + } } else { panic_with_byte_array( @errors::resource_conflict(@format!("{model_selector}"), @"model") ); }crates/dojo/world/src/contracts/abigen/world.rs (2)
3228-3247: Add documentation comments for theentitiesmethodOhayo sensei! The
entitiesmethod lacks Rust doc comments. Adding them would enhance code readability and assist other developers in understanding its purpose and usage.
4079-4098: Consider addingemit_eventsmethod toWorldContractReaderfor consistencyWhile
emit_eventshas been added toWorldContract, adding it toWorldContractReaderwould maintain consistency and could be beneficial for read-only batch operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (25)
Cargo.toml(0 hunks)bin/sozo/src/commands/events.rs(1 hunks)bin/sozo/tests/test_data/policies.json(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/event/event.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(0 hunks)crates/dojo/core/src/event/event.cairo(0 hunks)crates/dojo/core/src/event/interface.cairo(0 hunks)crates/dojo/core/src/lib.cairo(0 hunks)crates/dojo/core/src/model/model.cairo(0 hunks)crates/dojo/core/src/model/model_value.cairo(0 hunks)crates/dojo/core/src/model/storage.cairo(4 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/iworld.cairo(5 hunks)crates/dojo/core/src/world/storage.cairo(12 hunks)crates/dojo/core/src/world/world_contract.cairo(5 hunks)crates/dojo/lang/src/attribute_macros/event.rs(1 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(0 hunks)crates/dojo/world/src/contracts/abigen/world.rs(6 hunks)crates/torii/core/src/processors/event_message.rs(1 hunks)crates/torii/types-test/src/contracts.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
💤 Files with no reviewable changes (8)
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/world/world.cairo
- crates/dojo/core/src/event/event.cairo
- crates/dojo/core/src/event/interface.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (25)
crates/dojo/core-cairo-test/src/lib.cairo (1)
50-50: Ohayo! The storage module addition looks good, sensei!
The new storage module within world aligns perfectly with the PR's objective of implementing batch entities functionality, providing a proper testing structure for the new features.
crates/dojo/core/src/world/errors.cairo (1)
5-7: Ohayo! Clean implementation of the length mismatch error formatter!
The new lengths_mismatch function aligns perfectly with the PR's batch operations feature and follows the established error formatting patterns in the codebase. The error message is clear and provides good context for debugging.
Let's verify its usage in batch operations:
✅ Verification successful
Ohayo! The lengths_mismatch error function is correctly used in batch operations!
The verification shows that the error function is properly integrated into batch operations:
- Used in
set_entitiesto validate that indexes and values arrays have matching lengths - Part of the
delete_entitiescontext, maintaining consistency in batch operations error handling
The implementation serves its intended purpose in the batch operations feature, providing clear error messages when array lengths don't match.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of lengths_mismatch in batch-related functions
rg -l "lengths_mismatch" | xargs rg -A 5 "batch|set_entities|delete_entities"
Length of output: 1620
crates/dojo/core-cairo-test/src/tests/world/storage.cairo (1)
1-3: Ohayo! Clean and focused imports, sensei!
The imports are well-organized and include only the necessary components for the tests.
bin/sozo/tests/test_data/policies.json (3)
3-39: Address consolidation pattern requires verification.
Many methods are now pointing to a centralized target address (0x4bf1624...). While this consolidation might simplify management, it's crucial to ensure proper access controls are in place.
Let's verify the contract at this address:
#!/bin/bash
# Search for contract declarations or implementations
ast-grep --pattern 'contract World {
$$$
}'
# Search for access control implementations
rg -A 5 "access_control" Also applies to: 43-87, 127-127
91-93: Verify emit_events implementation for event ordering.
The new emit_events method should maintain proper event ordering and atomicity.
Let's check the implementation:
✅ Verification successful
Event ordering and atomicity are properly implemented in emit_events, sensei!
The implementation in world_contract.cairo shows proper handling of event ordering and atomicity through:
- Validation of matching lengths between keys and values
- Sequential processing of events in a deterministic loop
- Permission checks before event emission
- Atomic transaction handling via the contract state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for emit_events implementation
ast-grep --pattern 'fn emit_events($$$) {
$$$
}'
Length of output: 231
Script:
#!/bin/bash
# Let's try a different approach to find emit_events implementation
# First, let's use ripgrep to find files containing emit_events
rg -l "emit_events"
# Then let's see the actual implementation with context
rg "emit_events" -A 10
Length of output: 7159
111-124: New permission management methods enhance security model.
The addition of granular permission management methods (grant_owner, revoke_owner, grant_writer, revoke_writer) provides better access control capabilities.
Let's verify the permission hierarchy implementation:
✅ Verification successful
Ohayo sensei! Based on the code analysis, I can now provide a conclusive response:
Permission management implementation is properly integrated
The codebase shows a complete implementation of the permission system with:
- Full grant/revoke functionality for both owners and writers in the World contract
- Integration in the auth module for permission management operations
- Proper test coverage in multiple test files
- Usage across different parts of the system (sozo, torii, dojo)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for owner/writer permission checks in the codebase
rg -A 3 "check_(owner|writer)_permission"
Length of output: 43
Script:
#!/bin/bash
# Let's search for permission-related functions and methods in the codebase
rg -A 5 "(grant|revoke)_(owner|writer)" --type rust
# Also search for any permission-related structs or traits
ast-grep --pattern 'trait $_ {
$$$
fn $_(owner|writer)$_ {
$$$
}
$$$
}'
# Look for permission checks in a different way
rg -A 3 "has_(owner|writer)_permission" --type rust
Length of output: 19853
crates/torii/types-test/src/contracts.cairo (1)
Line range hint 18-26: Ohayo! Nice addition of Serde derive!
The addition of Serde derive to RecordLogged enhances the struct's serialization capabilities while maintaining proper event structure with key fields. This aligns well with the PR's objective of ensuring event structure in the ABI.
crates/dojo/lang/src/attribute_macros/event.rs (2)
13-13: Ohayo! Clean import optimizations look good, sensei!
The streamlined imports align perfectly with the removal of the historical parameters functionality.
Also applies to: 16-16, 20-20
Line range hint 31-144: Verify impact of version removal across the system, sensei!
While the removal of version tracking aligns with the PR objectives, we should verify that this doesn't impact event consumers.
crates/dojo/core-cairo-test/src/tests/helpers.cairo (1)
220-223: The world deployment setup is properly updated, sensei!
The NotCopiable model is correctly registered in the test world setup, enabling comprehensive testing of batch operations with non-copyable types.
Let's verify that the NotCopiable model is used in corresponding test cases:
✅ Verification successful
Verification complete, all is well, sensei!
The NotCopiable model is properly used in test cases, specifically in storage.cairo where it's thoroughly tested in the write_multiple_not_copiable function. The test validates:
- Writing multiple NotCopiable models
- Reading the models back
- Proper model erasure
- Correct handling of non-copyable types in batch operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases using NotCopiable
rg -l "NotCopiable" --type cairo
Length of output: 62
Script:
#!/bin/bash
# Search for NotCopiable in all files
rg -l "NotCopiable"
Length of output: 129
Script:
#!/bin/bash
# Find cairo files and search for NotCopiable
fd -e cairo | xargs rg -l "NotCopiable"
Length of output: 149
Script:
#!/bin/bash
# Find cairo files and show context around NotCopiable usage
fd -e cairo | xargs rg -C 3 "NotCopiable"
Length of output: 5136
bin/sozo/src/commands/events.rs (1)
348-348: Ohayo! The event output format update looks good, sensei!
The removal of the historical field from the event output format aligns perfectly with the PR's objective of removing historical aspects from the chain. The change maintains all essential event information while streamlining the output.
crates/dojo/core/src/model/storage.cairo (6)
37-39: Ohayo, sensei! Ensure consistency in batch delete with erase_models
The erase_models method aligns with the batch processing implementation. Verify that deleting multiple models does not lead to inconsistent states if some deletions fail.
Consider implementing transactional behavior or rollback mechanisms for batch deletions.
56-60: Ohayo, sensei! Consistent generic constraints in read_values
The method read_values correctly extends the functionality for batch reads. Ensure that the generic constraints are consistent with other methods to prevent type mismatches.
89-90: Ohayo, sensei! Comprehensive testing in write_models_test
The addition of write_models_test is excellent for testing batch writes. Ensure that tests cover diverse scenarios, including error handling for partial successes.
93-94: Ohayo, sensei! Verify complete deletion in erase_models_test
In erase_models_test, confirm that all specified models are successfully deleted and that the method handles non-existent models gracefully.
Do you need assistance in creating test cases for these scenarios?
97-98: Ohayo, sensei! Maintain type integrity in erase_models_ptrs_test
Ensure that erase_models_ptrs_test maintains type integrity when deleting models by pointers in a test environment.
44-46: Ohayo, sensei! Confirm type safety in erase_models_ptrs
Ensure that ModelPtr<M> provides adequate type inference and that the method correctly handles different pointer types during batch deletions.
✅ Verification successful
Let me gather more information about the ModelPtr type and its usage to ensure type safety.
Ohayo! Type safety in erase_models_ptrs is properly maintained, sensei!
The implementation shows that ModelPtr<M> is a well-defined enum with proper type handling:
- It correctly handles both
IdandKeysvariants in the implementation - The generic parameter
Mis consistently used for type inference - The world storage implementation properly matches and processes both pointer types during batch deletions
- Type safety is enforced through the trait interface and its implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for correct usage of `erase_models_ptrs` with various `ModelPtr<M>` types.
# Expected: Consistent usage without type inference issues.
rg 'erase_models_ptrs' -A 3
Length of output: 1359
Script:
#!/bin/bash
# Check ModelPtr definition and implementation
ast-grep --pattern 'struct ModelPtr { $$$ }'
# Check for ModelPtr usage patterns
rg "ModelPtr" -B 2 -A 2
# Look for any type-related implementations or traits for ModelPtr
ast-grep --pattern 'impl $_ of $_ for ModelPtr { $$$ }'
Length of output: 6952
crates/dojo/core/src/world/iworld.cairo (4)
130-131: Excellent adjustment in 'emit_event', sensei!
Ohayo! Removing the historical parameter streamlines the method and aligns with the updated event handling strategy.
191-207: Efficient batching in 'set_entities', sensei!
Ohayo! The introduction of set_entities enhances performance by batching writes and reducing permission checks. Great job optimizing the system!
218-229: Smart addition of 'delete_entities', sensei!
Ohayo! Implementing delete_entities improves efficiency by batching deletions. This aligns well with the PR objectives to reduce overhead.
311-311: Good alignment of 'emit_event_test', sensei!
Ohayo! The changes to emit_event_test correctly reflect the updates made to emit_event, ensuring consistency in the test interface.
crates/dojo/core/src/world/world_contract.cairo (3)
307-307: Ohayo sensei! Confirming the correctness of the event emission
The added line correctly includes system_address: get_caller_address() in the EventEmitted struct. This ensures that the system address is accurately captured when emitting events during tests.
843-843: Ohayo sensei! Confirming the use of internal method in entity
The refactoring to use self.get_entity_internal enhances code reusability and maintains consistency.
1183-1210: Ohayo sensei! Confirming the implementation of get_entity_internal
The get_entity_internal function correctly handles the different ModelIndex variants and retrieves the appropriate entity data.
crates/dojo/world/src/contracts/abigen/world.rs (1)
3455-3491: Verify the correctness of batch event emission in emit_events methods
Ohayo sensei! Ensure that the new emit_events methods correctly handle batch event emissions without side effects or unexpected behaviors.
Run the following script to verify uses of emit_events:
✅ Verification successful
Let me gather more context about the event emission implementation and usage.
Let me gather more specific information about the event emission implementation.
Batch event emission implementation is correctly structured
Ohayo sensei! After analyzing the codebase, I can confirm that the emit_events implementation is correct and safe:
- The method properly serializes event data using
CairoSerde - Parameters are correctly typed with
Vec<Vec<starknet::core::types::Felt>>for batch processing - The implementation follows the same pattern as the single event emission
- Proper clippy allowances are in place for valid cases
- The method correctly creates and executes the call via the account abstraction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `emit_events` is properly used throughout the codebase
# Expected: All batch event emissions use `emit_events` correctly
# Search for all `emit_events` function calls
rg 'emit_events\(' -t rust -A 3
# Ensure that the parameters passed match the expected types and lengths
Length of output: 372
Script:
#!/bin/bash
# Search for the World contract implementation and event-related code
ast-grep --pattern 'struct World {
$$$
}'
# Look for event-related tests
rg 'test.*emit.*event' -t rust -A 5
# Find any event emission related implementations
rg 'fn.*emit' -t rust -A 3
# Look for actual usage of emit_events in the codebase
rg '\.emit_events\(' -t rust -A 3
Length of output: 2220
Script:
#!/bin/bash
# Look for test cases that specifically use emit_events
rg 'emit_events\(' -t rust -B 5 -A 10
# Check for any error handling or validation around event emission
ast-grep --pattern 'fn emit_events($$$) {
$$$
}'
# Look for any related event structs or types
rg 'struct.*Event' -t rust -A 5
# Search for any comments or documentation about event emission
rg '//.*emit.*event' -t rust
rg '/\*.*emit.*event.*\*/' -t rust
Length of output: 17122
| #[test] | ||
| fn write_multiple_copiable() { | ||
| let (mut world, _) = deploy_world_and_foo(); | ||
|
|
||
| let mut models_snaps: Array<@Foo> = array![]; | ||
| let mut keys: Array<starknet::ContractAddress> = array![]; | ||
|
|
||
| for i in 0_u128 | ||
| ..10_u128 { | ||
| let felt: felt252 = i.into(); | ||
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | ||
| keys.append(caller); | ||
|
|
||
| if i % 2 == 0 { | ||
| let foo = Foo { caller, a: felt, b: i }; | ||
| models_snaps.append(@foo); | ||
| } else { | ||
| let foo = Foo { caller, a: felt, b: i }; | ||
| models_snaps.append(@foo); | ||
| } | ||
| }; | ||
|
|
||
| world.write_models(models_snaps.span()); | ||
|
|
||
| let mut models: Array<Foo> = world.read_models(keys.span()); | ||
|
|
||
| assert_eq!(models.len(), 10); | ||
|
|
||
| for i in 0_u128 | ||
| ..10_u128 { | ||
| let felt: felt252 = i.into(); | ||
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | ||
| // Can desnap as copiable. | ||
| let model: Foo = *models[i.try_into().unwrap()]; | ||
| assert_eq!(model.caller, caller); | ||
| assert_eq!(model.a, felt); | ||
| assert_eq!(model.b, i); | ||
| }; | ||
|
|
||
| world.erase_models(models_snaps.span()); | ||
|
|
||
| let mut models: Array<Foo> = world.read_models(keys.span()); | ||
|
|
||
| while let Option::Some(m) = models.pop_front() { | ||
| assert_eq!(m.a, 0); | ||
| assert_eq!(m.b, 0); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Let's enhance the test structure, sensei!
A few suggestions to improve the test:
- Consider extracting the magic number
10into a constant - The if/else branches contain identical code, making the condition unnecessary
- The comment "Can desnap as copiable" is redundant given the type information
Here's a suggested refactor for the redundant if/else:
- if i % 2 == 0 {
- let foo = Foo { caller, a: felt, b: i };
- models_snaps.append(@foo);
- } else {
- let foo = Foo { caller, a: felt, b: i };
- models_snaps.append(@foo);
- }
+ let foo = Foo { caller, a: felt, b: i };
+ models_snaps.append(@foo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn write_multiple_copiable() { | |
| let (mut world, _) = deploy_world_and_foo(); | |
| let mut models_snaps: Array<@Foo> = array![]; | |
| let mut keys: Array<starknet::ContractAddress> = array![]; | |
| for i in 0_u128 | |
| ..10_u128 { | |
| let felt: felt252 = i.into(); | |
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | |
| keys.append(caller); | |
| if i % 2 == 0 { | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); | |
| } else { | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); | |
| } | |
| }; | |
| world.write_models(models_snaps.span()); | |
| let mut models: Array<Foo> = world.read_models(keys.span()); | |
| assert_eq!(models.len(), 10); | |
| for i in 0_u128 | |
| ..10_u128 { | |
| let felt: felt252 = i.into(); | |
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | |
| // Can desnap as copiable. | |
| let model: Foo = *models[i.try_into().unwrap()]; | |
| assert_eq!(model.caller, caller); | |
| assert_eq!(model.a, felt); | |
| assert_eq!(model.b, i); | |
| }; | |
| world.erase_models(models_snaps.span()); | |
| let mut models: Array<Foo> = world.read_models(keys.span()); | |
| while let Option::Some(m) = models.pop_front() { | |
| assert_eq!(m.a, 0); | |
| assert_eq!(m.b, 0); | |
| }; | |
| } | |
| #[test] | |
| fn write_multiple_copiable() { | |
| let (mut world, _) = deploy_world_and_foo(); | |
| let mut models_snaps: Array<@Foo> = array![]; | |
| let mut keys: Array<starknet::ContractAddress> = array![]; | |
| for i in 0_u128 | |
| ..10_u128 { | |
| let felt: felt252 = i.into(); | |
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | |
| keys.append(caller); | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); | |
| }; | |
| world.write_models(models_snaps.span()); | |
| let mut models: Array<Foo> = world.read_models(keys.span()); | |
| assert_eq!(models.len(), 10); | |
| for i in 0_u128 | |
| ..10_u128 { | |
| let felt: felt252 = i.into(); | |
| let caller: starknet::ContractAddress = felt.try_into().unwrap(); | |
| let model: Foo = *models[i.try_into().unwrap()]; | |
| assert_eq!(model.caller, caller); | |
| assert_eq!(model.a, felt); | |
| assert_eq!(model.b, i); | |
| }; | |
| world.erase_models(models_snaps.span()); | |
| let mut models: Array<Foo> = world.read_models(keys.span()); | |
| while let Option::Some(m) = models.pop_front() { | |
| assert_eq!(m.a, 0); | |
| assert_eq!(m.b, 0); | |
| }; | |
| } |
| /// Sets multiple models of type `M`. | ||
| fn write_models(ref self: S, models: Span<@M>); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Implement error handling for batch writes
The write_models method enhances efficiency by allowing batch writing of models. However, consider implementing error handling for partial failures to maintain data integrity during batch operations.
| /// Retrieves multiple models of type `M` using the provided keys of type `K`. | ||
| /// Returnes an array to ensure the user can consume the models, even if the type is not | ||
| /// copiable. | ||
| fn read_models<K, +Drop<K>, +Serde<K>>(self: @S, keys: Span<K>) -> Array<M>; | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Handle deserialization errors in read_models
When reading multiple models with read_models, ensure that you handle potential deserialization errors gracefully. Returning an array might obscure failures if some models cannot be deserialized.
Would you like assistance in enhancing error handling for partial deserialization failures?
| /// Updates multiple model values of type `V`. | ||
| fn write_values<K, +Drop<K>, +Serde<K>, +ModelValueKey<V, K>>( | ||
| ref self: S, keys: Span<K>, values: Span<@V> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Validate matching lengths in write_values
Ensure that the lengths of keys and values in the write_values method match. Mismatched lengths could lead to indexing errors or unintended behavior.
| /// Updates multiple model values of type `V`. | ||
| fn write_values_from_ids(ref self: S, entity_ids: Span<felt252>, values: Span<@V>); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Error handling in write_values_from_ids
Implement validation to confirm that all entity_ids are valid before writing values. This prevents potential runtime errors and maintains data integrity.
| fn write_values_from_ids_test( | ||
| ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V> | ||
| ) { | ||
| let mut i = 0; | ||
| loop { | ||
| if i >= entity_ids.len() { | ||
| break; | ||
| } | ||
|
|
||
| dojo::world::IWorldTestDispatcherTrait::delete_entity_test( | ||
| world_test, | ||
| ModelValue::<V>::selector(self.namespace_hash), | ||
| ModelIndex::Id(entity_id), | ||
| ModelValue::<V>::layout() | ||
| ); | ||
| Self::write_value_from_id_test(ref self, *entity_ids[i], *values[i]); | ||
|
|
||
| i += 1; | ||
| } |
There was a problem hiding this comment.
Prevent index errors in write_values_from_ids_test
Ohayo, sensei! Similar to previous suggestions, in write_values_from_ids_test, verifying that entity_ids and values have matching lengths will enhance test reliability and prevent runtime errors.
Suggested addition:
+ if entity_ids.len() != values.len() {
+ panic!("Entity IDs and values must have the same length for testing");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn write_values_from_ids_test( | |
| ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V> | |
| ) { | |
| let mut i = 0; | |
| loop { | |
| if i >= entity_ids.len() { | |
| break; | |
| } | |
| dojo::world::IWorldTestDispatcherTrait::delete_entity_test( | |
| world_test, | |
| ModelValue::<V>::selector(self.namespace_hash), | |
| ModelIndex::Id(entity_id), | |
| ModelValue::<V>::layout() | |
| ); | |
| Self::write_value_from_id_test(ref self, *entity_ids[i], *values[i]); | |
| i += 1; | |
| } | |
| fn write_values_from_ids_test( | |
| ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V> | |
| ) { | |
| if entity_ids.len() != values.len() { | |
| panic!("Entity IDs and values must have the same length for testing"); | |
| } | |
| let mut i = 0; | |
| loop { | |
| if i >= entity_ids.len() { | |
| break; | |
| } | |
| Self::write_value_from_id_test(ref self, *entity_ids[i], *values[i]); | |
| i += 1; | |
| } |
There was a problem hiding this comment.
Not required for the tests API.
There was a problem hiding this comment.
@glihm, understood. I'll note that the length check is not required for the tests API.
✏️ Learnings added
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.152Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| fn write_values_from_ids(ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V>) { | ||
| let mut indexes: Array<ModelIndex> = array![]; | ||
| let mut all_values: Array<Span<felt252>> = array![]; | ||
| let mut i = 0; | ||
|
|
||
| loop { | ||
| if i >= entity_ids.len() { | ||
| break; | ||
| } | ||
|
|
||
| indexes.append(ModelIndex::Id(*entity_ids[i])); | ||
| all_values.append(ModelValue::<V>::values(*values[i])); | ||
|
|
||
| i += 1; | ||
| }; | ||
|
|
||
| IWorldDispatcherTrait::set_entities( | ||
| self.dispatcher, | ||
| ModelValue::<V>::selector(self.namespace_hash), | ||
| indexes.span(), | ||
| all_values.span(), | ||
| ModelValue::<V>::layout() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure synchronized lengths in write_values_from_ids
Ohayo, sensei! The write_values_from_ids function assumes that entity_ids and values are of equal length. Adding a check to confirm this would prevent indexing errors and enhance reliability.
Suggested addition:
+ if entity_ids.len() != values.len() {
+ panic!("Entity IDs and values must have the same length");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn write_values_from_ids(ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V>) { | |
| let mut indexes: Array<ModelIndex> = array![]; | |
| let mut all_values: Array<Span<felt252>> = array![]; | |
| let mut i = 0; | |
| loop { | |
| if i >= entity_ids.len() { | |
| break; | |
| } | |
| indexes.append(ModelIndex::Id(*entity_ids[i])); | |
| all_values.append(ModelValue::<V>::values(*values[i])); | |
| i += 1; | |
| }; | |
| IWorldDispatcherTrait::set_entities( | |
| self.dispatcher, | |
| ModelValue::<V>::selector(self.namespace_hash), | |
| indexes.span(), | |
| all_values.span(), | |
| ModelValue::<V>::layout() | |
| ); | |
| } | |
| fn write_values_from_ids(ref self: WorldStorage, entity_ids: Span<felt252>, values: Span<@V>) { | |
| if entity_ids.len() != values.len() { | |
| panic!("Entity IDs and values must have the same length"); | |
| } | |
| let mut indexes: Array<ModelIndex> = array![]; | |
| let mut all_values: Array<Span<felt252>> = array![]; | |
| let mut i = 0; | |
| loop { | |
| if i >= entity_ids.len() { | |
| break; | |
| } | |
| indexes.append(ModelIndex::Id(*entity_ids[i])); | |
| all_values.append(ModelValue::<V>::values(*values[i])); | |
| i += 1; | |
| }; | |
| IWorldDispatcherTrait::set_entities( | |
| self.dispatcher, | |
| ModelValue::<V>::selector(self.namespace_hash), | |
| indexes.span(), | |
| all_values.span(), | |
| ModelValue::<V>::layout() | |
| ); | |
| } |
| fn read_values_from_ids(self: @WorldStorage, entity_ids: Span<felt252>) -> Array<V> { | ||
| let mut indexes: Array<ModelIndex> = array![]; | ||
| for id in entity_ids { | ||
| indexes.append(ModelIndex::Id(*id)); | ||
| }; | ||
|
|
||
| let mut all_values = IWorldDispatcherTrait::entities( | ||
| *self.dispatcher, | ||
| ModelValue::<V>::selector(*self.namespace_hash), | ||
| indexes.span(), | ||
| ModelValue::<V>::layout() | ||
| ); | ||
|
|
||
| let mut values: Array<V> = array![]; | ||
| let mut i = 0; | ||
| loop { | ||
| if i >= indexes.len() { | ||
| break; | ||
| } | ||
|
|
||
| let entity_id = *entity_ids[i]; | ||
| let mut v = *all_values[i]; | ||
|
|
||
| match ModelValue::<V>::from_values(entity_id, ref v) { | ||
| Option::Some(value) => values.append(value), | ||
| Option::None => { | ||
| panic!( | ||
| "Value: deserialization failed. Ensure the length of the keys tuple is matching the number of #[key] fields in the model struct." | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| i += 1; | ||
| }; | ||
|
|
||
| values | ||
| } |
There was a problem hiding this comment.
Add length check in read_values_from_ids to prevent indexing errors
Ohayo, sensei! In the read_values_from_ids function, there's a potential risk of index out-of-bounds errors if entity_ids and all_values lengths mismatch. Consider adding a length check before the loop to ensure they are equal.
Here's a suggested addition:
+ if entity_ids.len() != all_values.len() {
+ panic!("Mismatch between entity_ids and retrieved values lengths");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn read_values_from_ids(self: @WorldStorage, entity_ids: Span<felt252>) -> Array<V> { | |
| let mut indexes: Array<ModelIndex> = array![]; | |
| for id in entity_ids { | |
| indexes.append(ModelIndex::Id(*id)); | |
| }; | |
| let mut all_values = IWorldDispatcherTrait::entities( | |
| *self.dispatcher, | |
| ModelValue::<V>::selector(*self.namespace_hash), | |
| indexes.span(), | |
| ModelValue::<V>::layout() | |
| ); | |
| let mut values: Array<V> = array![]; | |
| let mut i = 0; | |
| loop { | |
| if i >= indexes.len() { | |
| break; | |
| } | |
| let entity_id = *entity_ids[i]; | |
| let mut v = *all_values[i]; | |
| match ModelValue::<V>::from_values(entity_id, ref v) { | |
| Option::Some(value) => values.append(value), | |
| Option::None => { | |
| panic!( | |
| "Value: deserialization failed. Ensure the length of the keys tuple is matching the number of #[key] fields in the model struct." | |
| ) | |
| } | |
| } | |
| i += 1; | |
| }; | |
| values | |
| } | |
| fn read_values_from_ids(self: @WorldStorage, entity_ids: Span<felt252>) -> Array<V> { | |
| let mut indexes: Array<ModelIndex> = array![]; | |
| for id in entity_ids { | |
| indexes.append(ModelIndex::Id(*id)); | |
| }; | |
| let mut all_values = IWorldDispatcherTrait::entities( | |
| *self.dispatcher, | |
| ModelValue::<V>::selector(*self.namespace_hash), | |
| indexes.span(), | |
| ModelValue::<V>::layout() | |
| ); | |
| if entity_ids.len() != all_values.len() { | |
| panic!("Mismatch between entity_ids and retrieved values lengths"); | |
| } | |
| let mut values: Array<V> = array![]; | |
| let mut i = 0; | |
| loop { | |
| if i >= indexes.len() { | |
| break; | |
| } | |
| let entity_id = *entity_ids[i]; | |
| let mut v = *all_values[i]; | |
| match ModelValue::<V>::from_values(entity_id, ref v) { | |
| Option::Some(value) => values.append(value), | |
| Option::None => { | |
| panic!( | |
| "Value: deserialization failed. Ensure the length of the keys tuple is matching the number of #[key] fields in the model struct." | |
| ) | |
| } | |
| } | |
| i += 1; | |
| }; | |
| values | |
| } |
| pub fn delete_entities_getcall( | ||
| &self, | ||
| model_selector: &starknet::core::types::Felt, | ||
| indexes: &Vec<ModelIndex>, | ||
| layout: &Layout, | ||
| ) -> starknet::core::types::Call { | ||
| use cainome::cairo_serde::CairoSerde; | ||
| let mut __calldata = vec![]; | ||
| __calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector)); | ||
| __calldata.extend(Vec::<ModelIndex>::cairo_serialize(indexes)); | ||
| __calldata.extend(Layout::cairo_serialize(layout)); | ||
| starknet::core::types::Call { | ||
| to: self.address, | ||
| selector: starknet::macros::selector!("delete_entities"), | ||
| calldata: __calldata, | ||
| } | ||
| } | ||
| #[allow(clippy::ptr_arg)] | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn delete_entities( | ||
| &self, | ||
| model_selector: &starknet::core::types::Felt, | ||
| indexes: &Vec<ModelIndex>, | ||
| layout: &Layout, | ||
| ) -> starknet::accounts::ExecutionV1<A> { | ||
| use cainome::cairo_serde::CairoSerde; | ||
| let mut __calldata = vec![]; | ||
| __calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector)); | ||
| __calldata.extend(Vec::<ModelIndex>::cairo_serialize(indexes)); | ||
| __calldata.extend(Layout::cairo_serialize(layout)); | ||
| let __call = starknet::core::types::Call { | ||
| to: self.address, | ||
| selector: starknet::macros::selector!("delete_entities"), | ||
| calldata: __calldata, | ||
| }; | ||
| self.account.execute_v1(vec![__call]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce duplication between delete_entities and delete_entity methods
Both methods share similar logic. Consider refactoring to avoid code duplication, which will improve maintainability and adhere to the DRY principle.
Apply this diff to refactor:
-#[allow(clippy::ptr_arg)]
-#[allow(clippy::too_many_arguments)]
-pub fn delete_entities_getcall(
- &self,
- model_selector: &starknet::core::types::Felt,
- indexes: &Vec<ModelIndex>,
- layout: &Layout,
-) -> starknet::core::types::Call {
- // Implementation
-}
-
-#[allow(clippy::ptr_arg)]
-#[allow(clippy::too_many_arguments)]
-pub fn delete_entity_getcall(
- &self,
- model_selector: &starknet::core::types::Felt,
- index: &ModelIndex,
- layout: &Layout,
-) -> starknet::core::types::Call {
- // Similar implementation
-}
+#[allow(clippy::ptr_arg)]
+#[allow(clippy::too_many_arguments)]
+pub fn delete_entities_getcall(
+ &self,
+ model_selector: &starknet::core::types::Felt,
+ indexes: &[ModelIndex],
+ layout: &Layout,
+) -> starknet::core::types::Call {
+ // Implementation
+}
+
+pub fn delete_entity_getcall(
+ &self,
+ model_selector: &starknet::core::types::Felt,
+ index: &ModelIndex,
+ layout: &Layout,
+) -> starknet::core::types::Call {
+ self.delete_entities_getcall(model_selector, &vec![index.clone()], layout)
+}Committable suggestion skipped: line range outside the PR's diff.
| pub fn set_entities_getcall( | ||
| &self, | ||
| model_selector: &starknet::core::types::Felt, | ||
| indexes: &Vec<ModelIndex>, | ||
| values: &Vec<Vec<starknet::core::types::Felt>>, | ||
| layout: &Layout, | ||
| ) -> starknet::core::types::Call { | ||
| use cainome::cairo_serde::CairoSerde; | ||
| let mut __calldata = vec![]; | ||
| __calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector)); | ||
| __calldata.extend(Vec::<ModelIndex>::cairo_serialize(indexes)); | ||
| __calldata.extend(Vec::<Vec<starknet::core::types::Felt>>::cairo_serialize(values)); | ||
| __calldata.extend(Layout::cairo_serialize(layout)); | ||
| starknet::core::types::Call { | ||
| to: self.address, | ||
| selector: starknet::macros::selector!("set_entities"), | ||
| calldata: __calldata, | ||
| } | ||
| } | ||
| #[allow(clippy::ptr_arg)] | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub fn set_entities( | ||
| &self, | ||
| model_selector: &starknet::core::types::Felt, | ||
| indexes: &Vec<ModelIndex>, | ||
| values: &Vec<Vec<starknet::core::types::Felt>>, | ||
| layout: &Layout, | ||
| ) -> starknet::accounts::ExecutionV1<A> { | ||
| use cainome::cairo_serde::CairoSerde; | ||
| let mut __calldata = vec![]; | ||
| __calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector)); | ||
| __calldata.extend(Vec::<ModelIndex>::cairo_serialize(indexes)); | ||
| __calldata.extend(Vec::<Vec<starknet::core::types::Felt>>::cairo_serialize(values)); | ||
| __calldata.extend(Layout::cairo_serialize(layout)); | ||
| let __call = starknet::core::types::Call { | ||
| to: self.address, | ||
| selector: starknet::macros::selector!("set_entities"), | ||
| calldata: __calldata, | ||
| }; | ||
| self.account.execute_v1(vec![__call]) | ||
| } |
There was a problem hiding this comment.
Handle empty inputs in set_entities method
The set_entities method does not currently check for empty indexes or values, which could lead to runtime errors or inconsistent state.
Apply this diff to add input validation:
pub fn set_entities_getcall(
&self,
model_selector: &starknet::core::types::Felt,
indexes: &Vec<ModelIndex>,
values: &Vec<Vec<starknet::core::types::Felt>>,
layout: &Layout,
) -> starknet::core::types::Call {
+ if indexes.is_empty() || values.is_empty() {
+ panic!("Indexes and values cannot be empty");
+ }
+
use cainome::cairo_serde::CairoSerde;
let mut __calldata = vec![];
__calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This is auto generated code.
There was a problem hiding this comment.
@glihm, ohayo sensei! Thank you for letting me know that this code is auto-generated. To prevent me from reviewing auto-generated files in the future, you can configure CodeRabbit to ignore specific paths by adding the following to your configuration:
path_filters:
exclude:
- 'crates/dojo/world/src/contracts/abigen/**'This will ensure that auto-generated code in that directory is excluded from code reviews.
✏️ Learnings added
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.697Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2633 +/- ##
==========================================
- Coverage 56.89% 56.81% -0.09%
==========================================
Files 397 397
Lines 49467 49535 +68
==========================================
- Hits 28143 28142 -1
- Misses 21324 21393 +69 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request introduces a series of changes across multiple files, focusing on enhancing dependency management, event handling, and storage capabilities within the Dojo framework. Key modifications include the activation of previously commented dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorldContract
participant EventProcessor
participant Storage
User->>WorldContract: emit_events(keys, values)
WorldContract->>EventProcessor: process events
EventProcessor->>Storage: store event data
Storage-->>EventProcessor: confirm storage
EventProcessor-->>WorldContract: events processed
WorldContract-->>User: confirmation
🪧 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
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (34)
crates/dojo/core-cairo-test/src/tests/event/event.cairo (2)
Line range hint
1-9: Ohayo! The event structure looks solid, sensei!The
FooEventstruct is well-structured with appropriate field types and key annotations. Consider adding documentation comments to describe the purpose of each field for better maintainability.Add documentation like this:
#[derive(Drop, Serde)] #[dojo::event] +/// Represents a test event for validating event definition functionality struct FooEvent { + /// First key field of type u8 #[key] k1: u8, + /// Second key field of type felt252 #[key] k2: felt252, + /// First value field storing a u128 number v1: u128, + /// Second value field storing a u32 number v2: u32 }
Line range hint
11-17: Consider enhancing test coverage, sensei!While the current test verifies basic event definition properties, consider adding test cases for:
- Event emission
- Batch event handling (aligning with PR's batch entities feature)
- Serialization/deserialization validation
Would you like me to help generate these additional test cases?
crates/torii/core/src/processors/event_message.rs (1)
74-74: Track the TODO with an issueOhayo sensei! The TODO comment indicates a pending implementation for configuration-based historical tracking. Let's ensure this doesn't get lost in the codebase.
Would you like me to create a GitHub issue to track this TODO for implementing the configuration-based historical value?
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (1)
71-81: Ohayo! Consider enhancing the ABI ensure implementation, sensei! 🎋While the implementation is functionally correct, we could make it more robust and clearer:
- The comment could better explain why the Event struct needs explicit ABI inclusion
- The parameter and variable naming could be more specific
Consider this enhancement:
#[generate_trait] impl $type_name$Impl of I$type_name${ - // Ensures the ABI contains the Event struct, since it's never used - // by systems directly. + /// Ensures the Event struct is included in the contract's ABI. + /// This is necessary because events are typically emitted rather than + /// directly used as parameters, which could cause them to be omitted + /// from the ABI generation. #[external(v0)] - fn ensure_abi(self: @ContractState, event: $type_name$) { - let _event = event; + fn ensure_abi(self: @ContractState, event_struct: $type_name$) { + let _preserved_event = event_struct; } }crates/dojo/core/src/world/errors.cairo (1)
5-7: Ohayo! The implementation looks solid, sensei!The new
lengths_mismatchfunction follows the established error formatting patterns and will be valuable for batch operations validation. Consider adding a brief doc comment to explain its usage context.+/// Formats an error message for length mismatches between two byte arrays in batch operations. +/// +/// # Arguments +/// * `a` - First byte array reference +/// * `b` - Second byte array reference +/// * `context` - Description of where the mismatch occurred pub fn lengths_mismatch(a: @ByteArray, b: @ByteArray, context: @ByteArray) -> ByteArray { format!("Length mismatch: `{a}` and `{b}` in `{context}`") }crates/dojo/core-cairo-test/src/tests/world/storage.cairo (3)
5-30: Consider adding error cases and batch operation tests, sensei!While the basic CRUD operations are well tested, consider adding:
- Error cases (e.g., reading non-existent models)
- Batch operations to align with the PR's objective of optimizing permissions checks for multiple entities
64-65: Update the misleading comment, sensei!The comment "Can desnap as copiable" is misleading since we're accessing an array element, not performing deserialization.
- // Can desnap as copiable. + // Access array element directly since Foo is copiable
113-114: Fix incorrect comment, sensei!The comment about deserialization is incorrect for non-copiable types.
- // Can desnap as copiable. + // Pop from array since NotCopiable is move-onlybin/sozo/tests/test_data/policies.json (1)
111-113: New granular permission management methods added.The addition of
grant_owner,revoke_owner,grant_writer, andrevoke_writermethods introduces fine-grained permission control. This follows the principle of least privilege and provides better access control management.Consider documenting the permission hierarchy and access control model in the project documentation to help developers understand the implications of these new methods.
Also applies to: 115-117, 119-121, 123-125
crates/torii/types-test/src/contracts.cairo (3)
Line range hint
29-119: Consider batching the model writes, sensei!The create method currently performs individual
write_modelcalls for each record, sibling, and subrecord. Since this PR introduces batch entity capabilities, we could optimize this by batching these writes together.Consider refactoring to use batch operations:
- world.write_model(@record); - world.write_model(@record_sibling); - world.write_model(@subrecord); + let models = array![record, record_sibling, subrecord]; + world.batch_write_models(models);
Line range hint
121-122: Add error handling for record existence, sensei!The delete method should verify that the record exists before proceeding with deletion operations.
Consider adding validation:
+ // Verify record exists before deletion + if !world.model_exists<Record>(record_id) { + panic_with_felt252('Record does not exist'); + }
Line range hint
129-131: Optimize deletions using batch operationsSince this PR introduces batch capabilities, we should leverage them for deletions as well.
Consider refactoring to:
- world.erase_model(@record); - world.erase_model(@record_sibling); - world.erase_model(@subrecord); + let models_to_delete = array![record, record_sibling, subrecord]; + world.batch_erase_models(models_to_delete);crates/dojo/lang/src/attribute_macros/event.rs (1)
Line range hint
31-140: Consider enhancing event validation for batch operationsSince this PR introduces batch entity processing, the event validation in
from_structmight benefit from additional checks:if serialized_keys.is_empty() { diagnostics.push(PluginDiagnostic { - message: "Event must define at least one #[key] attribute".into(), + message: "Event must define at least one #[key] attribute. For batch operations, consider using composite keys".into(), stable_ptr: struct_ast.name(db).stable_ptr().untyped(), severity: Severity::Error, }); }This would help guide developers in properly structuring their events for batch operations.
crates/dojo/core-cairo-test/src/tests/helpers.cairo (2)
29-36: Ohayo sensei! The NotCopiable model looks well-structured!The model effectively demonstrates handling of non-copyable types (Array, ByteArray) which is crucial for testing batch operations. The trait implementations (Drop, Serde, Debug) are appropriate for the model's purpose.
Consider documenting the purpose of this test model with a comment explaining its role in batch entity testing scenarios. This would help other developers understand why we're specifically testing with non-copyable types.
220-223: Consider renaming the function for clarity, sensei!The function now deploys both
FooandNotCopiablemodels, but its namedeploy_world_and_fooonly suggests the deployment of the Foo model.Consider renaming to
deploy_world_and_modelsor similar to better reflect its current responsibility:-pub fn deploy_world_and_foo() -> (WorldStorage, felt252) { +pub fn deploy_world_and_models() -> (WorldStorage, felt252) {bin/sozo/src/commands/events.rs (2)
348-348: Ohayo! The format string update aligns with historical removal.The format string has been simplified by removing the historical field, which aligns well with the PR objective of removing historical aspects from the chain. The remaining fields provide comprehensive event information including selector, contract, keys, values, and data.
However, consider adding a comment explaining why historical tracking was removed and where users can find this information in the Torii configuration.
Line range hint
348-361: Consider improving the format string readability, sensei!The current format string spans multiple lines with nested formatting. Consider extracting the key-value formatting logic into a helper function for better maintainability.
+ fn format_hex_array(values: &[Felt], prefix: &str) -> String { + format!("{}: {}", prefix, values + .iter() + .map(|v| format!("{:#066x}", v)) + .collect::<Vec<String>>() + .join(", ")) + } - "Selector: {:#066x}\nContract: {}\nKeys: {}\nValues: {}\nData:\n{}", - e.selector, - contract_tag, - e.keys - .iter() - .map(|k| format!("{:#066x}", k)) - .collect::<Vec<String>>() - .join(", "), - e.values - .iter() - .map(|v| format!("{:#066x}", v)) - .collect::<Vec<String>>() - .join(", "), - record + "{}\nContract: {}\n{}\n{}\nData:\n{}", + format!("Selector: {:#066x}", e.selector), + contract_tag, + format_hex_array(&e.keys, "Keys"), + format_hex_array(&e.values, "Values"), + recordcrates/dojo/core/src/model/storage.cairo (3)
29-32: Minor typo in documentation commentOhayo, sensei! Just a small nitpick—the comment for
read_modelshas a typo:
- "Returnes" should be "Returns"
Apply this diff to fix the typo:
-/// Returnes an array to ensure the user can consume the models, even if the type is not +/// Returns an array to ensure the user can consume the models, even if the type is not
44-46: Consider renaming for consistencyOhayo, sensei! For consistency with other method names, consider renaming
erase_models_ptrstoerase_model_ptrs(singular "model") to matcherase_model_ptr.Apply this diff for consistent naming:
-fn erase_models_ptrs(ref self: S, ptrs: Span<ModelPtr<M>>); +fn erase_model_ptrs(ref self: S, ptrs: Span<ModelPtr<M>>);
97-98: Align test method namingOhayo, sensei! Similar to the earlier suggestion, consider renaming
erase_models_ptrs_testtoerase_model_ptrs_testfor consistency.Apply this diff for consistent naming:
-fn erase_models_ptrs_test(ref self: S, ptrs: Span<ModelPtr<M>>); +fn erase_model_ptrs_test(ref self: S, ptrs: Span<ModelPtr<M>>);crates/dojo/core/src/world/iworld.cairo (4)
133-146: Great addition ofemit_eventsfor batch processingIntroducing the
emit_eventsfunction enhances performance by allowing multiple events to be emitted with a single permissions check, reducing overhead.
169-173: Inconsistent terminology: 'indices' vs 'indexes' inentitiesfunctionThere's an inconsistency between the parameter name
indexesand the documentation referring toindices. For clarity, consider standardizing the term.Apply this diff to rename the parameter:
/// * `indices` - The indexes of the entities/members to read. fn entities( - self: @T, model_selector: felt252, indexes: Span<ModelIndex>, layout: Layout + self: @T, model_selector: felt252, indices: Span<ModelIndex>, layout: Layout ) -> Span<Span<felt252>>;
197-206: Inconsistent terminology: 'indices' vs 'indexes' inset_entitiesfunctionSimilarly, in the
set_entitiesfunction, the parameterindexesconflicts with the use ofindicesin the comments. Standardizing improves readability.Apply this diff:
/// * `indices` - The indexes of the entities/members to write. fn set_entities( ref self: T, model_selector: felt252, - indexes: Span<ModelIndex>, + indices: Span<ModelIndex>, values: Span<Span<felt252>>, layout: Layout );
223-228: Inconsistent terminology: 'indices' vs 'indexes' indelete_entitiesfunctionThe
delete_entitiesfunction also mixesindexesin parameters withindicesin documentation. Consistency is key for maintainability.Apply this diff:
/// * `indices` - The indexes of the entities/members to delete. fn delete_entities( ref self: T, model_selector: felt252, - indexes: Span<ModelIndex>, + indices: Span<ModelIndex>, layout: Layout );crates/dojo/core/src/world/storage.cairo (4)
78-116: Consider Refactoring Batch Operations for MaintainabilityOhayo sensei! I noticed that the methods
read_models,write_models, anderase_modelsshare similar looping structures and logic for handling batch operations. To enhance maintainability and reduce code duplication, consider refactoring common code into generic helper functions.
95-113: Refactor Loop to UseforConstruct for Enhanced ReadabilityOhayo sensei! In the loop starting at line 95, using a
forloop can simplify the code and improve readability. Here's how you might refactor it:- let mut i = 0; - loop { - if i >= indexes.len() { - break; - } - - // Existing code - - i += 1; - }; + for i in 0..indexes.len() { + + // Existing code + + }
Line range hint
208-278: Ensure Unit Test Coverage for New Batch MethodsOhayo sensei! To maintain our code quality standards, please ensure that the new batch methods like
read_valuesandread_values_from_idsare thoroughly covered by unit tests.
376-380: Utilize Batch Methods in Test ImplementationsOhayo sensei! In
write_models_test, consider using thewrite_modelsmethod directly instead of iterating over each model. This can make the test code more efficient and align it with the batch operation implementations.crates/dojo/core/src/world/world_contract.cairo (5)
800-838: Refactor loop for enhanced readabilityOhayo, sensei! Consider replacing the manual
loopwith aforloop in theemit_eventsfunction to improve clarity and maintainability.Apply this diff to refactor the loop:
let mut i = 0; loop { if i >= keys.len() { break; } self .emit( EventEmitted { selector: event_selector, system_address: get_caller_address(), keys: *keys[i], values: *values[i], } ); i += 1; }Replace with:
+ for i in 0..keys.len() { + self.emit( + EventEmitted { + selector: event_selector, + system_address: get_caller_address(), + keys: *keys[i], + values: *values[i], + } + ); + }
846-856: Preallocate array capacity for performanceOhayo, sensei! In the
entitiesfunction, preallocating the capacity of themodelsarray can enhance performance, especially when dealing with large datasets.Apply this diff to preallocate the array capacity:
let mut models: Array<Span<felt252>> = array![]; + models.reserve(indexes.len());
875-907: Consistent loop usage for better maintainabilityOhayo, sensei! To maintain consistency and improve readability, consider refactoring the manual
loopin theset_entitiesfunction to aforloop.Apply this diff to refactor the loop:
let mut i = 0; loop { if i >= indexes.len() { break; } self.set_entity_internal(model_selector, *indexes[i], *values[i], layout); i += 1; };Replace with:
+ for i in 0..indexes.len() { + self.set_entity_internal(model_selector, *indexes[i], *values[i], layout); + }
921-939: Simplify iteration indelete_entitiesfunctionOhayo, sensei! Streamline the iteration over
indexesin thedelete_entitiesfunction by using a directforloop, enhancing code clarity.Apply this diff to simplify the loop:
for i in indexes { self.delete_entity_internal(model_selector, *i, layout); }Replace with:
+ for index in indexes { + self.delete_entity_internal(model_selector, *index, layout); + }
1183-1210: Enhance match expression with concise patternsOhayo, sensei! In the
get_entity_internalfunction, consider using concise patterns in thematchexpression for improved readability.Apply this diff to streamline the match cases:
match index { ModelIndex::Keys(keys) => { let entity_id = entity_id_from_keys(keys); storage::entity_model::read_model_entity(model_selector, entity_id, layout) }, ModelIndex::Id(entity_id) => { storage::entity_model::read_model_entity(model_selector, entity_id, layout) }, ModelIndex::MemberId(( entity_id, member_id )) => { storage::entity_model::read_model_member( model_selector, entity_id, member_id, layout ) } }Replace with:
+ match index { + ModelIndex::Keys(keys) => { + let entity_id = entity_id_from_keys(keys); + storage::entity_model::read_model_entity(model_selector, entity_id, layout) + }, + ModelIndex::Id(entity_id) => storage::entity_model::read_model_entity(model_selector, entity_id, layout), + ModelIndex::MemberId((entity_id, member_id)) => storage::entity_model::read_model_member( + model_selector, entity_id, member_id, layout + ), + }crates/dojo/world/src/contracts/abigen/world.rs (1)
3228-3247: Ohayo, sensei! Consider using slices instead of references to vectors in function parameters.In the functions
entities,delete_entities_getcall,emit_events_getcall,set_entities_getcall, and theentitiesmethod inWorldContractReader, parameters likeindexes: &Vec<ModelIndex>andvalues: &Vec<Vec<starknet::core::types::Felt>>are used. It's more idiomatic in Rust to accept slices, such as&[ModelIndex]or&[Vec<starknet::core::types::Felt>], for read-only access. This provides greater flexibility and can improve performance.Here's how you might refactor one of these functions:
- pub fn entities( - &self, - model_selector: &starknet::core::types::Felt, - indexes: &Vec<ModelIndex>, - layout: &Layout, - ) -> cainome::cairo_serde::call::FCall<A::Provider, Vec<Vec<starknet::core::types::Felt>>> { + pub fn entities( + &self, + model_selector: &starknet::core::types::Felt, + indexes: &[ModelIndex], + layout: &Layout, + ) -> cainome::cairo_serde::call::FCall<A::Provider, Vec<Vec<starknet::core::types::Felt>>> { // function body remains the same }Applying this change to similar function signatures enhances code flexibility.
Also applies to: 3338-3354, 3455-3471, 3809-3827, 4079-4098
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (25)
Cargo.toml(0 hunks)bin/sozo/src/commands/events.rs(1 hunks)bin/sozo/tests/test_data/policies.json(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/event/event.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/world.cairo(0 hunks)crates/dojo/core/src/event/event.cairo(0 hunks)crates/dojo/core/src/event/interface.cairo(0 hunks)crates/dojo/core/src/lib.cairo(0 hunks)crates/dojo/core/src/model/model.cairo(0 hunks)crates/dojo/core/src/model/model_value.cairo(0 hunks)crates/dojo/core/src/model/storage.cairo(4 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/iworld.cairo(5 hunks)crates/dojo/core/src/world/storage.cairo(12 hunks)crates/dojo/core/src/world/world_contract.cairo(5 hunks)crates/dojo/lang/src/attribute_macros/event.rs(1 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(0 hunks)crates/dojo/world/src/contracts/abigen/world.rs(6 hunks)crates/torii/core/src/processors/event_message.rs(1 hunks)crates/torii/types-test/src/contracts.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
💤 Files with no reviewable changes (8)
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/world/world.cairo
- crates/dojo/core/src/event/event.cairo
- crates/dojo/core/src/event/interface.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (34)
crates/dojo/core-cairo-test/src/lib.cairo (2)
50-50: Ohayo! The storage module addition looks good, sensei!
The new storage module under world aligns well with the PR's objective of introducing batch entity operations. This will help ensure the new storage functionality is properly tested.
Line range hint 19-21: Consider uncommenting related test modules for comprehensive coverage.
Ohayo sensei! I notice that the model and benchmarks modules are commented out. Since this PR introduces significant changes to entity handling and storage, it might be beneficial to uncomment and update these test modules to ensure comprehensive test coverage, especially for:
model/model.cairo: To test model-related functionality with the new batch operationsbenchmarks: To measure performance improvements from batch operations
Let's check if these modules exist in the repository:
Also applies to: 42-43
crates/dojo/core-cairo-test/src/tests/world/storage.cairo (2)
1-3: Ohayo sensei! The imports look clean and focused.
The necessary dependencies are properly imported for the storage tests.
81-128: Great test coverage for batch operations, sensei!
The test effectively verifies batch operations on non-copiable types with complex fields (arrays and strings), which aligns well with the PR's objective of optimizing multiple entity operations.
bin/sozo/tests/test_data/policies.json (2)
91-93: Ohayo sensei! New batch operations align perfectly with PR objectives.
The introduction of emit_events, set_entities, and delete_entities methods supports the PR's goal of reducing overhead for batch operations. These methods share the same target address (0x4bf...bdc) as their single-entity counterparts, maintaining consistency.
Also applies to: 99-101, 107-109
3-3: Verify upgrade target address changes.
Multiple upgrade method targets have been modified. Let's verify the consistency of these changes across the codebase.
Also applies to: 7-7, 39-39, 43-43, 127-127
crates/torii/types-test/src/contracts.cairo (2)
Line range hint 18-25: Ohayo! Nice work on enhancing the RecordLogged event structure!
The addition of Serde trait derivation aligns perfectly with the PR objective of ensuring event structure inclusion in the ABI. The key annotations are well-placed for proper event indexing.
Line range hint 126-127: Reconsider the subrecord_id calculation approach
The current approach of record_id + 1 for subrecord_id seems brittle and could lead to incorrect deletions if the IDs don't follow this pattern.
Let's verify the ID relationship pattern:
Consider storing the subrecord_id relationship explicitly or querying it from an index.
crates/dojo/lang/src/attribute_macros/event.rs (2)
13-22: Ohayo! LGTM on the import cleanup, sensei!
The import statements have been properly cleaned up after removing the historical parameter functionality.
Line range hint 1-140: Verify the impact of removing historical event tracking
The removal of historical event tracking aligns with the PR objectives, but we should verify the impact on existing consumers.
Let's verify the usage:
Consider documenting this architectural change in:
- Migration guides for existing consumers
- Event handling documentation
- Torii configuration examples, since historical tracking is moving there
✅ Verification successful
Let me gather more information about the event system changes:
The previous results show that historical event handling has moved to Torii, but let's verify the documentation and migration paths for users.
Let me try a different approach to find documentation and migration info:
The previous search revealed that historical events are now handled in Torii, but we need to verify if there's proper documentation and examples for users migrating from the old system.
Ohayo sensei! Let me check one more thing about the event system changes:
The previous results show that historical event tracking has been moved to Torii's responsibility, but let's verify if there are any migration notes or breaking changes documented.
Ohayo sensei! Based on the findings, I can now provide a conclusive response:
Historical event tracking removal is properly handled by Torii
The removal of historical event tracking from the core is safe because:
- Torii now handles historical events through dedicated tables (
event_messages_historical) - Migration scripts are in place (
20241028234131_event_message_historical.sql) - The functionality is fully implemented in Torii's event processing system
No additional documentation is needed as this is an internal architectural change that doesn't affect external consumers - they will continue to access historical events through Torii's interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to historical events
rg -i "historical.*event"
# Search for potential event consumers that might be affected
ast-grep --pattern 'fn $name($$$) {
$$$
emit_event($$$)
$$$
}'
Length of output: 1947
Script:
#!/bin/bash
# Check for event-related changes in migration files
fd migration sql$ --exec cat {}
# Look for event system documentation
fd -e md -e txt | grep -i "event" | xargs cat
# Check for event-related configuration examples
fd -g "*config*" -g "*example*" | grep -i "event" | xargs cat
Length of output: 334
Script:
#!/bin/bash
# Check for documentation files
fd -e md | xargs cat
# Look for examples directory content
fd -t f -d 3 example
# Check specific paths for configuration
rg -l "torii.*config"
Length of output: 45408
Script:
# Check for event-related changes in the codebase
rg -A 5 "event.*historical" crates/torii/
# Look for any migration or breaking changes documentation
fd -e md | grep -i "migration\|changelog" | xargs cat
Length of output: 6428
bin/sozo/src/commands/events.rs (1)
Line range hint 1-392: Verify event batch processing implementation.
The PR objectives mention batch entities support, but I don't see any direct changes in this file related to batch processing of events. Let's verify if we need additional changes to support batch operations.
crates/dojo/core/src/model/storage.cairo (11)
23-24: Efficient batch write method added
Ohayo, sensei! The addition of write_models enhances the capability to handle multiple models efficiently, aligning with the PR objectives to reduce overhead. The method signature and documentation look correct.
29-32: Verify trait constraints for non-copiable models
Ohayo, sensei! The comment mentions handling models "even if the type is not copiable." Ensure that Array<M> can handle non-copiable types and that M satisfies the necessary trait bounds (Drop and Serde).
Would you like to confirm that M implements Drop and Serde traits required by Array<M>?
37-39: Batch erase method improves efficiency
Ohayo, sensei! Introducing erase_models aligns with the goal of reducing overhead by allowing batch deletion of models. The method is well-documented and correctly implemented.
56-60: Batch read enhances performance
Ohayo, sensei! The read_values method adds batch retrieval of model values, which should improve performance significantly. The method signature and generic constraints appear correct.
64-66: Consistency in method naming
Ohayo, sensei! The method read_values_from_ids is a great addition. For consistency, ensure similar methods follow the same naming pattern across the traits.
Would you like to review other method names for consistency?
70-74: Batch update method for model values
Ohayo, sensei! The write_values function effectively adds batch update capability for model values, aligning with the goal to reduce overhead. The use of spans for keys and values is appropriate.
78-79: Efficient batch write from IDs
Ohayo, sensei! The write_values_from_ids method allows updating multiple values directly via entity IDs, enhancing efficiency.
89-90: Test trait extended for batch writes
Ohayo, sensei! Adding write_models_test to the ModelStorageTest trait ensures batch operations are testable, which is excellent for maintaining code quality.
93-94: Batch erase in tests improves coverage
Ohayo, sensei! The inclusion of erase_models_test allows for comprehensive testing of batch delete operations.
108-111: Batch write values in tests
Ohayo, sensei! The write_values_test method addition is great for testing batch updates of model values, ensuring robustness of the new functionality.
114-115: Completing test trait with batch write from IDs
Ohayo, sensei! The write_values_from_ids_test method rounds out the batch operation methods in the test trait, providing full test coverage.
crates/dojo/core/src/world/iworld.cairo (2)
130-131: Ohayo, sensei! Simplified emit_event function signature
The removal of the historical argument from emit_event streamlines the method and aligns with the updated event handling strategy.
311-311: Alignment of emit_event_test with updated emit_event
The emit_event_test function now reflects the updated signature of emit_event, maintaining consistency across the interface.
crates/dojo/core/src/world/storage.cairo (9)
128-143: Apply Loop Refactoring to write_models Method
Ohayo sensei! Similar to the previous suggestion, the write_models method can benefit from using a for loop instead of manual index management for improved readability.
154-166: Refactor Loops in erase_models for Consistency
Ohayo sensei! The loop in erase_models also follows the same pattern. Refactoring it with a for loop will maintain consistency across your batch operation methods.
182-200: Streamline Loop in erase_models_ptrs Method
Ohayo sensei! In erase_models_ptrs, consider refactoring the loop to enhance clarity and consistency with other methods.
231-278: Refactor Loop Using for in read_values Methods
Ohayo sensei! The loops in read_values and read_values_from_ids can be refactored to use for loops for better readability.
Line range hint 293-337: Optimize Loop Structures in write_values Methods
Ohayo sensei! Refactoring the loops in write_values and write_values_from_ids with for loops will enhance code clarity and maintain consistency.
395-400: Refactor erase_models_test to Use Batch Deletion
Ohayo sensei! Similar to the previous suggestion, erase_models_test can leverage the erase_models method for batch deletion to improve test efficiency.
418-443: Batch Deletion in erase_models_ptrs_test
Ohayo sensei! In erase_models_ptrs_test, consider refactoring to use a batch deletion approach if possible, to maintain consistency and improve performance in tests.
459-469: Enhance write_values_test with Batch Operations
Ohayo sensei! The write_values_test method can be refactored to use batch writing operations, aligning with the implementation in the main code.
484-496: Refactor Loops in write_values_from_ids_test for Clarity
Ohayo sensei! For improved readability, consider refactoring the loop in write_values_from_ids_test to a for loop.
crates/dojo/world/src/contracts/abigen/world.rs (1)
244-244: Ohayo, sensei! Confirm removal of the historical field in EventEmitted.
At line 244, the EventEmitted struct is instantiated without the historical field, consistent with the AI summary stating its removal. Please verify that all usages of EventEmitted across the codebase have been updated to reflect this change to prevent potential deserialization errors.
You can run the following script to identify any outdated usages:
✅ Verification successful
Ohayo! The historical field has been properly removed from all EventEmitted usages, sensei!
The search results show that all EventEmitted struct definitions and instantiations across the codebase are consistent with the new structure, containing only selector, system_address, keys, and values fields. No instances of the historical field were found in relation to EventEmitted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find instances where `EventEmitted` is used with the `historical` field.
# Search for `EventEmitted` struct initializations containing `historical`
rg "EventEmitted\s*\{[^}]*historical\s*:" -A 5
Length of output: 48
Script:
#!/bin/bash
# Let's try a broader search to find all EventEmitted struct usages and definitions
# Search for EventEmitted struct definitions
echo "=== EventEmitted struct definitions ==="
ast-grep --pattern 'struct EventEmitted {
$$$
}'
# Search for EventEmitted instantiations
echo -e "\n=== EventEmitted instantiations ==="
rg "EventEmitted\s*\{" -A 3
# Search for any mentions of 'historical' near EventEmitted
echo -e "\n=== 'historical' mentions near EventEmitted ==="
rg -B 2 -A 2 "EventEmitted.*historical|historical.*EventEmitted"
Length of output: 3877
| let historical = false; | ||
| db.set_event_message(entity, event_id, block_timestamp, historical).await?; |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! After analyzing the codebase, I can now provide a clear response about the configuration suggestion.
Consider using existing IndexingFlags for historical configuration
The hardcoded historical = false can leverage the existing configuration infrastructure. The EngineConfig already has a flags system (IndexingFlags) that's used for similar toggles like TRANSACTIONS and RAW_EVENTS. Adding a new flag for historical events would be consistent with the current architecture.
Example locations that need attention:
crates/torii/core/src/engine.rs: Add new flag constant likeHISTORICAL_EVENTStoIndexingFlagscrates/torii/core/src/processors/event_message.rs: Replace hardcodedfalsewith flag check
🔗 Analysis chain
Consider implementing configuration now
The hardcoded historical = false aligns with the PR's objective of removing historical aspects. However, since this will eventually need configuration support, consider implementing it now to avoid technical debt.
Let's check if there's existing configuration infrastructure we can leverage:
Let me search for any existing configuration infrastructure specifically related to event processing.
Let me check the implementation of EngineConfig to see if it already has support for historical events configuration.
Let me try a different approach to check the EngineConfig implementation.
Let me check what IndexingFlags contains since it's part of EngineConfig.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for configuration-related code in Torii
rg -g '!target' -g '!*.lock' -A 5 'config' crates/torii/
Length of output: 24032
Script:
#!/bin/bash
# Search for configuration related to event processing and historical data
rg -g '!target' -g '!*.lock' "EngineConfig|struct.*Config" crates/torii/core/src/
Length of output: 537
Script:
#!/bin/bash
# Check EngineConfig implementation
ast-grep --pattern 'struct EngineConfig {
$$$
}' crates/torii/core/src/engine.rs
Length of output: 84
Script:
#!/bin/bash
# Check EngineConfig implementation using ripgrep with context
rg -g '!target' -g '!*.lock' -A 10 'pub struct EngineConfig' crates/torii/core/src/
Length of output: 720
Script:
#!/bin/bash
# Check IndexingFlags implementation
rg -g '!target' -g '!*.lock' -A 10 'IndexingFlags' crates/torii/core/src/
Length of output: 4264
| if i % 2 == 0 { | ||
| let foo = Foo { caller, a: felt, b: i }; | ||
| models_snaps.append(@foo); | ||
| } else { | ||
| let foo = Foo { caller, a: felt, b: i }; | ||
| models_snaps.append(@foo); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify redundant conditional block, sensei!
The if-else block contains identical code. You can remove it entirely since both branches perform the same operation.
- if i % 2 == 0 {
- let foo = Foo { caller, a: felt, b: i };
- models_snaps.append(@foo);
- } else {
- let foo = Foo { caller, a: felt, b: i };
- models_snaps.append(@foo);
- }
+ let foo = Foo { caller, a: felt, b: i };
+ models_snaps.append(@foo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if i % 2 == 0 { | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); | |
| } else { | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); | |
| } | |
| let foo = Foo { caller, a: felt, b: i }; | |
| models_snaps.append(@foo); |
| if i % 2 == 0 { | ||
| let foo = NotCopiable { caller, a: array![felt], b: "ab" }; | ||
| models_snaps.append(@foo); | ||
| } else { | ||
| let foo = NotCopiable { caller, a: array![felt], b: "ab" }; | ||
| models_snaps.append(@foo); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate code block, sensei!
Similar to the previous test, this if-else block contains identical code paths.
- if i % 2 == 0 {
- let foo = NotCopiable { caller, a: array![felt], b: "ab" };
- models_snaps.append(@foo);
- } else {
- let foo = NotCopiable { caller, a: array![felt], b: "ab" };
- models_snaps.append(@foo);
- }
+ let foo = NotCopiable { caller, a: array![felt], b: "ab" };
+ models_snaps.append(@foo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if i % 2 == 0 { | |
| let foo = NotCopiable { caller, a: array![felt], b: "ab" }; | |
| models_snaps.append(@foo); | |
| } else { | |
| let foo = NotCopiable { caller, a: array![felt], b: "ab" }; | |
| models_snaps.append(@foo); | |
| } | |
| let foo = NotCopiable { caller, a: array![felt], b: "ab" }; | |
| models_snaps.append(@foo); |
| pub fn delete_entities_getcall( | ||
| &self, | ||
| model_selector: &starknet::core::types::Felt, | ||
| indexes: &Vec<ModelIndex>, | ||
| layout: &Layout, | ||
| ) -> starknet::core::types::Call { | ||
| use cainome::cairo_serde::CairoSerde; | ||
| let mut __calldata = vec![]; | ||
| __calldata.extend(starknet::core::types::Felt::cairo_serialize(model_selector)); | ||
| __calldata.extend(Vec::<ModelIndex>::cairo_serialize(indexes)); | ||
| __calldata.extend(Layout::cairo_serialize(layout)); | ||
| starknet::core::types::Call { | ||
| to: self.address, | ||
| selector: starknet::macros::selector!("delete_entities"), | ||
| calldata: __calldata, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Add input validation for matching lengths in batch operations.
The batch functions delete_entities_getcall, emit_events_getcall, and set_entities_getcall do not validate whether the lengths of input vectors like indexes and values (where applicable) match. This could lead to unexpected behavior or runtime errors if mismatched inputs are provided.
Consider adding checks to ensure that the lengths of these vectors are equal and handle discrepancies appropriately.
Also applies to: 3455-3471, 3809-3827
Add to the world the capabilities to batch entities, which reduce the overhead of permissions check when several entities are set/delete at once.
Ensure the event structure is in the ABI.
Historical is removed from the chain and will be implemented in Torii configuration.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Refactor
Chores