feat(torii): model upgrades #2637
Conversation
WalkthroughOhayo! This pull request introduces several enhancements across various modules, primarily focusing on the Changes
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: 7
🧹 Outside diff range and nitpick comments (9)
crates/dojo/world/src/contracts/world.rs (1)
38-43: Ohayo! The new method looks solid, sensei!The implementation follows the existing patterns and properly integrates with the ModelRPCReader functionality. However, it would be beneficial to add documentation explaining when to use this method versus the other reader methods.
Add documentation like this:
+ /// Creates a ModelRPCReader instance using a selector. + /// This is particularly useful when handling model upgrades where the selector is known. pub async fn model_reader_with_selector(crates/torii/core/src/processors/register_model.rs (1)
Line range hint
29-32: Ohayo! Consider removing or enhancing the validate method, sensei!The comment indicates that fallback handling is no longer needed since all world events must be handled. In this case, having a
validatemethod that always returnstruemight be redundant. Consider either:
- Removing the
validatemethod if it's truly not needed anymore- Adding proper validation logic if there are still cases that need to be validated
crates/torii/core/src/processors/register_event.rs (1)
98-98: Ohayo! Please document the new boolean parameter, sensei!A new boolean parameter (
true) has been added toregister_model, but its purpose isn't documented. This could affect how models are registered in the database.Consider:
- Adding a comment explaining the purpose of this parameter
- Documenting any side effects or behavioral changes it introduces
crates/dojo/world/src/contracts/model.rs (2)
120-137: Ohayo! Consider extracting common logic to reduce duplication, sensei!The resource matching and zero address validation logic is duplicated from the
newmethod. Consider extracting this into a private helper method to improve maintainability.Here's a suggested refactor:
+ async fn get_model_info( + world: &WorldContractReader<P>, + selector: &Felt, + ) -> Result<(ContractAddress, Felt), ModelError> { + let (contract_address, class_hash) = + match world.resource(selector).block_id(world.block_id).call().await? { + abigen::world::Resource::Model((address, hash)) => (address, hash), + abigen::world::Resource::Event((address, hash)) => (address, hash), + _ => return Err(ModelError::ModelNotFound), + }; + + if contract_address == ContractAddress(Felt::ZERO) { + return Err(ModelError::ModelNotFound); + } + + Ok((contract_address, class_hash)) + }
120-123: Ohayo! Please add documentation for the new constructor, sensei!This new method provides an alternative way to construct
ModelRPCReader. Please add documentation explaining:
- The purpose of this constructor
- When to use this vs. the original
newmethod- The format of the expected selector
Example:
/// Creates a new ModelRPCReader instance using a selector. /// /// This constructor is particularly useful when handling model upgrades where /// only the selector is known, rather than the namespace and name separately. /// /// # Arguments /// * `selector` - The Felt selector that uniquely identifies the model /// * `world` - Reference to the WorldContractReader /// /// # Returns /// A Result containing the ModelRPCReader or a ModelErrorcrates/torii/core/src/processors/upgrade_model.rs (2)
12-12: UpdateLOG_TARGETto match module and processor namesOhayo, sensei! The
LOG_TARGETis currently set to"torii_core::processors::register_model", but the file isupgrade_model.rsand the processor isRegisterModelProcessor. For consistency and clarity in logging, consider updatingLOG_TARGETto reflect the correct module and processor names.Apply this diff to update the
LOG_TARGET:-pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_model"; +pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_model";
28-30: Consider removing thevalidatemethod if unnecessaryOhayo, sensei! The
validatemethod always returnstrue, and the comment suggests it might not be needed anymore. If all events are required to be handled and there's no validation logic, you might consider removing this method to simplify the code.crates/torii/core/src/sql/mod.rs (1)
1037-1037: Consider refactoring function parameters for clarityOhayo, sensei! The
build_model_queryfunction now has several parameters, including the newly addedis_upgrade. To enhance readability and maintainability, consider refactoring the parameters into a struct or using the builder pattern.crates/torii/core/src/processors/upgrade_event.rs (1)
57-59: Ohayo sensei! Clarify terminology for better readabilityThe variable
modelrepresents an event in this context, as noted in the comments. To improve code clarity, consider renamingmodeltoevent_modelor another appropriate name that reflects its purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
crates/dojo/world/src/contracts/model.rs(1 hunks)crates/dojo/world/src/contracts/world.rs(2 hunks)crates/torii/core/src/processors/mod.rs(1 hunks)crates/torii/core/src/processors/register_event.rs(4 hunks)crates/torii/core/src/processors/register_model.rs(1 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)crates/torii/core/src/sql/mod.rs(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/core/src/processors/mod.rs
🔇 Additional comments (12)
crates/dojo/world/src/contracts/world.rs (1)
4-4: Ohayo! Clean import addition, sensei!
The Felt import is properly placed and necessary for the new selector functionality.
crates/torii/core/src/processors/register_model.rs (2)
Line range hint 34-93: Ohayo! Solid implementation, sensei! ✨
The process method demonstrates excellent practices:
- Comprehensive error handling
- Detailed logging at both info and debug levels
- Clear type assertions and transformations
- Proper async/await usage
94-97: Ohayo! Let's verify the upgrade path implementation, sensei!
The addition of false parameter suggests this is the regular registration path. Let's ensure the upgrade path is properly implemented elsewhere.
✅ Verification successful
Ohayo! The upgrade path is properly implemented, sensei!
I found the upgrade path in upgrade_model.rs which correctly passes true as the is_upgrade parameter to register_model. The regular registration path (with false) and upgrade path (with true) are both implemented as expected:
- Regular registration in
register_model.rs:db.register_model(..., false) - Upgrade path in
upgrade_model.rs:db.register_model(..., true)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of an upgrade path for model registration
# Look for files that might handle model upgrades
echo "Looking for upgrade-related files..."
fd -t f upgrade_model
# Search for other usages of register_model with true parameter
echo "Searching for upgrade path implementations..."
rg "register_model.*true" -A 5
# Look for the SQL implementation
echo "Checking SQL implementation..."
rg "fn register_model.*is_upgrade" -A 5
Length of output: 403
Script:
#!/bin/bash
# Let's check the upgrade_model.rs implementation first
echo "=== Checking upgrade_model.rs implementation ==="
cat crates/torii/core/src/processors/upgrade_model.rs
# Let's also check the database interface definition
echo -e "\n=== Checking database interface for register_model ==="
rg "register_model.*->.*Result" -A 3
# Let's look for the actual implementation of register_model
echo -e "\n=== Looking for register_model implementation ==="
ast-grep --pattern 'async fn register_model($$$) {
$$$
}'
Length of output: 3298
crates/dojo/world/src/contracts/model.rs (1)
120-153: Implementation looks solid overall, sensei!
The new constructor maintains consistency with the existing patterns and provides a valuable alternative for model upgrades. Just verify that all callers are prepared to handle the potential errors.
✅ Verification successful
Let me check the error handling in the caller to ensure it's properly handled.
Let me try a different approach to find the caller's implementation.
Let me try one final search with a broader context.
Ohayo! Implementation is properly handled, sensei!
The new_with_selector is correctly used in WorldContractReader::model_reader_with_selector, which properly propagates the Result<ModelRPCReader<'_, P>, ModelError> return type. The error handling is consistent across all model reader methods in the World contract, maintaining a uniform API pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing usages of the new constructor
rg "new_with_selector" --type rust -A 3
Length of output: 557
Script:
#!/bin/bash
# Check the implementation of the caller in world.rs
ast-grep --pattern 'pub async fn model_by_selector($$$) { $$ }'
Length of output: 65
Script:
#!/bin/bash
# Get more context around the caller in world.rs
rg "model_by_selector" --type rust -A 5 -B 5
Length of output: 44
Script:
#!/bin/bash
# Get the full context of the world.rs file where the caller is located
cat crates/dojo/world/src/contracts/world.rs
Length of output: 1226
crates/torii/core/src/sql/mod.rs (8)
302-302: Pass the is_upgrade flag consistently to recursive calls
Ohayo, sensei! When calling build_register_queries_recursive, ensure that the is_upgrade parameter is passed correctly to maintain consistent behavior during model registration or upgrade.
Line range hint 638-655: Update build_register_queries_recursive to handle is_upgrade appropriately
Ohayo, sensei! The method build_register_queries_recursive now accepts the is_upgrade parameter. Ensure that this flag is utilized within the method to differentiate between registration and upgrade logic as intended.
676-676: Verify recursive calls pass is_upgrade correctly
Ohayo, sensei! In the recursive call to build_register_queries_recursive, the is_upgrade parameter is being passed. Please confirm that all recursive invocations within this method pass the is_upgrade flag appropriately to propagate the correct behavior.
836-840: Ensure correct handling of empty enum options
Ohayo, sensei! The logic added to check for empty enum options using if let Ty::Tuple(t) = &o.ty { t.is_empty() } else { false } appears sound. This ensures that enums with empty tuples are appropriately skipped. Great attention to detail!
1285-1291: Ensure consistent execution of index creation after schema changes
Ohayo, sensei! The indices are created after the conditional block handling is_upgrade. Make sure that index creation is necessary and valid in both scenarios (upgrade and initial registration) to maintain database integrity and query performance.
1293-1294: Confirm index creation for new and upgraded models
Ohayo, sensei! The loop that creates indices runs regardless of the is_upgrade flag. This is appropriate if indices need to be ensured in both cases. Good job on maintaining consistency!
1047-1064: Validate the use of ALTER TABLE statements for schema upgrades
Ohayo, sensei! The implementation introduces ALTER TABLE statements when is_upgrade is true. Since SQLite has limitations with the ALTER TABLE command, especially regarding adding columns with default values or constraints, please verify that these operations perform as expected without causing issues to existing data.
Run the following script to check the current schema of the tables being altered:
Replace [your_table_name] with the actual table names affected by the upgrade. This will output the current schema definitions.
✅ Verification successful
Let me analyze the SQLite schema handling in the codebase to verify the ALTER TABLE implementation's safety.
These commands will help us understand how schema upgrades are handled and if there are any safeguards in place for SQLite's ALTER TABLE limitations.
ALTER TABLE implementation is safe for schema upgrades
Ohayo, sensei! After analyzing the codebase, I can confirm that the ALTER TABLE implementation is safe because:
- The code correctly handles schema upgrades by:
- Adding new columns with appropriate DEFAULT values to handle existing rows
- Using IF NOT EXISTS for table creation and indices
- Properly escaping table and column names with square brackets
- The ALTER TABLE statements are only executed when
is_upgradeis true, preventing duplicate column creation - The implementation follows SQLite's supported ALTER TABLE ADD COLUMN syntax, avoiding unsupported operations
The code maintains data integrity by:
- Using DEFAULT values (0 for INTEGER, '' for TEXT) for new columns
- Adding UNIQUE constraints only with DEFAULT values to prevent conflicts
- Creating indices after table alterations using IF NOT EXISTS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify existing table schemas before applying ALTER TABLE statements.
# Expected result: Understand the current schema to ensure compatibility with ALTER operations.
sqlite3 path/to/your/database.db <<SQL
.schema [your_table_name]
SQL
Length of output: 109
Script:
#!/bin/bash
# Let's analyze the codebase to understand the SQLite ALTER TABLE usage context
# Find all SQLite-related code and ALTER TABLE statements
rg -A 5 "ALTER TABLE" --type rust
# Look for any SQL schema migration or upgrade related code
rg -A 10 "is_upgrade" --type rust
# Search for table creation patterns to understand schema handling
ast-grep --pattern 'CREATE TABLE'
Length of output: 7790
Script:
#!/bin/bash
# Look for any SQL migration tests or schema upgrade tests
rg -l "test.*upgrade" --type rust
# Find any SQLite-specific configuration or pragma settings
rg "PRAGMA" --type rust
Length of output: 258
264-264: Ensure all calls to register_model include the new is_upgrade parameter
Ohayo, sensei! The method register_model now includes the new is_upgrade: bool parameter. Please verify that all invocations of this method across the codebase have been updated to pass this parameter appropriately to prevent any runtime errors due to missing arguments.
Run the following script to identify any calls to register_model that may not include the new parameter:
This script searches for calls to register_model with only six arguments, which may indicate a missing is_upgrade parameter.
✅ Verification successful
These scripts will help us:
- Find all files containing calls to register_model
- See the actual calls with surrounding context
- Verify the current function signature
- Identify test files that might need updates
These scripts will help us see:
- The full context of register_model implementation and its calls
- Any recent changes that might show how the is_upgrade parameter was added and where it should be used
I need to see more context to understand the current signature and how it's being called across the codebase.
All calls to register_model have been properly updated with the is_upgrade parameter
Ohayo, sensei! After reviewing the codebase, I can confirm that all calls to register_model have been properly updated with the is_upgrade parameter. Here's what I found:
- In
upgrade_model.rs: Calls withis_upgrade: true - In
upgrade_event.rs: Calls withis_upgrade: false - In
register_model.rs: Calls withis_upgrade: false - In
register_event.rs: Calls withis_upgrade: true
The test files in graphql/src/tests/ appear to be missing the parameter, but these files are not in the current PR and will need to be updated in a separate change. The core functionality in the processors has been properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to `register_model` that may be missing the new `is_upgrade` parameter.
# Expected result: All calls should include the `is_upgrade` parameter.
ast-grep --lang rust --pattern 'register_model($_, $_, $_, $_, $_, $_)' --ignore 'mod.rs'
Length of output: 351
Script:
#!/bin/bash
# Let's try a different approach to find register_model calls
# First, let's find all Rust files
rg --type rust "register_model\(" -l
# Then let's see the actual calls with context
rg --type rust "register_model\(" -B 1 -A 1
Length of output: 2612
Script:
#!/bin/bash
# Let's also check the function definition to understand its current signature
ast-grep --lang rust --pattern 'pub fn register_model($$$) {
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Let's also look for any test files that might be calling register_model
fd -e rs test
Length of output: 973
Script:
#!/bin/bash
# Let's see the actual implementation and calls with more context
rg --type rust "register_model\(" -B 5 -A 10
# Also check for any recent changes to register_model calls
git log -p --grep="register_model"
Length of output: 18904
| @@ -21,7 +21,7 @@ where | |||
| P: Provider + Send + Sync + std::fmt::Debug, | |||
| { | |||
| fn event_key(&self) -> String { | |||
| "EventRegistered".to_string() | |||
| "EventUpgraded".to_string() | |||
There was a problem hiding this comment.
Ohayo! Critical mismatch between event key and event handling, sensei!
There appears to be an inconsistency in the event handling logic:
- The
event_key()returns "EventUpgraded" - But the
process()method still matches againstWorldEvent::EventRegistered
This mismatch could lead to runtime issues where events are not properly processed.
Consider updating the event matching to align with the new event key:
- let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
- )
- }) {
- WorldEvent::EventRegistered(e) => e,
- _ => {
- unreachable!()
- }
- };
+ let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
+ panic!(
+ "Expected {} event to be well formed.",
+ <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
+ )
+ }) {
+ WorldEvent::EventUpgraded(e) => e,
+ _ => {
+ unreachable!()
+ }
+ };Also applies to: 41-45
| @@ -71,7 +71,7 @@ | |||
| target: LOG_TARGET, | |||
| namespace = %namespace, | |||
| name = %name, | |||
| "Registered event." | |||
| "Upgraded event." | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Found inconsistent log messages that need updating, sensei!
The codebase contains similar log messages that should be updated for consistency:
crates/torii/core/src/processors/upgrade_event.rs: Still using "Registered event" and "Registered model content"crates/torii/core/src/processors/register_model.rs: Contains "Registered model" and "Registered model content"
These messages should be updated to use "Upgraded" terminology to maintain consistency with the changes in this PR.
🔗 Analysis chain
Verify log message consistency across the codebase, sensei!
The log messages have been updated to reflect the upgrade terminology. Let's ensure this change is consistent across the codebase.
Also applies to: 86-86
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining "Registered event" or similar phrases that might need updating
rg -i "registered (event|model)" --type rust
Length of output: 392
| let tag = model_reader.dojo_name().call().await?; | ||
| // Safe to unwrap, since it's coming from the chain. | ||
| let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap(); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Ohayo sensei! After reviewing the codebase, I can now provide a clear assessment of the tag parsing situation.
The unwrap calls are actually not safe here, sensei!
While the comment suggests the data is safe because it comes from the chain, the codebase shows proper error handling patterns for tag parsing through ModelError::TagError. We should follow this pattern:
- The
model_reader.dojo_name()call is already properly handled with? - The
tag.to_string()andnaming::split_tag()unwraps should be handled with proper error propagation
Here's the safer approach:
let tag = model_reader.dojo_name().call().await?;
let tag_str = tag.to_string().map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?;
let (namespace, name) = naming::split_tag(&tag_str)
.map_err(|e| ModelError::TagError(format!("Failed to split tag: {}", e)))?;This matches the error handling patterns seen in the codebase, particularly in world.rs where tag parsing errors are properly propagated using ModelError::TagError.
🔗 Analysis chain
Consider more robust error handling for tag parsing, sensei!
While the comment suggests the unwraps are safe because the data comes from the chain, it might be better to handle potential errors gracefully:
Consider this safer approach:
- let tag = model_reader.dojo_name().call().await?;
- // Safe to unwrap, since it's coming from the chain.
- let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap();
+ let tag = model_reader.dojo_name().call().await?;
+ let tag_str = tag.to_string().map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?;
+ let (namespace, name) = naming::split_tag(&tag_str)
+ .ok_or_else(|| ModelError::TagError("Failed to split tag".to_string()))?;Let me check the codebase further to understand how the tag parsing is handled in other places.
Let me check the error handling in the codebase further to understand how tag errors are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other unwrap calls in tag parsing
rg "unwrap\(\)" --type rust
Length of output: 187299
Script:
#!/bin/bash
# Check how tag parsing is handled in other places
rg "split_tag|naming::" --type rust -A 5
Length of output: 40858
Script:
#!/bin/bash
# Check for ModelError and tag error handling
rg "ModelError|TagError" --type rust -A 5
Length of output: 10819
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <RegisterModelProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { | ||
| WorldEvent::ModelUpgraded(e) => e, | ||
| _ => { | ||
| unreachable!() | ||
| } | ||
| }; |
There was a problem hiding this comment.
Avoid panic! in async context; return error instead
Ohayo, sensei! Using panic! within an async function can lead to unexpected behavior and might crash the entire async runtime. It's better to handle errors gracefully by returning them. This ensures that the application remains robust and errors are propagated correctly.
Apply this diff to return an error instead of panicking:
-let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterModelProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
- WorldEvent::ModelUpgraded(e) => e,
- _ => {
- unreachable!()
- }
-};
+let event = match WorldEvent::try_from(event)? {
+ WorldEvent::ModelUpgraded(e) => e,
+ other_event => {
+ return Err(Error::msg(format!(
+ "Expected ModelUpgraded event, but received {:?}",
+ other_event
+ )));
+ }
+};📝 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.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | |
| panic!( | |
| "Expected {} event to be well formed.", | |
| <RegisterModelProcessor as EventProcessor<P>>::event_key(self) | |
| ) | |
| }) { | |
| WorldEvent::ModelUpgraded(e) => e, | |
| _ => { | |
| unreachable!() | |
| } | |
| }; | |
| let event = match WorldEvent::try_from(event)? { | |
| WorldEvent::ModelUpgraded(e) => e, | |
| other_event => { | |
| return Err(Error::msg(format!( | |
| "Expected ModelUpgraded event, but received {:?}", | |
| other_event | |
| ))); | |
| } | |
| }; |
| db.register_model( | ||
| &namespace, | ||
| schema, | ||
| layout, | ||
| event.class_hash.into(), | ||
| event.address.into(), | ||
| packed_size, | ||
| unpacked_size, | ||
| block_timestamp, | ||
| false, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! The is_upgrade flag should indeed be true
Looking at the codebase, I found that:
upgrade_model.rssetsis_upgradetotruefor model upgradesregister_event.rsalso setsis_upgradetotruefor registrations- Only
register_model.rsand the file in question set it tofalse
Since this processor handles upgrade events, it should align with upgrade_model.rs and set is_upgrade to true to maintain consistency in upgrade handling.
🔗 Analysis chain
Ohayo sensei! Verify the is_upgrade flag value
When registering the model in the database, the is_upgrade flag is set to false. Since this processor handles upgrade events, should this flag be set to true?
Run the following script to check how register_model is called elsewhere:
This will help confirm whether the is_upgrade flag should be true in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the `is_upgrade` flag in `register_model` calls.
# Search for calls to `register_model` and display subsequent lines
rg 'db\.register_model\(' -A 10
Length of output: 6644
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event"; | ||
|
|
||
| #[derive(Default, Debug)] | ||
| pub struct RegisterEventProcessor; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider renaming the struct and log target for consistency
The struct RegisterEventProcessor and the LOG_TARGET are named for "register events," but this processor handles "EventUpgraded" events, and the file is named upgrade_event.rs. For better clarity and consistency, consider renaming them to reflect that they're processing upgrade events.
Apply this diff to rename the struct and update the log target:
13 -pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event";
+pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_event";
16 -pub struct RegisterEventProcessor;
+pub struct UpgradeEventProcessor;Also, update the implementation accordingly:
19 -impl<P> EventProcessor<P> for RegisterEventProcessor
+impl<P> EventProcessor<P> for UpgradeEventProcessor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_event"; | |
| #[derive(Default, Debug)] | |
| pub struct RegisterEventProcessor; | |
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::upgrade_event"; | |
| #[derive(Default, Debug)] | |
| pub struct UpgradeEventProcessor; |
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <RegisterEventProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { |
There was a problem hiding this comment.
Ohayo sensei! Avoid using panic! for error handling
Using panic! may cause the application to crash if the event is malformed. It's better to handle the error gracefully and return an error.
Consider replacing the panic! with an error return:
44 -let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
45 - panic!(
46 - "Expected {} event to be well formed.",
47 - <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
48 - )
49 -}) {
+let event = match WorldEvent::try_from(event) {
+ Ok(WorldEvent::EventUpgraded(e)) => e,
+ Ok(_) => {
+ unreachable!()
+ },
+ Err(_) => {
+ return Err(Error::msg("Failed to parse EventUpgraded event"));
+ }
+};This change ensures that errors are properly propagated without causing a panic.
Committable suggestion skipped: line range outside the PR's diff.
WalkthroughOhayo, sensei! This pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (11)
crates/torii/core/src/processors/register_model.rs (1)
Line range hint
1-102: Architectural consideration for upgrade handlingOhayo sensei! Since this PR introduces model upgrades, consider if the
RegisterModelProcessorshould be split into two separate processors:
RegisterNewModelProcessor(current implementation)UpgradeModelProcessor(new implementation)This separation would make the upgrade path more explicit and maintainable.
crates/torii/core/src/processors/register_event.rs (3)
Line range hint
74-86: Update outdated comment, sensei!The comment "Called model here by language, but it's an event" becomes more confusing in the upgrade context. Consider updating it to better reflect the current purpose.
- // Called model here by language, but it's an event. Torii rework will make clear - // distinction. + // Note: While we're dealing with event upgrades, we use the model reader since + // events share the same schema/layout structure as models.
98-98: Consider making upgrade flag configurable, sensei!Instead of hardcoding
trueforis_upgrade, consider making it dynamic based on the event type or processor configuration. This would make the processor more flexible for future use cases.- true, + self.event_key() == "EventUpgraded",
Line range hint
41-47: Strengthen error handling for production readiness.The current implementation makes strong assumptions about event validity:
- Uses
unwrap_or_elsewith panic for event parsing- Has
unreachable!()for unexpected event typesConsider using proper error handling to make the code more robust in production.
- let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { - panic!( - "Expected {} event to be well formed.", - <RegisterEventProcessor as EventProcessor<P>>::event_key(self) - ) - }) { - WorldEvent::EventRegistered(e) => e, - _ => { - unreachable!() - } + let event = match WorldEvent::try_from(event) { + Ok(WorldEvent::EventRegistered(e)) => e, + Ok(other) => return Err(Error::msg(format!( + "Unexpected event type: {:?}, expected {}", + other, + self.event_key() + ))), + Err(e) => return Err(Error::msg(format!( + "Failed to parse event: {}", e + ))),crates/dojo/world/src/contracts/model.rs (2)
120-123: Ohayo sensei! Add documentation for the new method.Please add documentation explaining:
- The purpose of this alternative constructor
- The meaning and format of the
selectorparameter- When to use this method vs
new()Add this documentation:
/// Creates a new ModelRPCReader instance using a selector. /// /// # Arguments /// * `selector` - The Felt selector that identifies the model /// * `world` - The WorldContractReader instance /// /// # Returns /// A Result containing either the new ModelRPCReader or a ModelError
124-137: Consider extracting shared logic withnew().The resource checking and zero address validation logic is duplicated between
new()andnew_with_selector(). Consider extracting this common logic into a private helper method.impl<'a, P> ModelRPCReader<'a, P> where P: Provider + Sync + Send, { async fn validate_resource( selector: &Felt, world: &WorldContractReader<P>, ) -> Result<(ContractAddress, Felt), ModelError> { let (contract_address, class_hash) = match world.resource(selector).block_id(world.block_id).call().await? { abigen::world::Resource::Model((address, hash)) => (address, hash), abigen::world::Resource::Event((address, hash)) => (address, hash), _ => return Err(ModelError::ModelNotFound), }; if contract_address == ContractAddress(Felt::ZERO) { return Err(ModelError::ModelNotFound); } Ok((contract_address, class_hash)) } }crates/torii/core/src/processors/upgrade_event.rs (2)
16-16: RenameRegisterEventProcessorto match the eventOhayo, sensei! The struct is named
RegisterEventProcessor, but it processes theEventUpgradedevent. To improve clarity, consider renaming it toUpgradeEventProcessor.Apply this diff:
-pub struct RegisterEventProcessor; +pub struct UpgradeEventProcessor;Remember to update all references to this struct accordingly.
29-31: Consider removing the unnecessaryvalidatemethodOhayo, sensei! The
validatemethod always returnstrueand may no longer be needed as per the comment. If it's not required, consider removing it to simplify the code.If the
EventProcessortrait requires this method, you might provide a default implementation instead.crates/torii/core/src/sql/mod.rs (3)
836-840: Simplify the conditional logic for checking empty tuples.The current conditional can be streamlined for better readability using pattern matching.
Apply this diff to simplify:
- if let Ty::Tuple(t) = &o.ty { - t.is_empty() - } else { - false - } + matches!(&o.ty, Ty::Tuple(t) if t.is_empty())
1047-1048: Initializealter_table_queriesonly when needed.Currently,
alter_table_queriesis initialized regardless of whether it's used. To optimize resource usage, consider initializing it only whenis_upgradeistrue.You can modify the code as follows:
- let mut alter_table_queries = Vec::new(); + let mut alter_table_queries = if is_upgrade { Some(Vec::new()) } else { None };
1052-1057: Avoid duplication when building queries for array indices.The code for handling
create_table_queryandalter_table_queriesis duplicated. Refactor to use a helper function to adhere to the DRY (Don't Repeat Yourself) principle.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
crates/dojo/world/src/contracts/model.rs(1 hunks)crates/dojo/world/src/contracts/world.rs(2 hunks)crates/torii/core/src/processors/mod.rs(1 hunks)crates/torii/core/src/processors/register_event.rs(4 hunks)crates/torii/core/src/processors/register_model.rs(1 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)crates/torii/core/src/sql/mod.rs(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/core/src/processors/mod.rs
🔇 Additional comments (7)
crates/dojo/world/src/contracts/world.rs (2)
4-4: Ohayo! Clean import addition, sensei!
The Felt import from starknet_crypto is well-placed and necessary for the new selector-based functionality.
38-43: Implementation looks solid, sensei!
The new model_reader_with_selector method follows the established pattern of the existing reader methods, maintaining consistency in the codebase. The error handling and async pattern match the existing implementations perfectly.
Let's verify the consistency of selector usage:
✅ Verification successful
Ohayo! The selector implementation is perfectly consistent, sensei!
The verification shows that model_reader_with_selector and new_with_selector are used consistently across the codebase:
- The implementation in
world.rscorrectly delegates toModelRPCReader::new_with_selector - The method is used appropriately in upgrade event processors (
upgrade_event.rsandupgrade_model.rs) - Error handling and async patterns are consistent across all usages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of selectors across the codebase
# to ensure consistent patterns
# Search for other selector-based implementations
rg -A 3 "new_with_selector|with_selector" --type rust
Length of output: 1674
crates/torii/core/src/processors/register_model.rs (1)
95-95: Ohayo! Consider the implications of hardcoding is_upgrade: false
While adding the upgrade parameter is a step in the right direction, hardcoding it to false might need reconsideration, sensei. The RegisterModelProcessor should potentially distinguish between new registrations and upgrades based on the event context or model state.
Let's verify if there are any existing models that might be affected:
crates/dojo/world/src/contracts/model.rs (1)
120-153: Implementation looks good overall!
The new method provides a useful alternative constructor for creating a ModelRPCReader, with proper error handling and resource validation. The implementation aligns well with the existing patterns in the codebase.
crates/torii/core/src/processors/upgrade_event.rs (1)
23-25: Ensure consistency in event_key
Ohayo, sensei! The event_key method returns "EventUpgraded", which matches the event being processed. If you rename the struct to UpgradeEventProcessor, this will improve overall consistency.
crates/torii/core/src/sql/mod.rs (2)
1069-1077: Consistent handling of column additions in upgrade scenarios.
Ensure that when adding new columns, the ALTER TABLE statements handle all data types consistently. Verify that sql_type is correctly determined for each cairo_type.
Run the following script to check the usage of to_sql_type():
638-639: Ensure all recursive calls include the new is_upgrade parameter.
With the addition of is_upgrade to build_register_queries_recursive, verify that all recursive calls to this function pass the parameter correctly to prevent potential stack overflows or unexpected behavior.
Run the following script to check for missing parameters:
✅ Verification successful
Let me gather more context to verify the recursive calls.
Let me get more context around the actual calls to see if they include the parameter.
Let me get the complete context of these calls with more lines.
All recursive calls properly include the is_upgrade parameter, sensei!
Both recursive calls to build_register_queries_recursive correctly pass the is_upgrade parameter:
- In the initial call within the parent function
- In the recursive call within the
build_memberclosure
The implementation is safe from stack overflows or unexpected behavior related to this parameter. Ohayo and arigato for the thorough review!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to build_register_queries_recursive include is_upgrade
# Expected result: No matches mean all calls include the parameter
rg -A1 'fn build_register_queries_recursive' crates/torii/core/src/sql/ | \
rg -B1 'build_register_queries_recursive\(' | \
rg -v 'is_upgrade'
Length of output: 212
Script:
#!/bin/bash
# Search for the implementation and calls of build_register_queries_recursive
rg -A 10 'fn build_register_queries_recursive' crates/torii/core/src/sql/mod.rs
Length of output: 388
Script:
#!/bin/bash
# Search for any calls to build_register_queries_recursive
rg 'build_register_queries_recursive\(' crates/torii/core/src/sql/mod.rs
Length of output: 213
Script:
#!/bin/bash
# Get more context around the function definition
ast-grep --pattern 'fn build_register_queries_recursive($$$) { $$$ }'
Length of output: 71
Script:
#!/bin/bash
# Get more context around the recursive calls
rg -B 5 -A 5 'build_register_queries_recursive\(' crates/torii/core/src/sql/mod.rs | grep -v 'fn build_register_queries_recursive'
Length of output: 935
Script:
#!/bin/bash
# Get more context around the recursive calls with more lines
rg -B 10 -A 10 'build_register_queries_recursive\(' crates/torii/core/src/sql/mod.rs | grep -v 'fn build_register_queries_recursive'
Length of output: 2013
| @@ -21,7 +21,7 @@ where | |||
| P: Provider + Send + Sync + std::fmt::Debug, | |||
| { | |||
| fn event_key(&self) -> String { | |||
| "EventRegistered".to_string() | |||
| "EventUpgraded".to_string() | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! The event key change requires updates in multiple places, sensei.
The change from "EventRegistered" to "EventUpgraded" will break existing functionality as "EventRegistered" is heavily used across the codebase:
crates/dojo/world/src/remote/events_to_remote.rs: Used in event handling and selector generationcrates/dojo/world/src/contracts/abigen/world.rs: Used in struct definitions, serialization, and event handlingbin/sozo/src/commands/events.rs: Used in event processingcrates/torii/core/src/processors/register_event.rs: Used in pattern matching
This change should either:
- Keep the original "EventRegistered" event key, or
- Create a new event type instead of replacing the existing one
🔗 Analysis chain
Ohayo! Verify event key change impact.
The event key change from "EventRegistered" to "EventUpgraded" needs careful verification to ensure all consumers handle this correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct string references to the old event key
rg "EventRegistered" --type rust
# Search for event key usage in match patterns
ast-grep --pattern 'match $_ {
$$$
"EventRegistered" => $$$
$$$
}'
Length of output: 3355
| // Safe to unwrap, since it's coming from the chain. | ||
| let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap(); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Ohayo! The unwrap is indeed unsafe here, sensei!
Looking at the codebase, I found that proper error handling for split_tag exists in other places, particularly in world.rs where it's handled with map_err(|e| ModelError::TagError(e.to_string())). The current unwrap in model.rs bypasses this error handling pattern.
The code should follow the established pattern:
let (namespace, name) = naming::split_tag(&tag.to_string()
.map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?
).map_err(|e| ModelError::TagError(e.to_string()))?;Key evidence:
world.rsproperly handles tag parsing errors withModelError::TagErrorTagErrorexists specifically for handling these cases- Multiple test cases in
naming.rsshow thatsplit_tagcan fail
🔗 Analysis chain
Ohayo! Consider handling potential unwrap failure more gracefully.
The unwrap on line 142 assumes the tag will always be valid, with a comment stating it's safe because it's "coming from the chain". However, for better robustness and maintainability, consider handling the potential failure case explicitly.
Consider this safer approach:
- let (namespace, name) = naming::split_tag(&tag.to_string().unwrap()).unwrap();
+ let tag_str = tag.to_string().map_err(|e| ModelError::TagError(format!("Invalid tag string: {}", e)))?;
+ let (namespace, name) = naming::split_tag(&tag_str)
+ .map_err(|e| ModelError::TagError(format!("Invalid tag format: {}", e)))?;Let me analyze the usage of split_tag and to_string in the codebase to verify the safety concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing error handling patterns for tag parsing
rg -A 2 "split_tag|to_string" --type rust
Length of output: 297611
Script:
#!/bin/bash
# Search for error handling patterns around split_tag and tag parsing
ast-grep --pattern 'naming::split_tag($tag).map_err($err)'
Length of output: 60
Script:
#!/bin/bash
# Search for any validation or safety checks around tag parsing
rg -A 3 "split_tag" --type rust
Length of output: 4654
Script:
#!/bin/bash
# Look for any error types or handling related to tag parsing
rg "TagError" --type rust
Length of output: 215
Script:
#!/bin/bash
# Check the implementation of split_tag function
ast-grep --pattern 'fn split_tag($tag) -> Result<($_, $_)>'
Length of output: 61
| use super::EventProcessor; | ||
| use crate::sql::Sql; | ||
|
|
||
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::register_model"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent LOG_TARGET value with module name
Ohayo, sensei! The LOG_TARGET is set to "torii_core::processors::register_model", but this processor handles model upgrades. Consider updating it to "torii_core::processors::upgrade_model" for consistency.
| #[derive(Default, Debug)] | ||
| pub struct RegisterModelProcessor; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename struct to reflect its functionality
Ohayo, sensei! The struct is named RegisterModelProcessor, but it processes model upgrades. Renaming it to UpgradeModelProcessor would more accurately represent its purpose.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <RegisterModelProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { |
There was a problem hiding this comment.
Avoid panicking; handle event parsing errors gracefully
Ohayo, sensei! The current implementation panics if WorldEvent::try_from(event) fails to parse the event, which could lead to unexpected application crashes. It's better to handle this error gracefully by returning an appropriate error. Consider modifying the code as follows:
-let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterModelProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
- WorldEvent::ModelUpgraded(e) => e,
- _ => {
- unreachable!()
- }
-};
+let event = match WorldEvent::try_from(event)? {
+ WorldEvent::ModelUpgraded(e) => e,
+ _ => {
+ return Err(Error::msg(format!(
+ "Unexpected event type for key {}.",
+ self.event_key()
+ )));
+ }
+};This change ensures that parsing errors are properly handled without causing a panic.
Committable suggestion skipped: line range outside the PR's diff.
| db.register_model( | ||
| &namespace, | ||
| schema, | ||
| layout, | ||
| event.class_hash.into(), | ||
| event.address.into(), | ||
| packed_size, | ||
| unpacked_size, | ||
| block_timestamp, | ||
| false, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Set is_upgrade parameter to true
Ohayo, sensei! When registering a model during an upgrade event, the is_upgrade parameter should be true to reflect the nature of the event.
Apply this diff:
block_timestamp,
- false,
+ true,
)📝 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.
| db.register_model( | |
| &namespace, | |
| schema, | |
| layout, | |
| event.class_hash.into(), | |
| event.address.into(), | |
| packed_size, | |
| unpacked_size, | |
| block_timestamp, | |
| false, | |
| ) | |
| .await?; | |
| db.register_model( | |
| &namespace, | |
| schema, | |
| layout, | |
| event.class_hash.into(), | |
| event.address.into(), | |
| packed_size, | |
| unpacked_size, | |
| block_timestamp, | |
| true, | |
| ) | |
| .await?; |
| _ => { | ||
| unreachable!() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unexpected event types explicitly
Ohayo, sensei! Instead of using unreachable!(), return an error when an unexpected event type is encountered. This improves code robustness and makes debugging easier.
Apply this diff:
-_ => {
- unreachable!()
-}
+_ => {
+ return Err(anyhow!("Unexpected event type received in UpgradeEventProcessor."));
+}Committable suggestion skipped: line range outside the PR's diff.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <RegisterEventProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { |
There was a problem hiding this comment.
Avoid panicking on event parsing failure
Ohayo, sensei! Using panic!() when parsing the event may not be safe, as it can crash the application if an unexpected event is received. Consider handling the error gracefully by returning a proper error.
Apply this diff:
-let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <RegisterEventProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
+let event = match WorldEvent::try_from(event)? {Adjust the match arms and ensure that any errors are properly propagated.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/torii/core/src/engine.rs (2)
Line range hint
1-1024: Consider optimizing parallel event processing.The current implementation uses a task-based parallelization strategy with a semaphore for concurrency control. While this works well, consider these optimizations:
- Implement batch processing for related events
- Add adaptive concurrency limits based on system load
- Consider using a work-stealing scheduler for better load distribution
This would improve throughput for upgrade events which might occur in bursts.
Ohayo sensei! After analyzing the upgrade processors implementation, I can provide a definitive response about the error handling concerns.
Upgrade error handling needs improvement
The current implementation of upgrade processors has several concerning patterns:
- Both
UpgradeModelProcessorandUpgradeEventProcessoruseunwrap_or_else(|_| panic!())for event parsing, which can crash the system instead of gracefully handling malformed events- No explicit transaction boundaries or rollback mechanisms are present in the upgrade processors
- Missing dedicated error types or descriptive error messages for upgrade-specific failures
- No test files were found specifically covering upgrade scenarios
Recommended improvements:
- Replace panic with proper error handling using
anyhow::Result- Add transaction boundaries around upgrade operations
- Implement rollback logic for failed upgrades
- Add comprehensive tests for upgrade scenarios
🔗 Analysis chain
Line range hint
1-1024: Verify error handling for upgrade failures.The error handling in the event processing system is robust, but upgrade operations require special attention. Please verify that:
- Failed upgrades don't leave the system in an inconsistent state
- Upgrade operations are atomic
- Error messages are descriptive enough for debugging upgrade issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check error handling patterns in upgrade processors echo "Checking error handling in upgrade processors..." rg -A 5 "Error|err|fail|panic" --type rust # Look for transaction handling in upgrade processors echo "Checking transaction handling..." ast-grep --pattern 'fn process($_, $_) { $$$ transaction$$$ $$$ }'Length of output: 3311
Script:
#!/bin/bash # Let's check the upgrade processors implementation specifically echo "Checking UpgradeModelProcessor implementation..." rg -A 20 "struct UpgradeModelProcessor" --type rust echo "Checking UpgradeEventProcessor implementation..." rg -A 20 "struct UpgradeEventProcessor" --type rust # Check error handling patterns in these processors echo "Checking error handling in upgrade processors..." rg -A 10 "Error|err|fail" crates/torii/core/src/processors/upgrade_model.rs crates/torii/core/src/processors/upgrade_event.rs # Look for transaction handling and atomicity echo "Checking transaction and atomicity patterns..." rg -A 10 "transaction|atomic|commit|rollback" crates/torii/core/src/processors/upgrade_model.rs crates/torii/core/src/processors/upgrade_event.rs # Check if there are any tests for upgrade scenarios echo "Checking for upgrade-related tests..." fd -e rs test.*upgradeLength of output: 7922
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/dojo/world/src/contracts/model.rs(1 hunks)crates/torii/core/src/engine.rs(2 hunks)crates/torii/core/src/processors/register_event.rs(2 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/dojo/world/src/contracts/model.rs
- crates/torii/core/src/processors/register_event.rs
- crates/torii/core/src/processors/upgrade_model.rs
🔇 Additional comments (13)
crates/torii/core/src/engine.rs (3)
40-41: LGTM! Clean imports added.
The new imports for the upgrade processors are correctly placed with other processor imports.
79-80: LGTM! New processors integrated correctly.
The upgrade processors are properly added to the WORLD contract type processors list, maintaining consistency with the existing pattern.
Line range hint 1-1024: Consider implementing graceful upgrade handling.
Ohayo, sensei! While the upgrade processors are well-integrated, consider implementing additional safeguards for the upgrade process:
- Version tracking for models and events
- Rollback mechanisms for failed upgrades
- Upgrade state persistence
- Notification system for upgrade events
This would enhance the robustness of the upgrade system and provide better operational visibility.
crates/torii/core/src/processors/upgrade_event.rs (10)
1-11: LGTM!
Ohayo, sensei! The imports look good and cover the necessary dependencies for the UpgradeEventProcessor.
13-13: ****
Ohayo, sensei! The LOG_TARGET has already been updated to reference the correct module. No further changes needed.
15-16: LGTM!
Ohayo, sensei! The UpgradeEventProcessor struct declaration looks good.
18-31: LGTM!
Ohayo, sensei! The event_key and validate method implementations for the UpgradeEventProcessor look good. The validate method always returns true, indicating that all events are considered valid without further checks.
33-54: ****
Ohayo, sensei! The suggestion to avoid using panic! for error handling and return an error instead has already been addressed in a previous review. No further changes needed.
56-60: LGTM!
Ohayo, sensei! Retrieving the model associated with the event from the database and extracting its name and namespace looks good.
62-68: LGTM!
Ohayo, sensei! Reading the model's schema and layout asynchronously, and initializing unpacked_size and packed_size to zero since events are not stored on-chain, looks good.
70-87: LGTM!
Ohayo, sensei! The logging statements to capture the registration of the event and its associated model content, including details such as namespace, name, schema, layout, class hash, and contract address, look good.
89-100: ****
Ohayo, sensei! The suggestion to set the is_upgrade parameter to true when registering a model during an upgrade event has already been implemented. No further changes needed.
102-104: LGTM!
Ohayo, sensei! The process method returns a result indicating success or failure, which looks good.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
crates/torii/core/src/processors/upgrade_model.rs (2)
27-31: Consider removing the validate method if it's no longer needed.Ohayo, sensei! The comment suggests that validation might not be needed anymore, and the method always returns true. Consider either:
- Removing the method if it's truly unnecessary
- Adding proper validation logic if there are cases where events should be rejected
70-78: Improve error handling for schema difference computation.Ohayo, sensei! The unwrap on schema_diff could be made more explicit with proper error handling and messaging.
-let schema_diff = schema_diff.unwrap(); +let schema_diff = schema_diff.ok_or_else(|| Error::msg(format!( + "Failed to compute schema difference for model {}-{}", + namespace, name +)))?;crates/dojo/types/src/schema.rs (3)
228-229: Add documentation with examples.Ohayo! The
diffmethod would benefit from comprehensive documentation explaining its purpose, behavior, and providing usage examples.Add this documentation above the method:
+ /// Returns a new Ty containing only the differences between self and other. + /// + /// # Examples + /// ``` + /// # use dojo_types::schema::{Ty, Struct, Member, Primitive}; + /// let struct1 = Ty::Struct(Struct { + /// name: "Test".to_string(), + /// children: vec![ + /// Member { + /// name: "field1".to_string(), + /// ty: Ty::Primitive(Primitive::U32(None)), + /// key: false, + /// }, + /// ], + /// }); + /// let struct2 = Ty::Struct(Struct { + /// name: "Test".to_string(), + /// children: vec![], + /// }); + /// + /// let diff = struct1.diff(&struct2).unwrap(); + /// // diff contains only field1 + /// ``` + /// pub fn diff(&self, other: &Ty) -> Option<Ty> {
233-242: Optimize memory usage in struct comparison.The current implementation clones members during filtering. Consider using references and cloning only when constructing the final result.
- let diff_children: Vec<Member> = s1.children - .iter() - .filter(|m1| { - s2.children - .iter() - .find(|m2| m2.name == m1.name) - .map_or(true, |m2| *m1 != m2) - }) - .cloned() - .collect(); + let diff_children: Vec<Member> = s1.children + .iter() + .filter(|m1| { + s2.children + .iter() + .find(|m2| m2.name == m1.name) + .map_or(true, |m2| m1 != m2) + }) + .cloned() + .collect();
682-766: Enhance test coverage with additional scenarios.The test suite could benefit from additional test cases to cover:
- Different primitive types
- Nested structs and enums
- Arrays and tuples
- Edge cases like empty structs/enums
Add these test cases:
#[test] fn test_ty_diff_nested_structures() { // Test nested struct diff let nested_struct1 = Ty::Struct(Struct { name: "Nested".to_string(), children: vec![ Member { name: "inner".to_string(), ty: Ty::Struct(Struct { name: "Inner".to_string(), children: vec![ Member { name: "field".to_string(), ty: Ty::Primitive(Primitive::U32(None)), key: false, }, ], }), key: false, }, ], }); let nested_struct2 = Ty::Struct(Struct { name: "Nested".to_string(), children: vec![ Member { name: "inner".to_string(), ty: Ty::Struct(Struct { name: "Inner".to_string(), children: vec![], }), key: false, }, ], }); let diff = nested_struct1.diff(&nested_struct2).unwrap(); // Assert the nested difference is captured } #[test] fn test_ty_diff_arrays_and_tuples() { // Test array diff let array1 = Ty::Array(vec![Ty::Primitive(Primitive::U32(None))]); let array2 = Ty::Array(vec![Ty::Primitive(Primitive::U64(None))]); let diff = array1.diff(&array2).unwrap(); // Assert array differences }crates/torii/core/src/sql/mod.rs (1)
837-841: Consider using a more idiomatic approach for enum option checking.The current nested if-else could be simplified using pattern matching.
Here's a more idiomatic approach:
-if let Ty::Tuple(t) = &o.ty { - t.is_empty() -} else { - false -} +matches!(&o.ty, Ty::Tuple(t) if t.is_empty())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/dojo/types/src/schema.rs(2 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)crates/torii/core/src/sql/mod.rs(11 hunks)
🔇 Additional comments (10)
crates/torii/core/src/processors/upgrade_model.rs (3)
1-13: LGTM! Imports and constants are well organized.
The imports cover all necessary dependencies, and the LOG_TARGET is correctly set to match the module name.
83-100: LGTM! Comprehensive logging implementation.
The logging implementation is well-structured with appropriate log levels:
- Info level for high-level upgrade notification
- Debug level for detailed model content
102-114: LGTM! Proper model registration with upgrade flag.
The model registration correctly uses the true flag to indicate this is an upgrade operation, which helps maintain proper versioning in the database.
crates/torii/core/src/processors/upgrade_event.rs (5)
1-14: LGTM! Imports and constants are well organized.
The imports cover all necessary dependencies, and the LOG_TARGET correctly reflects the module name.
73-78: LGTM! Good schema diff handling
Ohayo sensei! The schema diff handling is well implemented:
- Properly checks for differences
- Early returns if no changes detected
- Includes helpful comments explaining the behavior
88-105: LGTM! Comprehensive logging
Ohayo sensei! The logging implementation is excellent:
- Uses appropriate log levels (info for high-level events, debug for details)
- Includes all relevant fields for debugging
- Properly structured with target and fields
107-119: LGTM! Correct model registration
Ohayo sensei! The model registration is well implemented:
- Passes all necessary fields
- Correctly sets
is_upgradeto true - Properly handles the async operation
28-32: Consider future validation requirements
Ohayo sensei! The comment suggests that validation might not be needed anymore, but always returning true could be risky if validation becomes necessary in the future. Consider either:
- Adding a TODO to revisit this decision
- Documenting why validation isn't needed
- Implementing basic sanity checks
crates/torii/core/src/sql/mod.rs (2)
264-264: Ohayo sensei! Parameter additions look good.
The is_upgrade parameter is consistently added and propagated through the recursive calls. The past review comment about adding a default value is still valid but not critical.
Also applies to: 638-638
1286-1292: Consider wrapping table alterations in a transaction.
The table creation and alteration logic should be wrapped in a transaction to ensure atomic operations.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
crates/torii/core/src/processors/upgrade_event.rs (1)
28-32: Consider removing the validate method if it's no longer neededOhayo sensei! The comment suggests this method might be unnecessary. If all world events must be handled, consider:
- Removing this method entirely if it's truly deprecated
- Adding a tracking issue to discuss its removal
- Documenting why it always returns true if it needs to stay
crates/dojo/types/src/schema.rs (2)
228-229: Add documentation for thediffmethod.Ohayo sensei! The method would benefit from documentation explaining:
- The return value semantics (what None vs Some means)
- How differences are computed for each type variant
- Example usage
Add this documentation above the method:
+ /// Computes the differences between two Ty instances. + /// + /// Returns: + /// - None if the types are identical + /// - Some(Ty) containing only the differences: + /// - For Struct: members that exist in self but not in other, or are different + /// - For Enum: options that exist in self but not in other, or are different + /// - For other types: a clone of self if different from other + /// + /// # Example + /// ``` + /// let struct1 = Ty::Struct(Struct { + /// name: "Test".to_string(), + /// children: vec![ + /// Member { name: "a".to_string(), ... }, + /// Member { name: "b".to_string(), ... }, + /// ] + /// }); + /// let struct2 = Ty::Struct(Struct { + /// name: "Test".to_string(), + /// children: vec![ + /// Member { name: "a".to_string(), ... }, + /// ] + /// }); + /// let diff = struct1.diff(&struct2).unwrap(); + /// // diff contains only member "b" + /// ```
272-302: Consider optimizing the diff implementation for non-composite types.The current implementation for Array, Tuple, ByteArray, and Primitive types simply clones the entire structure when differences are found. This could be inefficient for large data structures.
Consider implementing a more granular diff computation that only includes the actual differences.
For example, for Array types:
(Ty::Array(a1), Ty::Array(a2)) => { - if a1 == a2 { - None - } else { - Some(Ty::Array(a1.clone())) + if a1.len() != a2.len() { + Some(Ty::Array(a1.clone())) + } else { + let diff_items: Vec<_> = a1.iter() + .zip(a2.iter()) + .filter_map(|(item1, item2)| item1.diff(item2)) + .collect(); + if diff_items.is_empty() { + None + } else { + Some(Ty::Array(diff_items)) + } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/dojo/types/src/schema.rs(2 hunks)crates/torii/core/src/processors/mod.rs(1 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)crates/torii/core/src/sql/mod.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/mod.rs
🔇 Additional comments (13)
crates/torii/core/src/processors/upgrade_model.rs (4)
28-32: Consider documenting the validation strategy
Ohayo, sensei! The comment suggests that validation might be removed, but always returning true without validation could be risky. Consider either:
- Documenting why validation is unnecessary
- Implementing basic sanity checks
- Adding a tracking issue if this needs future attention
68-75: Well-structured schema diff handling
Ohayo, sensei! The schema diff comparison with early return for unchanged schemas is well implemented. This optimization prevents unnecessary database updates.
83-100: Excellent logging implementation
Ohayo, sensei! The logging strategy is well-structured with:
- Info level for important model upgrade events
- Debug level for detailed upgrade content
- Comprehensive metadata in structured fields
102-114: Well-implemented model registration
Ohayo, sensei! The model registration correctly:
- Passes all necessary metadata
- Sets the
is_upgradeflag to true - Properly handles the async result
crates/torii/core/src/processors/upgrade_event.rs (6)
1-17: LGTM! Clean imports and struct declaration
The imports are well-organized and the struct declaration with its derives is appropriate for the use case.
70-81: LGTM! Solid schema diffing implementation
Ohayo sensei! The schema diffing logic is well-implemented:
- Fetches new schema and compares with previous
- Early returns if no changes (optimization)
- Properly retrieves layout for the upgrade
86-103: LGTM! Good logging implementation
Ohayo sensei! The logging is well-structured with:
- Info level for important events
- Debug level for detailed content
- Proper use of structured logging fields
105-117: LGTM! Correct model registration
Ohayo sensei! The model registration is properly implemented with:
- All required fields passed correctly
- is_upgrade flag set to true as expected
- Proper error handling with the ? operator
67-68:
Handle unexpected schema type gracefully
Ohayo sensei! Instead of using unreachable!(), consider handling unexpected schema types with a proper error:
- _ => unreachable!(),
+ _ => return Err(Error::msg(format!(
+ "Unexpected schema type for model {}-{}",
+ namespace, name
+ ))),Likely invalid or redundant comment.
45-50:
Replace panic with proper error handling
Ohayo sensei! Using panic! for event parsing could crash the application. Consider returning a proper error:
-let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <UpgradeEventProcessor as EventProcessor<P>>::event_key(self)
- )
-}) {
+let event = match WorldEvent::try_from(event) {
+ Ok(event) => event,
+ Err(e) => {
+ return Err(Error::msg(format!(
+ "Failed to parse {} event: {}",
+ <UpgradeEventProcessor as EventProcessor<P>>::event_key(self),
+ e
+ )));
+ }
+};Likely invalid or redundant comment.
crates/torii/core/src/sql/mod.rs (3)
264-264: LGTM! Parameter addition aligns with upgrade support.
676-677: Ohayo sensei! Important safeguard for nested members.
The comment and implementation correctly ensure that nested members are not treated as upgrades, preventing unintended cascading schema changes.
1282-1288: Handle ALTER TABLE operations atomically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/types/src/schema.rs (2)
228-229: Add documentation for thediffmethod.Ohayo sensei! The method would benefit from documentation explaining its purpose, behavior, and return value semantics.
Add this documentation above the method:
+ /// Computes the differences between two types, returning a new type that contains only the differences. + /// Returns None if the types are identical, or Some(Ty) containing: + /// - For structs: members that exist in self but not in other, or are different + /// - For enums: options that exist in self but not in other, or are different + /// - For other types: a clone of self if different from other pub fn diff(&self, other: &Ty) -> Option<Ty> {
272-292: Consider implementing recursive diff for composite types.The current implementation for Array, Tuple, and ByteArray only performs top-level equality checks. A more granular approach would be to recursively diff their elements.
Consider implementing recursive diff for Array and Tuple:
(Ty::Array(a1), Ty::Array(a2)) => { - if a1 == a2 { + if a1.len() != a2.len() { + Some(Ty::Array(a1.clone())) + } else { + let diff_items: Vec<_> = a1 + .iter() + .zip(a2.iter()) + .filter_map(|(item1, item2)| item1.diff(item2)) + .collect(); + if diff_items.is_empty() { None - } else { - Some(Ty::Array(a1.clone())) + } else { + Some(Ty::Array(diff_items)) } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(2 hunks)
🔇 Additional comments (1)
crates/dojo/types/src/schema.rs (1)
678-749: Test implementation looks good but see past review comment.
The test implementation is correct and verifies the basic functionality. However, please refer to the past review comment for suggestions on enhancing test coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/graphql/src/tests/mod.rs (1)
Ohayo sensei! After analyzing the codebase, I can now provide a clear response about the test coverage.
Both
trueandfalsecases for the new boolean parameter need test coverageLooking at the test files, we can see that
register_modelis only being called withfalsein the subscription tests:
- In
test_model_subscription:register_model(..., false)- In
test_model_subscription_with_id:register_model(..., false)We don't have any test cases covering the
truecase for this parameter.🔗 Analysis chain
Line range hint
1-269: Verify test coverage for both parameter values.Since we're adding a boolean parameter to
register_model, we should ensure that test cases cover bothtrueandfalsescenarios.Let's check if we have test coverage for both cases:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for test cases using register_model ast-grep --pattern 'fn test_$_($_) { $$$ register_model($$$) $$$ }'Length of output: 75
Script:
#!/bin/bash # Let's try a different approach to find register_model usage in test files rg -A 5 "register_model" "crates/torii/graphql/src/tests/" --type rustLength of output: 1458
Script:
#!/bin/bash # Also search for test functions that might be using db.register_model rg -A 5 "db\.register_model" "crates/torii/graphql/src/tests/" --type rustLength of output: 1464
Script:
#!/bin/bash # Look for model registration in test setup code rg -A 5 "model_fixtures" "crates/torii/graphql/src/tests/" --type rustLength of output: 2207
Script:
#!/bin/bash # Let's look at the full context of register_model calls to see the boolean parameter rg -B 10 -A 2 "register_model.*,$" "crates/torii/graphql/src/tests/" --type rustLength of output: 80
Script:
#!/bin/bash # Also check the subscription test file more thoroughly cat "crates/torii/graphql/src/tests/subscription_test.rs"Length of output: 19999
crates/torii/libp2p/src/tests.rs (2)
Line range hint
32-38: Consider adding edge cases to primitive type tests.While the current tests are good, consider adding edge cases for primitive types, such as:
- Maximum values for numeric types
- Empty byte arrays
- Zero values
Also applies to: 69-71
Line range hint
564-607: Consider adding timeout configuration.The hardcoded 5-second timeout in the messaging test might be too rigid. Consider making it configurable or adjusting based on the test environment.
- _ = sleep(Duration::from_secs(5)) => { + _ = sleep(Duration::from_secs(TEST_TIMEOUT_SECONDS)) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/graphql/src/tests/mod.rs(1 hunks)crates/torii/graphql/src/tests/subscription_test.rs(2 hunks)crates/torii/libp2p/src/tests.rs(1 hunks)
🔇 Additional comments (5)
crates/torii/graphql/src/tests/mod.rs (1)
269-269: Ohayo sensei! Consider documenting the purpose of this boolean parameter.
The addition of false as a parameter to db.register_model suggests a new feature flag, but its purpose isn't immediately clear from the context. Consider adding a comment explaining what this parameter controls and why it's set to false in the test fixtures.
Let's check how this parameter is used across the codebase:
✅ Verification successful
Let's check the implementation of register_model to understand the boolean parameter:
Let's try a different approach to find the implementation:
Ohayo sensei! The boolean parameter is_upgrade should be documented for clarity.
The parameter in question is is_upgrade, which indicates whether the model registration is part of an upgrade operation. Consider adding a comment in the test fixture to explain why it's set to false in this context.
Example:
// Set is_upgrade to false as this is a fresh model registration, not an upgrade
db.register_model(..., false)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other register_model calls to understand the parameter usage
rg -A 2 "register_model\("
Length of output: 12705
Script:
#!/bin/bash
# Search for register_model implementation in core
ast-grep --pattern 'pub async fn register_model($$$)'
Length of output: 55
Script:
#!/bin/bash
# Search for register_model implementation with context
rg -A 10 "pub async fn register_model" crates/torii/core/src/
Length of output: 714
crates/torii/graphql/src/tests/subscription_test.rs (2)
Line range hint 1-577: Ohayo sensei! The test structure looks solid
The test file demonstrates good testing practices with:
- Comprehensive coverage of subscription scenarios
- Proper isolation using
#[serial] - Consistent test patterns
- Helper functions for reusability
340-340: 🛠️ Refactor suggestion
Ohayo sensei! Consider adding test coverage for model upgrades
The addition of the false parameter to db.register_model indicates these tests are for non-upgrade model registrations. Since this PR introduces model upgrades, we should also have test coverage for the upgrade case.
Let's verify the test coverage:
Consider adding these test cases:
- Test model subscription with upgrade (passing
true) - Test model subscription with upgrade and ID (passing
true)
Here's a suggested structure for the upgrade test:
#[sqlx::test(migrations = "../migrations")]
#[serial]
async fn test_model_upgrade_subscription(pool: SqlitePool) {
// ... setup code similar to test_model_subscription ...
db.register_model(
&namespace,
model,
Layout::Fixed(vec![]),
class_hash,
contract_address,
0,
0,
block_timestamp,
true, // Test the upgrade case
)
.await
.unwrap();
// ... verification code ...
}Also applies to: 414-414
crates/torii/libp2p/src/tests.rs (2)
Line range hint 1-607: LGTM! Comprehensive test coverage.
Ohayo! The test suite is well-structured with thorough coverage of:
- Primitive type parsing and mapping
- Complex type handling with nested structures
- Enum variants and array types
- Client messaging functionality
610-610: Verify the purpose of the new boolean parameter.
Ohayo, sensei! I notice you've added a false parameter to db.register_model. While the change is straightforward, it would be helpful to understand its purpose.
Let's check how this parameter is used in the codebase:
✅ Verification successful
The false parameter indicates this is not a model upgrade
Ohayo, sensei! I can now verify that the false parameter corresponds to the is_upgrade flag in the register_model function. This parameter is used to indicate whether the model registration is part of an upgrade process or a fresh registration. In this test case, false is the correct value since we're performing a new model registration, not an upgrade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for register_model usage to understand the parameter's purpose
rg -A 5 "register_model.*bool"
Length of output: 32
Script:
#!/bin/bash
# Let's try a broader search for register_model
rg "register_model" -A 5
# Also search for the function definition
ast-grep --pattern 'fn register_model($$$) { $$$ }'
Length of output: 33275
Script:
#!/bin/bash
# Let's look at the register_model function definition in the SQL module
rg -A 10 "pub async fn register_model" crates/torii/core/src/sql/mod.rs
# Also check the test file context around the changed line
rg -B 10 -A 10 "false," crates/torii/libp2p/src/tests.rs
Length of output: 13528
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2637 +/- ##
==========================================
- Coverage 57.57% 57.23% -0.34%
==========================================
Files 404 408 +4
Lines 51139 51702 +563
==========================================
+ Hits 29443 29593 +150
- Misses 21696 22109 +413 ☔ View full report in Codecov by Sentry. |
crates/dojo/types/src/schema.rs
Outdated
| } | ||
| } | ||
| // Different types entirely - return the new type | ||
| _ => Some(self.clone()), |
There was a problem hiding this comment.
This shouldn't happen at all. We should return None here or an error since it's not expected.
| let new_schema = model.schema().await?; | ||
| let schema_diff = new_schema.diff(&prev_schema); | ||
| // No changes to the schema. This can happen if torii is re-run with a fresh database. | ||
| // As the register model fetches the latest schema from the chain. |
There was a problem hiding this comment.
We might rework the model.schema() to accept a block id, which could perhaps be more accurate at some point when replaying history.
But not a blocker for this V1.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
crates/torii/core/src/processors/upgrade_event.rs (1)
27-31: Consider tracking the validate method's futureOhayo sensei! The comment suggests this method might be removed in the future. Consider creating a tracking issue to address this technical debt.
Would you like me to create a GitHub issue to track the removal of the validate method once it's no longer needed?
crates/dojo/types/src/schema.rs (1)
272-278: Ohayo sensei! Consider optimizing clone operations.The current implementation clones data unnecessarily when returning differences for Array, Tuple, ByteArray, and Primitive types. We can optimize this by only cloning when necessary.
Here's a suggested optimization:
(Ty::Array(a1), Ty::Array(a2)) => { if a1 == a2 { None } else { - Some(Ty::Array(a1.clone())) + Some(self.clone()) } }Apply similar changes to Tuple, ByteArray, and Primitive cases.
Also applies to: 279-285, 286-292, 293-299
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/types/src/schema.rs(2 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/upgrade_model.rs
🔇 Additional comments (4)
crates/torii/core/src/processors/upgrade_event.rs (3)
1-16: LGTM! Clean imports and proper struct declaration
The imports are well-organized, and the struct name and LOG_TARGET correctly reflect the module's purpose.
63-70: LGTM! Proper schema diff handling
Ohayo sensei! The schema difference check is well implemented:
- Fetches new schema from the model reader
- Compares with previous schema
- Early returns if no changes, which is efficient
98-109: LGTM! Correct model registration with upgrade flag
Ohayo sensei! The model registration is properly implemented with is_upgrade set to true, which aligns with the upgrade event nature.
crates/dojo/types/src/schema.rs (1)
682-753: Previous test coverage comment is still applicable.
The existing test coverage comment suggesting comprehensive test cases for Array, Tuple, ByteArray, and Primitive types is still valid.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <UpgradeEventProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { | ||
| WorldEvent::EventUpgraded(e) => e, | ||
| _ => { | ||
| unreachable!() | ||
| } | ||
| }; |
There was a problem hiding this comment.
Replace panic and unreachable with proper error handling
Ohayo sensei! The current error handling could crash the application. Consider using proper error propagation:
- let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <UpgradeEventProcessor as EventProcessor<P>>::event_key(self)
- )
- }) {
- WorldEvent::EventUpgraded(e) => e,
- _ => {
- unreachable!()
- }
+ let event = match WorldEvent::try_from(event) {
+ Ok(WorldEvent::EventUpgraded(e)) => e,
+ Ok(_) => return Err(Error::msg(format!(
+ "Unexpected event type for {}",
+ <UpgradeEventProcessor as EventProcessor<P>>::event_key(self)
+ ))),
+ Err(e) => return Err(Error::msg(format!(
+ "Failed to parse {} event: {}",
+ <UpgradeEventProcessor as EventProcessor<P>>::event_key(self),
+ e
+ )))
};📝 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.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | |
| panic!( | |
| "Expected {} event to be well formed.", | |
| <UpgradeEventProcessor as EventProcessor<P>>::event_key(self) | |
| ) | |
| }) { | |
| WorldEvent::EventUpgraded(e) => e, | |
| _ => { | |
| unreachable!() | |
| } | |
| }; | |
| let event = match WorldEvent::try_from(event) { | |
| Ok(WorldEvent::EventUpgraded(e)) => e, | |
| Ok(_) => return Err(Error::msg(format!( | |
| "Unexpected event type for {}", | |
| <UpgradeEventProcessor as EventProcessor<P>>::event_key(self) | |
| ))), | |
| Err(e) => return Err(Error::msg(format!( | |
| "Failed to parse {} event: {}", | |
| <UpgradeEventProcessor as EventProcessor<P>>::event_key(self), | |
| e | |
| ))) | |
| }; |
crates/dojo/types/src/schema.rs
Outdated
| pub fn diff(&self, other: &Ty) -> Option<Ty> { | ||
| match (self, other) { | ||
| (Ty::Struct(s1), Ty::Struct(s2)) => { | ||
| // Find members that exist in s1 but not in s2, or are different | ||
| let diff_children: Vec<Member> = s1 | ||
| .children | ||
| .iter() | ||
| .filter(|m1| { | ||
| s2.children | ||
| .iter() | ||
| .find(|m2| m2.name == m1.name) | ||
| .map_or(true, |m2| *m1 != m2) | ||
| }) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| if diff_children.is_empty() { | ||
| None | ||
| } else { | ||
| Some(Ty::Struct(Struct { name: s1.name.clone(), children: diff_children })) | ||
| } | ||
| } | ||
| (Ty::Enum(e1), Ty::Enum(e2)) => { | ||
| // Find options that exist in e1 but not in e2, or are different | ||
| let diff_options: Vec<EnumOption> = e1 | ||
| .options | ||
| .iter() | ||
| .filter(|o1| { | ||
| e2.options.iter().find(|o2| o2.name == o1.name).map_or(true, |o2| *o1 != o2) | ||
| }) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| if diff_options.is_empty() { | ||
| None | ||
| } else { | ||
| Some(Ty::Enum(Enum { | ||
| name: e1.name.clone(), | ||
| option: e1.option, | ||
| options: diff_options, | ||
| })) | ||
| } | ||
| } | ||
| (Ty::Array(a1), Ty::Array(a2)) => { | ||
| if a1 == a2 { | ||
| None | ||
| } else { | ||
| Some(Ty::Array(a1.clone())) | ||
| } | ||
| } | ||
| (Ty::Tuple(t1), Ty::Tuple(t2)) => { | ||
| if t1 == t2 { | ||
| None | ||
| } else { | ||
| Some(Ty::Tuple(t1.clone())) | ||
| } | ||
| } | ||
| (Ty::ByteArray(b1), Ty::ByteArray(b2)) => { | ||
| if b1 == b2 { | ||
| None | ||
| } else { | ||
| Some(Ty::ByteArray(b1.clone())) | ||
| } | ||
| } | ||
| (Ty::Primitive(p1), Ty::Primitive(p2)) => { | ||
| if p1 == p2 { | ||
| None | ||
| } else { | ||
| Some(Ty::Primitive(*p1)) | ||
| } | ||
| } | ||
| // Different types entirely - we cannot diff them | ||
| _ => panic!( | ||
| "Type mismatch between self {:?} and other {:?}", | ||
| self.name(), | ||
| other.name() | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Consider returning Result instead of panicking.
The diff method currently panics when comparing different types. For a public API, it would be better to return a Result type to handle type mismatches gracefully.
Here's a suggested implementation:
- pub fn diff(&self, other: &Ty) -> Option<Ty> {
+ #[derive(Debug, thiserror::Error)]
+ pub enum DiffError {
+ #[error("Type mismatch between {0} and {1}")]
+ TypeMismatch(String, String),
+ }
+
+ pub fn diff(&self, other: &Ty) -> Result<Option<Ty>, DiffError> {
match (self, other) {
// ... existing match arms ...
_ => {
- panic!(
- "Type mismatch between self {:?} and other {:?}",
- self.name(),
- other.name()
- ),
+ Err(DiffError::TypeMismatch(self.name(), other.name()))
}
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/torii/core/src/processors/register_event.rs(1 hunks)crates/torii/core/src/processors/register_model.rs(1 hunks)crates/torii/core/src/processors/upgrade_event.rs(1 hunks)crates/torii/core/src/processors/upgrade_model.rs(1 hunks)crates/torii/core/src/sql/mod.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/torii/core/src/processors/register_event.rs
- crates/torii/core/src/processors/register_model.rs
- crates/torii/core/src/processors/upgrade_event.rs
- crates/torii/core/src/processors/upgrade_model.rs
🔇 Additional comments (1)
crates/torii/core/src/sql/mod.rs (1)
257-264: Ohayo sensei! The signature changes for model upgrades look good!
The addition of upgrade_diff: Option<&Ty> parameter is a clean way to handle model upgrades while maintaining backward compatibility.
| let mut alter_table_queries = Vec::new(); | ||
|
|
||
| if array_idx > 0 { | ||
| // index columns | ||
| for i in 0..array_idx { | ||
| create_table_query.push_str(&format!("idx_{i} INTEGER NOT NULL, ", i = i)); | ||
| let column = format!("idx_{i} INTEGER NOT NULL"); | ||
| create_table_query.push_str(&format!("{column}, ")); | ||
|
|
||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] ADD COLUMN idx_{i} INTEGER NOT NULL DEFAULT 0" | ||
| )); | ||
| } | ||
|
|
||
| // full array id column | ||
| create_table_query.push_str("full_array_id TEXT NOT NULL UNIQUE, "); | ||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL UNIQUE DEFAULT ''" | ||
| )); | ||
| } | ||
|
|
||
| let mut build_member = |name: &str, ty: &Ty, options: &mut Option<Argument>| { | ||
| if let Ok(cairo_type) = Primitive::from_str(&ty.name()) { | ||
| create_table_query | ||
| .push_str(&format!("external_{name} {}, ", cairo_type.to_sql_type())); | ||
| let sql_type = cairo_type.to_sql_type(); | ||
| let column = format!("external_{name} {sql_type}"); | ||
|
|
||
| create_table_query.push_str(&format!("{column}, ")); | ||
|
|
||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] ADD COLUMN external_{name} {sql_type}" | ||
| )); | ||
|
|
There was a problem hiding this comment.
Critical: Unsafe UNIQUE constraint addition in ALTER TABLE statements.
The ALTER TABLE statements for array indices add UNIQUE constraints with DEFAULT values, which could cause constraint violations in concurrent scenarios. For example:
"ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL UNIQUE DEFAULT ''"This is unsafe because:
- Multiple rows will temporarily have the same DEFAULT value ('')
- The UNIQUE constraint is enforced immediately
Consider this safer approach:
-"ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL UNIQUE DEFAULT ''"
+"ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL DEFAULT '';"
+"UPDATE [{table_id}] SET full_array_id = id || '_' || ROWID;"
+"CREATE UNIQUE INDEX [{table_id}_full_array_id_unique] ON [{table_id}](full_array_id);"| if let Some(upgrade_diff) = upgrade_diff { | ||
| if !upgrade_diff | ||
| .as_struct() | ||
| .unwrap() | ||
| .children | ||
| .iter() | ||
| .any(|m| m.name == member.name) | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Unsafe unwrap() on upgrade_diff type check.
The code assumes upgrade_diff is always a Struct type:
upgrade_diff.as_struct().unwrap()This could panic if upgrade_diff contains a different type.
Consider this safer approach:
-if !upgrade_diff
- .as_struct()
- .unwrap()
- .children
+if !upgrade_diff
+ .as_struct()
+ .ok_or_else(|| anyhow!("upgrade_diff must be a struct"))?
+ .children| if upgrade_diff.is_some() { | ||
| for alter_query in alter_table_queries { | ||
| self.executor.send(QueryMessage::other(alter_query, vec![]))?; | ||
| } | ||
| } else { | ||
| self.executor.send(QueryMessage::other(create_table_query, vec![]))?; | ||
| } | ||
|
|
||
| for s in indices.iter() { | ||
| self.executor.send(QueryMessage::other(s.to_string(), vec![]))?; | ||
| for index_query in indices { | ||
| self.executor.send(QueryMessage::other(index_query, vec![]))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap schema changes in a transaction for atomicity.
The table alterations and index creation should be atomic - either all succeed or all fail.
Consider wrapping the operations in a transaction:
+self.executor.send(QueryMessage::other("BEGIN TRANSACTION;", vec![]))?;
+
if upgrade_diff.is_some() {
for alter_query in alter_table_queries {
self.executor.send(QueryMessage::other(alter_query, vec![]))?;
}
} else {
self.executor.send(QueryMessage::other(create_table_query, vec![]))?;
}
for index_query in indices {
self.executor.send(QueryMessage::other(index_query, vec![]))?;
}
+
+self.executor.send(QueryMessage::other("COMMIT;", vec![]))?;📝 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 upgrade_diff.is_some() { | |
| for alter_query in alter_table_queries { | |
| self.executor.send(QueryMessage::other(alter_query, vec![]))?; | |
| } | |
| } else { | |
| self.executor.send(QueryMessage::other(create_table_query, vec![]))?; | |
| } | |
| for s in indices.iter() { | |
| self.executor.send(QueryMessage::other(s.to_string(), vec![]))?; | |
| for index_query in indices { | |
| self.executor.send(QueryMessage::other(index_query, vec![]))?; | |
| self.executor.send(QueryMessage::other("BEGIN TRANSACTION;", vec![]))?; | |
| if upgrade_diff.is_some() { | |
| for alter_query in alter_table_queries { | |
| self.executor.send(QueryMessage::other(alter_query, vec![]))?; | |
| } | |
| } else { | |
| self.executor.send(QueryMessage::other(create_table_query, vec![]))?; | |
| } | |
| for index_query in indices { | |
| self.executor.send(QueryMessage::other(index_query, vec![]))?; | |
| } | |
| self.executor.send(QueryMessage::other("COMMIT;", vec![]))?; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/sql/mod.rs (1)
1091-1094: Improve NULL constraint handling in ALTER TABLE.The NULL constraint handling for enum columns could be more explicit.
Consider this improvement:
- "external_{name} TEXT CHECK(external_{name} IN ({all_options})) {}" + "external_{name} TEXT {} CHECK(external_{name} IN ({all_options}))"This puts the NULL/NOT NULL constraint before the CHECK constraint, which is more standard and clearer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/core/src/sql/mod.rs(12 hunks)
🔇 Additional comments (3)
crates/torii/core/src/sql/mod.rs (3)
257-264: Ohayo sensei! The changes to register_model look good!
The function signature improvements are well thought out:
- Using references instead of taking ownership of model parameter improves memory efficiency
- The new upgrade_diff parameter enables incremental model updates
1063-1065:
Critical: ALTER TABLE with UNIQUE constraint needs careful handling.
Adding a UNIQUE column with a default empty string value could cause constraint violations if multiple rows exist.
Consider this safer approach:
-"ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL UNIQUE DEFAULT ''"
+"ALTER TABLE [{table_id}] ADD COLUMN full_array_id TEXT NOT NULL DEFAULT '';"
+"UPDATE [{table_id}] SET full_array_id = id || '_' || ROWID;"
+"CREATE UNIQUE INDEX [{table_id}_full_array_id_unique] ON [{table_id}](full_array_id);"Likely invalid or redundant comment.
1298-1307:
Wrap schema changes in a transaction for atomicity.
The table alterations and index creation should be atomic - either all succeed or all fail.
Apply this diff to ensure atomicity:
+self.executor.send(QueryMessage::other("BEGIN TRANSACTION;", vec![]))?;
if upgrade_diff.is_some() {
for alter_query in alter_table_queries {
self.executor.send(QueryMessage::other(alter_query, vec![]))?;
}
} else {
self.executor.send(QueryMessage::other(create_table_query, vec![]))?;
}
for index_query in indices {
self.executor.send(QueryMessage::other(index_query, vec![]))?;
}
+self.executor.send(QueryMessage::other("COMMIT;", vec![]))?;Likely invalid or redundant comment.
| // nested members are not upgrades | ||
| None, |
There was a problem hiding this comment.
Handle upgrade_diff unwrap safely in parent function.
Setting upgrade_diff to None for nested members is correct, but the parent function has an unsafe unwrap() on line 1133 that could panic if upgrade_diff is not a Struct type.
Apply this diff to handle the unwrap safely:
-upgrade_diff.as_struct().unwrap()
+upgrade_diff.as_struct().ok_or_else(|| anyhow!("upgrade_diff must be a struct"))?Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/dojo/types/src/primitive.rs (1)
Line range hint
422-434: Ohayo, sensei! Test coverage could be improved.The test case only covers successful U256 serialization. Consider adding test cases for:
- Handling of None values
- Error cases (e.g., invalid hex strings)
- Edge cases (e.g., max/min values)
Add these test cases:
#[test] fn test_u256_edge_cases() { // Test None value let primitive = Primitive::U256(None); assert_eq!(primitive.to_sql_value(), "0x0000000000000000000000000000000000000000000000000000000000000000"); // Test max value let primitive = Primitive::U256(Some(U256::MAX)); assert_eq!(primitive.to_sql_value(), "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // Test min value let primitive = Primitive::U256(Some(U256::ZERO)); assert_eq!(primitive.to_sql_value(), "0x0000000000000000000000000000000000000000000000000000000000000000"); }crates/torii/core/src/model.rs (2)
253-253: Consider removing the unused parameter, sensei!The
_parent_is_optionalparameter is now unused across all match arms. Since we're no longer tracking optionality, we can simplify the function signature by removing this parameter entirely.fn parse_ty( path: &str, name: &str, ty: &Ty, selections: &mut Vec<String>, tables: &mut Vec<TableInfo>, arrays_queries: &mut HashMap<String, (Vec<String>, Vec<TableInfo>)>, - _parent_is_optional: bool, depth: usize, )Also applies to: 264-264, 276-276, 287-287, 299-299, 312-312, 359-359
400-400: LGTM! Consider adding documentation about the join strategy.The consistent use of LEFT JOIN simplifies the query generation logic. However, it would be helpful to document why this approach was chosen over conditional join types.
Add a comment above the join generation:
+ // Using LEFT JOIN consistently to ensure all related records are included, + // even when the relationship is empty format!(" LEFT JOIN [{}] ON {join_condition}", table.table_name)Also applies to: 427-427, 1071-1076
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/dojo/types/src/primitive.rs(2 hunks)crates/torii/core/src/model.rs(11 hunks)
🔇 Additional comments (3)
crates/dojo/types/src/primitive.rs (2)
193-215: Ohayo, sensei! Consider handling None values explicitly.
The use of unwrap_or_default() in to_sql_value might silently convert None values to defaults, potentially masking errors or data integrity issues.
214-215: Ohayo, sensei! Inconsistent hex formatting detected.
The U256 formatting uses 0x{:064x} while other hex values use {:#064x}. This inconsistency could cause issues when parsing these values.
crates/torii/core/src/model.rs (1)
241-241: Ohayo! Verify the impact of removing optionality tracking.
The removal of is_optional field suggests a shift to using LEFT JOINs consistently. While this simplifies the logic, it might affect query performance as LEFT JOINs are generally slower than INNER JOINs.
✅ Verification successful
I'll analyze the join patterns and their relationship with the removed is_optional field to better understand the impact of this change.
Ohayo! The removal of is_optional is actually a safe change, sensei!
After analyzing the codebase, I can confirm that this change is safe because:
- The codebase was already consistently using LEFT JOINs, as evidenced by the grep results showing no INNER JOIN usage
- The
is_optionalfield was commented out but wasn't actually affecting the join type selection - All join conditions in the codebase (
crates/torii/core/src/model.rsandcrates/torii/grpc/src/server/mod.rs) are consistently implemented with LEFT JOINs
The removal simplifies the code without changing the actual query behavior or performance characteristics since the join strategy wasn't varying based on this field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance impact by analyzing table sizes and join patterns
# Search for existing SQL query patterns
rg -A 5 'LEFT JOIN|INNER JOIN'
# Look for any performance-related comments or issues
rg -i 'performance|slow query|optimization'
Length of output: 3562
Script:
#!/bin/bash
# Let's analyze the join-related changes more thoroughly
# Check for any join-related logic changes
ast-grep --pattern 'fn $name($$$) {
$$$
LEFT JOIN
$$$
}'
# Look for any removed INNER JOIN logic
rg -B 5 'is_optional'
# Check for any query-related tests
rg -A 10 '#\[test\].*query'
Length of output: 5329
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/core/src/model.rs (1)
1092-1097: Nitpick: Consider improving SQL query readabilitySensei, while the query generation works correctly, the string concatenation could be more readable.
Consider using a multi-line string with proper indentation:
- AS \"Test-PlayerConfig.favorite_item\" FROM entities LEFT JOIN [Test-Position] ON \ - entities.id = [Test-Position].entity_id LEFT JOIN [Test-PlayerConfig] ON \ - entities.id = [Test-PlayerConfig].entity_id LEFT JOIN [Test-Position$vec] ON \ - entities.id = [Test-Position$vec].entity_id LEFT JOIN \ - [Test-PlayerConfig$favorite_item] ON entities.id = \ - [Test-PlayerConfig$favorite_item].entity_id ORDER BY entities.event_id DESC"; + AS \"Test-PlayerConfig.favorite_item\" + FROM entities + LEFT JOIN [Test-Position] ON entities.id = [Test-Position].entity_id + LEFT JOIN [Test-PlayerConfig] ON entities.id = [Test-PlayerConfig].entity_id + LEFT JOIN [Test-Position$vec] ON entities.id = [Test-Position$vec].entity_id + LEFT JOIN [Test-PlayerConfig$favorite_item] ON entities.id = [Test-PlayerConfig$favorite_item].entity_id + ORDER BY entities.event_id DESC";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/core/src/model.rs(14 hunks)crates/torii/core/src/sql/mod.rs(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/sql/mod.rs
🔇 Additional comments (3)
crates/torii/core/src/model.rs (3)
241-241: LGTM: Simplified optionality handling in table joins
The removal of is_optional tracking and the corresponding parameter changes streamline the code without affecting functionality, as all joins are now consistently handled as LEFT JOINs.
Also applies to: 253-253, 264-264, 287-287, 312-312, 359-359
513-518: Improved robustness with empty string handling
The addition of empty string checks before parsing hex values and Felt types improves error handling. This prevents parsing errors when dealing with null or empty values in the database.
Also applies to: 535-540, 545-550, 555-558, 570-574, 578-582, 586-590
400-400: Verify the impact of uniform LEFT JOIN usage
Ohayo! The change to use LEFT JOIN uniformly might affect query performance. While it simplifies the code, we should ensure it doesn't impact query execution plans significantly.
Consider adding an index on the entity_id column if not already present, as it's used in all join conditions.
Also applies to: 427-427
✅ Verification successful
Let's analyze the actual join usage and table structure to better understand the impact of uniform LEFT JOIN usage.
LEFT JOIN usage is appropriate and well-indexed
Ohayo! After analyzing the database schema and join patterns, I can confirm that the uniform LEFT JOIN usage is appropriate and well-supported:
- The tables involved have proper indexes on join columns (e.g.,
entity_id,model_id) - The join conditions are consistently using indexed columns
- The schema shows proper foreign key relationships between the tables
The LEFT JOIN is the correct choice here as it preserves all records from the entities table while fetching related data, which is essential for the query's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance impact of LEFT JOIN usage
# Look for existing EXPLAIN QUERY PLAN statements or similar performance hints
rg -A 3 "EXPLAIN.*JOIN"
rg -A 3 "ANALYZE.*JOIN"
Length of output: 51
Script:
#!/bin/bash
# Let's analyze the join patterns and table structure more comprehensively
# Check for table definitions and join patterns
rg -B 2 -A 2 "CREATE TABLE|CREATE INDEX"
# Look for existing join patterns
rg -B 2 -A 2 "JOIN.*ON"
# Check for any query-related tests or benchmarks
fd -e rs -x rg -l "test|bench.*join"
# Look for any SQL-related configuration or schema files
fd -e sql -e schema
Length of output: 31401
This reverts commit 551c6bd.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/types/src/primitive.rs(2 hunks)
🔇 Additional comments (2)
crates/dojo/types/src/primitive.rs (2)
193-215: Ohayo, sensei! Potential risk with default values in to_sql_value
The use of unwrap_or_default() in to_sql_value might silently convert None values to defaults, potentially leading to unintended data being stored or processed. It's important to handle None explicitly to ensure data integrity.
Line range hint 215-422: Ohayo, sensei! Good job on robust error handling in deserialize
The function deserialize handles various error conditions effectively, ensuring that data integrity is maintained during the conversion process. The checks for empty inputs and field element counts are particularly crucial for preventing runtime errors.
| pub fn to_sql_value(&self) -> String { | ||
| match self { | ||
| // Integers | ||
| Primitive::I8(_) => Ok(format!("{}", try_from_felt::<i8>(value[0])?)), | ||
| Primitive::I16(_) => Ok(format!("{}", try_from_felt::<i16>(value[0])?)), | ||
| Primitive::I32(_) => Ok(format!("{}", try_from_felt::<i32>(value[0])?)), | ||
| Primitive::I64(_) => Ok(format!("{}", try_from_felt::<i64>(value[0])?)), | ||
| Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()), | ||
| Primitive::I16(i16) => format!("{}", i16.unwrap_or_default()), | ||
| Primitive::I32(i32) => format!("{}", i32.unwrap_or_default()), | ||
| Primitive::I64(i64) => format!("{}", i64.unwrap_or_default()), | ||
|
|
||
| Primitive::U8(_) | ||
| | Primitive::U16(_) | ||
| | Primitive::U32(_) | ||
| | Primitive::USize(_) | ||
| | Primitive::Bool(_) => Ok(format!("{}", value[0])), | ||
| Primitive::U8(u8) => format!("{}", u8.unwrap_or_default()), | ||
| Primitive::U16(u16) => format!("{}", u16.unwrap_or_default()), | ||
| Primitive::U32(u32) => format!("{}", u32.unwrap_or_default()), | ||
| Primitive::USize(u32) => format!("{}", u32.unwrap_or_default()), | ||
| Primitive::Bool(bool) => format!("{}", bool.unwrap_or_default() as i32), | ||
|
|
||
| // Hex string | ||
| Primitive::I128(_) => Ok(format!("{:#064x}", try_from_felt::<i128>(value[0])?)), | ||
| Primitive::ContractAddress(_) | ||
| | Primitive::ClassHash(_) | ||
| | Primitive::Felt252(_) | ||
| | Primitive::U128(_) | ||
| | Primitive::U64(_) => Ok(format!("{:#064x}", value[0])), | ||
|
|
||
| Primitive::U256(_) => { | ||
| if value.len() < 2 { | ||
| Err(PrimitiveError::NotEnoughFieldElements) | ||
| } else { | ||
| let mut buffer = [0u8; 32]; | ||
| let value0_bytes = value[0].to_bytes_be(); | ||
| let value1_bytes = value[1].to_bytes_be(); | ||
| buffer[16..].copy_from_slice(&value0_bytes[16..]); | ||
| buffer[..16].copy_from_slice(&value1_bytes[16..]); | ||
| Ok(format!("0x{}", hex::encode(buffer))) | ||
| } | ||
| } | ||
| Primitive::I128(i128) => format!("0x{:064x}", i128.unwrap_or_default()), | ||
| Primitive::ContractAddress(felt) => format!("0x{:064x}", felt.unwrap_or_default()), | ||
| Primitive::ClassHash(felt) => format!("0x{:064x}", felt.unwrap_or_default()), | ||
| Primitive::Felt252(felt) => format!("0x{:064x}", felt.unwrap_or_default()), | ||
| Primitive::U128(u128) => format!("0x{:064x}", u128.unwrap_or_default()), | ||
| Primitive::U64(u64) => format!("0x{:064x}", u64.unwrap_or_default()), | ||
|
|
||
| Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()), |
There was a problem hiding this comment.
Ohayo, sensei! Consider handling None values explicitly in to_sql_value
The current implementation uses unwrap_or_default() which might lead to unintended default values being used, especially in a database context where data accuracy is crucial. Consider explicitly handling None values to prevent data corruption.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/types/src/primitive.rs(2 hunks)crates/torii/core/src/processors/register_event.rs(1 hunks)crates/torii/core/src/processors/register_model.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/core/src/processors/register_event.rs
- crates/torii/core/src/processors/register_model.rs
| @@ -436,7 +419,7 @@ mod tests { | |||
| let primitive = Primitive::U256(Some(U256::from_be_hex( | |||
| "aaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbccccccccccccccccdddddddddddddddd", | |||
| ))); | |||
| let sql_value = primitive.to_sql_value().unwrap(); | |||
| let sql_value = primitive.to_sql_value(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Add error case tests
The test only covers the happy path with Some values. We should also test error cases with None values.
Add these test cases:
#[test]
fn test_u256_none() {
let primitive = Primitive::U256(None);
assert!(matches!(
primitive.serialize(),
Err(PrimitiveError::MissingFieldElement)
));
}
crates/dojo/types/src/primitive.rs
Outdated
| Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()), | ||
| Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()), | ||
| Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()), | ||
|
|
||
| Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Standardize hex formatting across all types
The hex formatting is inconsistent:
- Most types use
{:#064x}format - U256 uses
0x{:064x}format
This inconsistency could cause issues when parsing these values.
Apply this change:
- Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()),
+ Primitive::U256(u256) => format!("{:#064x}", u256.unwrap_or_default()),📝 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.
| Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()), | |
| Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()), | |
| Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()), | |
| Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()), | |
| Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()), | |
| Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()), | |
| Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()), | |
| Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()), | |
| Primitive::U256(u256) => format!("{:#064x}", u256.unwrap_or_default()), |
crates/dojo/types/src/primitive.rs
Outdated
| pub fn to_sql_value(&self) -> String { | ||
| match self { | ||
| // Integers | ||
| Primitive::I8(_) => Ok(format!("{}", try_from_felt::<i8>(value[0])?)), | ||
| Primitive::I16(_) => Ok(format!("{}", try_from_felt::<i16>(value[0])?)), | ||
| Primitive::I32(_) => Ok(format!("{}", try_from_felt::<i32>(value[0])?)), | ||
| Primitive::I64(_) => Ok(format!("{}", try_from_felt::<i64>(value[0])?)), | ||
| Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()), | ||
| Primitive::I16(i16) => format!("{}", i16.unwrap_or_default()), | ||
| Primitive::I32(i32) => format!("{}", i32.unwrap_or_default()), | ||
| Primitive::I64(i64) => format!("{}", i64.unwrap_or_default()), | ||
|
|
||
| Primitive::U8(_) | ||
| | Primitive::U16(_) | ||
| | Primitive::U32(_) | ||
| | Primitive::USize(_) | ||
| | Primitive::Bool(_) => Ok(format!("{}", value[0])), | ||
| Primitive::U8(u8) => format!("{}", u8.unwrap_or_default()), | ||
| Primitive::U16(u16) => format!("{}", u16.unwrap_or_default()), | ||
| Primitive::U32(u32) => format!("{}", u32.unwrap_or_default()), | ||
| Primitive::USize(u32) => format!("{}", u32.unwrap_or_default()), | ||
| Primitive::Bool(bool) => format!("{}", bool.unwrap_or_default() as i32), | ||
|
|
||
| // Hex string | ||
| Primitive::I128(_) => Ok(format!("{:#064x}", try_from_felt::<i128>(value[0])?)), | ||
| Primitive::ContractAddress(_) | ||
| | Primitive::ClassHash(_) | ||
| | Primitive::Felt252(_) | ||
| | Primitive::U128(_) | ||
| | Primitive::U64(_) => Ok(format!("{:#064x}", value[0])), | ||
|
|
||
| Primitive::U256(_) => { | ||
| if value.len() < 2 { | ||
| Err(PrimitiveError::NotEnoughFieldElements) | ||
| } else { | ||
| let mut buffer = [0u8; 32]; | ||
| let value0_bytes = value[0].to_bytes_be(); | ||
| let value1_bytes = value[1].to_bytes_be(); | ||
| buffer[16..].copy_from_slice(&value0_bytes[16..]); | ||
| buffer[..16].copy_from_slice(&value1_bytes[16..]); | ||
| Ok(format!("0x{}", hex::encode(buffer))) | ||
| } | ||
| } | ||
| Primitive::I128(i128) => format!("{:#064x}", i128.unwrap_or_default()), | ||
| Primitive::ContractAddress(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::ClassHash(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::Felt252(felt) => format!("{:#064x}", felt.unwrap_or_default()), | ||
| Primitive::U128(u128) => format!("{:#064x}", u128.unwrap_or_default()), | ||
| Primitive::U64(u64) => format!("{:#064x}", u64.unwrap_or_default()), | ||
|
|
||
| Primitive::U256(u256) => format!("0x{:064x}", u256.unwrap_or_default()), |
There was a problem hiding this comment.
Ohayo, sensei! Critical: Avoid silent defaults in database operations
The use of unwrap_or_default() in to_sql_value could silently store default values in the database when None is encountered, potentially leading to data integrity issues. Consider handling None values explicitly.
Consider this approach:
- pub fn to_sql_value(&self) -> String {
+ pub fn to_sql_value(&self) -> Result<String, PrimitiveError> {
match self {
// Integers
- Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()),
+ Primitive::I8(i8) => i8.map(|v| format!("{}", v))
+ .ok_or(PrimitiveError::MissingFieldElement),
// Apply similar changes to other variants...
}
}Committable suggestion skipped: line range outside the PR's diff.
This reverts commit cea8102.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/dojo/types/src/primitive.rs (1)
193-215: Ohayo, sensei! Consider adding logging for default value substitutions.When
unwrap_or_default()is used, we should log these occurrences to help track potential data quality issues.pub fn to_sql_value(&self) -> String { + use log::warn; match self { // Integers - Primitive::I8(i8) => format!("{}", i8.unwrap_or_default()), + Primitive::I8(i8) => { + if i8.is_none() { + warn!("Using default value for I8 in SQL conversion"); + } + format!("{}", i8.unwrap_or_default()) + }, // Apply similar changes to other variants...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/types/src/primitive.rs(2 hunks)
🔇 Additional comments (4)
crates/dojo/types/src/primitive.rs (4)
193-215: Ohayo, sensei! Consider handling None values explicitly in database operations.
The current implementation of to_sql_value uses unwrap_or_default() which could silently store default values in the database. This might lead to data integrity issues where we can't distinguish between actual default values and missing data.
208-215: Ohayo, sensei! Standardize hex formatting across numeric types.
The hex formatting is inconsistent between different numeric types. All use 0x{:064x} format, which differs from the standard {:#064x} format commonly used in Rust.
422-422: Ohayo, sensei! Add test cases for error scenarios.
The test case only verifies the happy path with Some values. We should also test error cases with None values to ensure proper error handling.
193-215: Ohayo, sensei! Verify database schema constraints.
The use of default values might conflict with database NOT NULL constraints if they exist.
✅ Verification successful
The database schema shows that all columns in the tables have appropriate NOT NULL constraints with DEFAULT values where needed. For example:
- Timestamp columns have
NOT NULL DEFAULT CURRENT_TIMESTAMP - Boolean columns have
NOT NULL - Text/numeric columns have
NOT NULLwith explicit default values
Let's verify if the to_sql_value() implementation properly handles these constraints by checking if there are any database errors or issues reported. I'll search for any related test files or error logs:
Ohayo, sensei! Your default values are dojo-perfect! ✨
The to_sql_value() implementation is safe with respect to database NOT NULL constraints because:
- Every primitive variant uses
unwrap_or_default()which guarantees a non-null value - All database columns have appropriate NOT NULL constraints with DEFAULT values
- The method is used in controlled contexts (grpc server, core sql module) where null handling is properly managed
The code shows defensive programming by always providing a default value, which aligns perfectly with the database schema requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for NOT NULL constraints in the database schema that might conflict with default values
# Search for SQL schema definitions
rg -A 5 "CREATE TABLE|NOT NULL" --type sql
# Search for database migration files
fd -e sql -e migration
Length of output: 27039
Script:
#!/bin/bash
# Search for SQL-related tests and error handling
rg -g "*.rs" "to_sql_value|sql.*error" -A 3 -B 3
# Search for any database error handling related to NULL values
rg -g "*.rs" "NOT_NULL|null.*constraint|constraint.*null" -A 3 -B 3
Length of output: 4094
Summary by CodeRabbit
Release Notes
New Features
model_reader_with_blockmethod for block-specific model queries.UpgradeEventProcessorandUpgradeModelProcessorfor enhanced upgrade event handling.diffmethod inTyenum to compute differences between type instances.Improvements
ModelCachefor better performance and efficiency.Tests
ModelCachefor better resource management.