feat(torii-core): store update member #2182
Conversation
Ohayo, Sensei!WalkthroughThis update enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StoreUpdateMemberProcessor
participant Database
participant Sql
User->>StoreUpdateMemberProcessor: Event with Update Member Data
StoreUpdateMemberProcessor->>Sql: process(event, ...)
Sql->>Database: Check if entity exists
Database-->>Sql: Entity existence result
Sql->>Database: Update model member
Database-->>Sql: Update result
Sql-->>StoreUpdateMemberProcessor: Processing result
StoreUpdateMemberProcessor-->>User: Success/Failure response
This diagram illustrates the workflow for updating a model member when an event is received, detailing the interactions among the Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2182 +/- ##
==========================================
+ Coverage 69.82% 70.08% +0.26%
==========================================
Files 340 344 +4
Lines 44764 45120 +356
==========================================
+ Hits 31256 31624 +368
+ Misses 13508 13496 -12 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- bin/torii/src/main.rs (2 hunks)
- crates/torii/core/src/processors/mod.rs (1 hunks)
- crates/torii/core/src/processors/store_update_member.rs (1 hunks)
- crates/torii/core/src/sql.rs (2 hunks)
Additional context used
GitHub Check: codecov/patch
crates/torii/core/src/processors/store_update_member.rs
[warning] 28-30: crates/torii/core/src/processors/store_update_member.rs#L28-L30
Added lines #L28 - L30 were not covered by tests
[warning] 32-34: crates/torii/core/src/processors/store_update_member.rs#L32-L34
Added lines #L32 - L34 were not covered by tests
[warning] 36-38: crates/torii/core/src/processors/store_update_member.rs#L36-L38
Added lines #L36 - L38 were not covered by tests
[warning] 40-43: crates/torii/core/src/processors/store_update_member.rs#L40-L43
Added lines #L40 - L43 were not covered by tests
[warning] 54-98: crates/torii/core/src/processors/store_update_member.rs#L54-L98
Added lines #L54 - L98 were not covered by testsbin/torii/src/main.rs
[warning] 176-176: bin/torii/src/main.rs#L176
Added line #L176 was not covered by testscrates/torii/core/src/sql.rs
[warning] 274-292: crates/torii/core/src/sql.rs#L274-L292
Added lines #L274 - L292 were not covered by tests
[warning] 294-295: crates/torii/core/src/sql.rs#L294-L295
Added lines #L294 - L295 were not covered by tests
Additional comments not posted (1)
crates/torii/core/src/processors/mod.rs (1)
15-15: Modulestore_update_memberadded successfully.This change is consistent with the PR's objective to enhance store update functionalities.
However, ensure that the new module integrates seamlessly with the existing system.
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::store_update_record"; | ||
|
|
||
| const MEMBER_INDEX: usize = 2; | ||
|
|
||
| #[derive(Default, Debug)] | ||
| pub struct StoreUpdateMemberProcessor; | ||
|
|
||
| #[async_trait] | ||
| impl<P> EventProcessor<P> for StoreUpdateMemberProcessor | ||
| where | ||
| P: Provider + Send + Sync + std::fmt::Debug, | ||
| { | ||
| fn event_key(&self) -> String { | ||
| "StoreUpdateMember".to_string() | ||
| } | ||
|
|
||
| fn validate(&self, event: &Event) -> bool { | ||
| if event.keys.len() > 1 { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| event_key = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self), | ||
| invalid_keys = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_keys_as_string(self, event), | ||
| "Invalid event keys." | ||
| ); | ||
| return false; | ||
| } | ||
| true | ||
| } | ||
|
|
||
| async fn process( | ||
| &self, | ||
| _world: &WorldContractReader<P>, | ||
| db: &mut Sql, | ||
| _block_number: u64, | ||
| block_timestamp: u64, | ||
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | ||
| _event_id: &str, | ||
| event: &Event, | ||
| ) -> Result<(), Error> { | ||
| let selector = event.data[MODEL_INDEX]; | ||
| let entity_id = event.data[ENTITY_ID_INDEX]; | ||
| let member_selector = event.data[MEMBER_INDEX]; | ||
|
|
||
| let model = db.model(selector).await?; | ||
|
|
||
| info!( | ||
| target: LOG_TARGET, | ||
| name = %model.name(), | ||
| entity_id = format!("{:#x}", entity_id), | ||
| "Store update record.", | ||
| ); | ||
|
|
||
| let values_start = MEMBER_INDEX + 1; | ||
| let values_end: usize = | ||
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | ||
|
|
||
| // Skip the length to only get the values as they will be deserialized. | ||
| let values = event.data[values_start + 1..=values_end].to_vec(); | ||
|
|
||
| let tag = naming::get_tag(model.namespace(), model.name()); | ||
|
|
||
| // Keys are read from the db, since we don't have access to them when only | ||
| // the entity id is passed. | ||
| let keys = db.get_entity_keys(entity_id, &tag).await?; | ||
| let mut keys_and_unpacked = [keys, values].concat(); | ||
|
|
||
| let schema = model.schema().await?; | ||
| let mut ty = schema | ||
| .as_struct() | ||
| .expect("model schema must be a struct") | ||
| .children | ||
| .iter() | ||
| .find(|c| { | ||
| get_selector_from_name(&c.name).expect("invalid selector for member name") | ||
| == member_selector | ||
| }) | ||
| .context("member not found")? | ||
| .ty | ||
| .clone(); | ||
| ty.deserialize(&mut keys_and_unpacked)?; | ||
|
|
||
| db.set_model_member(entity_id, &schema.name(), &ty, block_timestamp).await | ||
| } |
There was a problem hiding this comment.
Comprehensive implementation of StoreUpdateMemberProcessor.
The processor is well-implemented with methods for event validation and processing. However, the codecov warnings indicate missing test coverage which is crucial for ensuring the functionality works as expected.
+ // TODO: Add unit tests to cover the new functionality.Would you like me to help in writing some initial unit tests or open a GitHub issue to track this task?
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::store_update_record"; | |
| const MEMBER_INDEX: usize = 2; | |
| #[derive(Default, Debug)] | |
| pub struct StoreUpdateMemberProcessor; | |
| #[async_trait] | |
| impl<P> EventProcessor<P> for StoreUpdateMemberProcessor | |
| where | |
| P: Provider + Send + Sync + std::fmt::Debug, | |
| { | |
| fn event_key(&self) -> String { | |
| "StoreUpdateMember".to_string() | |
| } | |
| fn validate(&self, event: &Event) -> bool { | |
| if event.keys.len() > 1 { | |
| info!( | |
| target: LOG_TARGET, | |
| event_key = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self), | |
| invalid_keys = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_keys_as_string(self, event), | |
| "Invalid event keys." | |
| ); | |
| return false; | |
| } | |
| true | |
| } | |
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| block_timestamp: u64, | |
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | |
| _event_id: &str, | |
| event: &Event, | |
| ) -> Result<(), Error> { | |
| let selector = event.data[MODEL_INDEX]; | |
| let entity_id = event.data[ENTITY_ID_INDEX]; | |
| let member_selector = event.data[MEMBER_INDEX]; | |
| let model = db.model(selector).await?; | |
| info!( | |
| target: LOG_TARGET, | |
| name = %model.name(), | |
| entity_id = format!("{:#x}", entity_id), | |
| "Store update record.", | |
| ); | |
| let values_start = MEMBER_INDEX + 1; | |
| let values_end: usize = | |
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | |
| // Skip the length to only get the values as they will be deserialized. | |
| let values = event.data[values_start + 1..=values_end].to_vec(); | |
| let tag = naming::get_tag(model.namespace(), model.name()); | |
| // Keys are read from the db, since we don't have access to them when only | |
| // the entity id is passed. | |
| let keys = db.get_entity_keys(entity_id, &tag).await?; | |
| let mut keys_and_unpacked = [keys, values].concat(); | |
| let schema = model.schema().await?; | |
| let mut ty = schema | |
| .as_struct() | |
| .expect("model schema must be a struct") | |
| .children | |
| .iter() | |
| .find(|c| { | |
| get_selector_from_name(&c.name).expect("invalid selector for member name") | |
| == member_selector | |
| }) | |
| .context("member not found")? | |
| .ty | |
| .clone(); | |
| ty.deserialize(&mut keys_and_unpacked)?; | |
| db.set_model_member(entity_id, &schema.name(), &ty, block_timestamp).await | |
| } | |
| pub(crate) const LOG_TARGET: &str = "torii_core::processors::store_update_record"; | |
| const MEMBER_INDEX: usize = 2; | |
| #[derive(Default, Debug)] | |
| pub struct StoreUpdateMemberProcessor; | |
| #[async_trait] | |
| impl<P> EventProcessor<P> for StoreUpdateMemberProcessor | |
| where | |
| P: Provider + Send + Sync + std::fmt::Debug, | |
| { | |
| fn event_key(&self) -> String { | |
| "StoreUpdateMember".to_string() | |
| } | |
| fn validate(&self, event: &Event) -> bool { | |
| if event.keys.len() > 1 { | |
| info!( | |
| target: LOG_TARGET, | |
| event_key = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self), | |
| invalid_keys = %<StoreUpdateMemberProcessor as EventProcessor<P>>::event_keys_as_string(self, event), | |
| "Invalid event keys." | |
| ); | |
| return false; | |
| } | |
| true | |
| } | |
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| block_timestamp: u64, | |
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | |
| _event_id: &str, | |
| event: &Event, | |
| ) -> Result<(), Error> { | |
| let selector = event.data[MODEL_INDEX]; | |
| let entity_id = event.data[ENTITY_ID_INDEX]; | |
| let member_selector = event.data[MEMBER_INDEX]; | |
| let model = db.model(selector).await?; | |
| info!( | |
| target: LOG_TARGET, | |
| name = %model.name(), | |
| entity_id = format!("{:#x}", entity_id), | |
| "Store update record.", | |
| ); | |
| let values_start = MEMBER_INDEX + 1; | |
| let values_end: usize = | |
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | |
| // Skip the length to only get the values as they will be deserialized. | |
| let values = event.data[values_start + 1..=values_end].to_vec(); | |
| let tag = naming::get_tag(model.namespace(), model.name()); | |
| // Keys are read from the db, since we don't have access to them when only | |
| // the entity id is passed. | |
| let keys = db.get_entity_keys(entity_id, &tag).await?; | |
| let mut keys_and_unpacked = [keys, values].concat(); | |
| let schema = model.schema().await?; | |
| let mut ty = schema | |
| .as_struct() | |
| .expect("model schema must be a struct") | |
| .children | |
| .iter() | |
| .find(|c| { | |
| get_selector_from_name(&c.name).expect("invalid selector for member name") | |
| == member_selector | |
| }) | |
| .context("member not found")? | |
| .ty | |
| .clone(); | |
| ty.deserialize(&mut keys_and_unpacked)?; | |
| db.set_model_member(entity_id, &schema.name(), &ty, block_timestamp).await | |
| } | |
| } | |
| // TODO: Add unit tests to cover the new functionality. |
Tools
GitHub Check: codecov/patch
[warning] 28-30: crates/torii/core/src/processors/store_update_member.rs#L28-L30
Added lines #L28 - L30 were not covered by tests
[warning] 32-34: crates/torii/core/src/processors/store_update_member.rs#L32-L34
Added lines #L32 - L34 were not covered by tests
[warning] 36-38: crates/torii/core/src/processors/store_update_member.rs#L36-L38
Added lines #L36 - L38 were not covered by tests
[warning] 40-43: crates/torii/core/src/processors/store_update_member.rs#L40-L43
Added lines #L40 - L43 were not covered by tests
[warning] 54-98: crates/torii/core/src/processors/store_update_member.rs#L54-L98
Added lines #L54 - L98 were not covered by tests
crates/torii/core/src/sql.rs
Outdated
| pub async fn set_model_member( | ||
| &mut self, | ||
| entity_id: Felt, | ||
| model_tag: &str, | ||
| member: &Ty, | ||
| block_timestamp: u64, | ||
| ) -> Result<()> { | ||
| let entity_id = format!("{:#x}", entity_id); | ||
| let path = vec![model_tag.to_string()]; | ||
| // update model member | ||
| self.build_set_entity_queries_recursive( | ||
| path, | ||
| &entity_id, | ||
| (&entity_id, false), | ||
| member, | ||
| block_timestamp, | ||
| &vec![], | ||
| ); | ||
| self.query_queue.execute_all().await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Robust implementation of set_model_member.
The function is well-implemented and crucial for updating model members. However, it lacks test coverage, which is essential for ensuring its reliability and correctness.
+ // TODO: Add unit tests to cover `set_model_member`.Would you like assistance in creating unit tests for this function, or should I open a GitHub issue to track this task?
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 async fn set_model_member( | |
| &mut self, | |
| entity_id: Felt, | |
| model_tag: &str, | |
| member: &Ty, | |
| block_timestamp: u64, | |
| ) -> Result<()> { | |
| let entity_id = format!("{:#x}", entity_id); | |
| let path = vec![model_tag.to_string()]; | |
| // update model member | |
| self.build_set_entity_queries_recursive( | |
| path, | |
| &entity_id, | |
| (&entity_id, false), | |
| member, | |
| block_timestamp, | |
| &vec![], | |
| ); | |
| self.query_queue.execute_all().await?; | |
| Ok(()) | |
| } | |
| pub async fn set_model_member( | |
| &mut self, | |
| entity_id: Felt, | |
| model_tag: &str, | |
| member: &Ty, | |
| block_timestamp: u64, | |
| ) -> Result<()> { | |
| let entity_id = format!("{:#x}", entity_id); | |
| let path = vec![model_tag.to_string()]; | |
| // update model member | |
| self.build_set_entity_queries_recursive( | |
| path, | |
| &entity_id, | |
| (&entity_id, false), | |
| member, | |
| block_timestamp, | |
| &vec![], | |
| ); | |
| self.query_queue.execute_all().await?; | |
| Ok(()) | |
| } | |
| // TODO: Add unit tests to cover `set_model_member`. |
Tools
GitHub Check: codecov/patch
[warning] 274-292: crates/torii/core/src/sql.rs#L274-L292
Added lines #L274 - L292 were not covered by tests
[warning] 294-295: crates/torii/core/src/sql.rs#L294-L295
Added lines #L294 - L295 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crates/torii/core/src/processors/store_update_member.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/store_update_member.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crates/torii/core/src/processors/store_update_member.rs (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/store_update_member.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- crates/torii/core/src/processors/store_update_member.rs (1 hunks)
- crates/torii/core/src/sql.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/store_update_member.rs
Additional comments not posted (2)
crates/torii/core/src/sql.rs (2)
Line range hint
586-692:
Visibility change tobuild_set_entity_queries_recursiveis appropriate.The visibility change to
build_set_entity_queries_recursiveis appropriate and necessary for its usage in other parts of the codebase.However, ensure that all function calls to
build_set_entity_queries_recursiveare correctly updated to reflect its new visibility.Verification successful
Visibility change to
build_set_entity_queries_recursiveis correctly reflected.The visibility change to
build_set_entity_queries_recursiveis appropriate and has been correctly updated in all occurrences within the file.
- All instances of
build_set_entity_queries_recursiveare withincrates/torii/core/src/sql.rsand reflect the updated visibility.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `build_set_entity_queries_recursive` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the updated visibility. rg --type rust -A 5 $'build_set_entity_queries_recursive'Length of output: 3292
276-299: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
set_model_membermatch the new signature.Verification successful
Function usage matches the new signature.
The function call to
set_model_memberinstore_update_member.rsmatches the new signature correctly.
entity_idis of typeFelt.falseis of typebool.schema.name()returns a&str.tyis of typeTy.event_idis of type&str.block_timestampis of typeu64.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `set_model_member` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'set_model_member'Length of output: 648
Script:
#!/bin/bash # Description: Extract the types of the variables used in the set_model_member function call in store_update_member.rs # Extract the definition of the function where set_model_member is called rg --type rust -A 20 'fn ' crates/torii/core/src/processors/store_update_member.rs # Extract the type of `schema` rg --type rust -A 5 'schema' crates/torii/core/src/processors/store_update_member.rs # Extract the type of `ty` rg --type rust -A 5 'ty' crates/torii/core/src/processors/store_update_member.rs # Extract the type of `event_id` rg --type rust -A 5 'event_id' crates/torii/core/src/processors/store_update_member.rs # Extract the type of `block_timestamp` rg --type rust -A 5 'block_timestamp' crates/torii/core/src/processors/store_update_member.rsLength of output: 4033
crates/torii/core/src/sql.rs
Outdated
| pub async fn set_model_member( | ||
| &mut self, | ||
| entity_id: Felt, | ||
| is_event_message: bool, | ||
| model_tag: &str, | ||
| member: &Ty, | ||
| event_id: &str, | ||
| block_timestamp: u64, | ||
| ) -> Result<()> { | ||
| let entity_id = format!("{:#x}", entity_id); | ||
| let path = vec![model_tag.to_string()]; | ||
| // update model member | ||
| self.build_set_entity_queries_recursive( | ||
| path, | ||
| event_id, | ||
| (&entity_id, is_event_message), | ||
| member, | ||
| block_timestamp, | ||
| &vec![], | ||
| ); | ||
| self.query_queue.execute_all().await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Reminder: Add tests.
The set_model_member function is well-implemented and crucial for updating model members. However, it lacks test coverage, which is essential for ensuring its reliability and correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crates/torii/core/src/sql.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/sql.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- crates/torii/core/src/sql.rs (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/sql.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (13)
- crates/dojo-core/src/world.cairo (3 hunks)
- crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json (2 hunks)
- crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml (1 hunks)
- crates/dojo-world/src/contracts/abi/world.rs (2 hunks)
- crates/torii/core/src/processors/store_update_member.rs (1 hunks)
- examples/spawn-and-move/Scarb.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json (2 hunks)
- examples/spawn-and-move/manifests/dev/base/dojo-world.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json (2 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.json (8 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.toml (5 hunks)
- examples/spawn-and-move/manifests/release/base/abis/dojo-world.json (2 hunks)
- examples/spawn-and-move/manifests/release/base/dojo-world.toml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/store_update_member.rs
Additional comments not posted (29)
crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/dojo-world.toml (1)
2-3: Verify the correctness of the new hash values.The
class_hashandoriginal_class_hashvalues have been updated. Ensure that these new hash values are correct and consistent with the updated class definitions.examples/spawn-and-move/manifests/dev/base/dojo-world.toml (1)
2-3: Verify the correctness of the new hash values.The
class_hashandoriginal_class_hashvalues have been updated. Ensure that these new hash values are correct and consistent with the updated class definitions.examples/spawn-and-move/manifests/release/base/dojo-world.toml (1)
2-3: Verify the correctness of the new hash values.The
class_hashandoriginal_class_hashvalues have been updated. Ensure that these new hash values are correct and consistent with the updated class definitions.examples/spawn-and-move/Scarb.toml (1)
46-46: Verify the correctness of the new world address.The
world_addressvalue has been updated. Ensure that this new address is correct and consistent with the intended environment.examples/spawn-and-move/manifests/dev/deployment/manifest.toml (7)
3-4: Verify the correctness of the new class hashes.Ensure that the new
class_hashandoriginal_class_hashvalues are correct and consistent with the intended updates.
6-6: Verify the correctness of the new address.Ensure that the new
addressvalue is correct and consistent with the intended updates.
7-7: Verify the correctness of the new transaction hash.Ensure that the new
transaction_hashvalue is correct and consistent with the intended updates.
26-26: Verify the correctness of the new address.Ensure that the new
addressvalue is correct and consistent with the intended updates.
43-43: Verify the correctness of the new address.Ensure that the new
addressvalue is correct and consistent with the intended updates.
57-57: Verify the correctness of the new address.Ensure that the new
addressvalue is correct and consistent with the intended updates.
71-71: Verify the correctness of the new address.Ensure that the new
addressvalue is correct and consistent with the intended updates.crates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.json (2)
978-1004: Verify the correctness and consistency of the new event structure.Ensure that the structure of the new event "StoreUpdateMember" is correct and consistent with the existing events.
1186-1190: Verify the correctness and consistency of the new event reference.Ensure that the reference to the new event "StoreUpdateMember" in the "events" section is correct and consistent with the existing events.
examples/spawn-and-move/manifests/dev/base/abis/dojo-world.json (2)
978-1004: Verify the correctness and consistency of the new event structure.Ensure that the structure of the new event "StoreUpdateMember" is correct and consistent with the existing events.
1186-1190: Verify the correctness and consistency of the new event reference.Ensure that the reference to the new event "StoreUpdateMember" in the "events" section is correct and consistent with the existing events.
examples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.json (2)
978-1004: New event definition looks good.The new event
StoreUpdateMemberis well-defined with appropriate member types.
1186-1190: Integration of new event in enum looks good.The
StoreUpdateMemberevent is correctly integrated within thedojo::world::world::Eventenum.examples/spawn-and-move/manifests/release/base/abis/dojo-world.json (2)
978-1004: LGTM! New eventStoreUpdateMemberis well-defined.The new event
StoreUpdateMemberwith memberstable,entity_id,member_selector, andvaluesis consistent with the existing event definitions.
1186-1190: LGTM! New eventStoreUpdateMemberadded to events array.The addition of
StoreUpdateMemberto the events array ensures proper recognition and processing.crates/dojo-world/src/contracts/abi/world.rs (2)
984-1010: LGTM! New eventStoreUpdateMemberis well-defined.The new event
StoreUpdateMemberwith memberstable,entity_id,member_selector, andvaluesis consistent with the existing event definitions.
1192-1196: LGTM! New eventStoreUpdateMemberadded toabigen!macro.The addition of
StoreUpdateMemberto theabigen!macro ensures proper recognition and processing.crates/dojo-core/src/world.cairo (3)
234-240: LGTM! Struct definition is correct.The
StoreUpdateMemberstruct is properly defined with appropriate field types.
872-877: LGTM! Function changes are correct.The
set_entityfunction correctly handles theModelIndex::MemberIdpattern match and emits theStoreUpdateMemberevent.
159-159: LGTM! Event emission logic is correct.The
emitfunction correctly handles custom event emissions.examples/spawn-and-move/manifests/dev/deployment/manifest.json (5)
4-5: Verify the correctness of the new class hash values.Ensure that the new
class_hashandoriginal_class_hashvalues are correct and consistent with the intended updates.Verification successful
The new class hash values are correctly updated and consistent.
The
class_hashandoriginal_class_hashvalues are identical and correctly formatted as per the intended updates in themanifest.jsonfile.
class_hash:0x7c5960ecfee50226cf08560851f996d91627a8893e9376613624c61838fb820original_class_hash:0x7c5960ecfee50226cf08560851f996d91627a8893e9376613624c61838fb820Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new class hash values. # Test: Search for the new class hash values. Expect: No mismatches. rg --type json '0x7c5960ecfee50226cf08560851f996d91627a8893e9376613624c61838fb820'Length of output: 391
1191-1195: Verify the correctness and completeness of the new event variantStoreUpdateMember.Ensure that the new event variant
StoreUpdateMemberis correct and complete with all necessary attributes.Verification successful
Verify the correctness and completeness of the new event variant
StoreUpdateMember.The
StoreUpdateMemberevent variant is correctly integrated and consistently formatted across multiple files in the codebase:
examples/spawn-and-move/manifests/dev/deployment/manifest.jsonexamples/spawn-and-move/manifests/dev/base/abis/dojo-world.jsonexamples/spawn-and-move/manifests/dev/deployment/abis/dojo-world.jsonexamples/spawn-and-move/manifests/release/base/abis/dojo-world.jsoncrates/dojo-lang/src/manifest_test_data/compiler_cairo/manifests/dev/base/abis/dojo-world.jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness and completeness of the new event variant `StoreUpdateMember`. # Test: Search for the new event variant. Expect: No mismatches. rg --type json 'StoreUpdateMember'Length of output: 1861
1224-1224: Verify the correctness of the new address value.Ensure that the new
addressvalue is correct and consistent with the intended updates.
1225-1225: Verify the correctness of the new transaction hash value.Ensure that the new
transaction_hashvalue is correct and consistent with the intended updates.
983-1009: Verify the correctness and completeness of the new event definitionStoreUpdateMember.Ensure that the new event definition
StoreUpdateMemberis correct and complete with all necessary member attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- crates/dojo-lang/src/event.rs (1 hunks)
- examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.toml (1 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.json (8 hunks)
- examples/spawn-and-move/manifests/dev/deployment/manifest.toml (5 hunks)
- examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml (1 hunks)
- examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.toml (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- examples/spawn-and-move/manifests/dev/deployment/manifest.json
- examples/spawn-and-move/manifests/dev/deployment/manifest.toml
Additional comments not posted (6)
examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.toml (1)
2-3: Update class_hash and original_class_hash values.The new
class_hashandoriginal_class_hashvalues have been updated correctly. Ensure that these new values are consistent with the intended contract updates.Verification successful
Update class_hash and original_class_hash values.
The new
class_hashandoriginal_class_hashvalues have been updated correctly across multiple files in the repository. The updated values are consistent with the intended contract updates.
examples/spawn-and-move/manifests/dev/deployment/manifest.tomlexamples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.tomlexamples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.tomlScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of new class_hash and original_class_hash values. # Test: Search for the new hash values in the repository. Expect: Consistent usage of the new hash values. rg --type toml '0x112caad1aed1e3d21fc70eb118db9ff75632f1e3f6c2f387d116fcb6e1eee4'Length of output: 1074
examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.toml (1)
2-3: Update class_hash and original_class_hash values.The new
class_hashandoriginal_class_hashvalues have been updated correctly. Ensure that these new values are consistent with the intended contract updates.Verification successful
The new
class_hashandoriginal_class_hashvalues are consistent.The specific hash values
0x112caad1aed1e3d21fc70eb118db9ff75632f1e3f6c2f387d116fcb6e1eee4are used consistently across the related files in the repository.
examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-others-61de2c18.tomlexamples/spawn-and-move/manifests/dev/deployment/manifest.tomlexamples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-others-61de2c18.tomlThese values align with the intended contract updates.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of new class_hash and original_class_hash values. # Test: Search for the new hash values in the repository. Expect: Consistent usage of the new hash values. rg --type toml '0x112caad1aed1e3d21fc70eb118db9ff75632f1e3f6c2f387d116fcb6e1eee4'Length of output: 1074
Script:
#!/bin/bash # Description: Verify if there are any other class_hash or original_class_hash values in the repository. # Test: Search for any class_hash or original_class_hash values in the repository. rg --type toml 'class_hash = "0x' --type toml 'original_class_hash = "0x'Length of output: 141
Script:
#!/bin/bash # Description: Verify if there are any other class_hash or original_class_hash values in the repository. # Test: Search for any class_hash values in the repository. rg --type toml 'class_hash = "0x' # Test: Search for any original_class_hash values in the repository. rg --type toml 'original_class_hash = "0x'Length of output: 30814
examples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.toml (1)
2-3: Update class_hash and original_class_hash values.The new
class_hashandoriginal_class_hashvalues have been updated correctly. Ensure that these new values are consistent with the intended contract updates.examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml (1)
2-3: Update class_hash and original_class_hash values.The new
class_hashandoriginal_class_hashvalues have been updated correctly. Ensure that these new values are consistent with the intended contract updates.Verification successful
The new
class_hashandoriginal_class_hashvalues are used consistently across the relevant files.
examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.tomlexamples/spawn-and-move/manifests/dev/deployment/manifest.tomlexamples/spawn-and-move/manifests/dev/base/contracts/dojo_examples-actions-40b6994c.tomlexamples/spawn-and-move/manifests/dev/deployment/manifest.jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of new class_hash and original_class_hash values. # Test: Search for the new hash values in the repository. Expect: Consistent usage of the new hash values. rg --type toml '0x7b59e8f1039dd99e39af7695bbbd5bcc4ffd7852f54576fe4acf89590298c01'Length of output: 1085
Script:
#!/bin/bash # Description: Search for references to the intended contract updates that mention the class_hash and original_class_hash values. # Test: Search for the hash values in the repository, including documentation and other relevant files. rg '0x7b59e8f1039dd99e39af7695bbbd5bcc4ffd7852f54576fe4acf89590298c01'Length of output: 1386
crates/dojo-lang/src/event.rs (2)
Line range hint
1-84:
LGTM!The rest of the
handle_event_structfunction remains unchanged and appears to be logically consistent and correct.Also applies to: 87-146
85-86: Verify the definition and accessibility ofselectormethod.The change replaces
selector!("$struct_name$")withdojo::model::Model::<$struct_name$>::selector(). Ensure that theselectormethod is correctly defined and accessible withindojo::model::Model.
| async fn process( | ||
| &self, | ||
| _world: &WorldContractReader<P>, | ||
| db: &mut Sql, | ||
| _block_number: u64, | ||
| block_timestamp: u64, | ||
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | ||
| event_id: &str, | ||
| event: &Event, | ||
| ) -> Result<(), Error> { | ||
| let selector = event.data[MODEL_INDEX]; | ||
| let entity_id = event.data[ENTITY_ID_INDEX]; | ||
| let member_selector = event.data[MEMBER_INDEX]; | ||
|
|
||
| let model = db.model(selector).await?; | ||
| let schema = model.schema().await?; | ||
|
|
||
| let mut member = schema | ||
| .as_struct() | ||
| .expect("model schema must be a struct") | ||
| .children | ||
| .iter() | ||
| .find(|c| { | ||
| get_selector_from_name(&c.name).expect("invalid selector for member name") | ||
| == member_selector | ||
| }) | ||
| .context("member not found")? | ||
| .clone(); | ||
|
|
||
| info!( | ||
| target: LOG_TARGET, | ||
| name = %model.name(), | ||
| entity_id = format!("{:#x}", entity_id), | ||
| member = %member.name, | ||
| "Store update member.", | ||
| ); | ||
|
|
||
| let values_start = MEMBER_INDEX + 1; | ||
| let values_end: usize = | ||
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | ||
|
|
||
| // Skip the length to only get the values as they will be deserialized. | ||
| let mut values = event.data[values_start + 1..=values_end].to_vec(); | ||
|
|
||
| let tag = naming::get_tag(model.namespace(), model.name()); | ||
|
|
||
| if !db.does_entity_exist(tag.clone(), entity_id).await? { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| tag, | ||
| entity_id = format!("{:#x}", entity_id), | ||
| "Entity not found, must be set before updating a member.", | ||
| ); | ||
|
|
||
| return Ok(()); | ||
| } | ||
|
|
||
| member.ty.deserialize(&mut values)?; | ||
|
|
||
| db.set_model_member(entity_id, false, &schema.name(), &member, event_id, block_timestamp) | ||
| .await | ||
| } |
There was a problem hiding this comment.
Comprehensive implementation of process.
The process function is well-implemented with clear steps for handling store update member events. However, the codecov warnings indicate missing test coverage which is crucial for ensuring the functionality works as expected.
+ // TODO: Add unit tests to cover the new functionality.Would you like me to help in writing some initial unit tests or open a GitHub issue to track this task?
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.
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| block_timestamp: u64, | |
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | |
| event_id: &str, | |
| event: &Event, | |
| ) -> Result<(), Error> { | |
| let selector = event.data[MODEL_INDEX]; | |
| let entity_id = event.data[ENTITY_ID_INDEX]; | |
| let member_selector = event.data[MEMBER_INDEX]; | |
| let model = db.model(selector).await?; | |
| let schema = model.schema().await?; | |
| let mut member = schema | |
| .as_struct() | |
| .expect("model schema must be a struct") | |
| .children | |
| .iter() | |
| .find(|c| { | |
| get_selector_from_name(&c.name).expect("invalid selector for member name") | |
| == member_selector | |
| }) | |
| .context("member not found")? | |
| .clone(); | |
| info!( | |
| target: LOG_TARGET, | |
| name = %model.name(), | |
| entity_id = format!("{:#x}", entity_id), | |
| member = %member.name, | |
| "Store update member.", | |
| ); | |
| let values_start = MEMBER_INDEX + 1; | |
| let values_end: usize = | |
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | |
| // Skip the length to only get the values as they will be deserialized. | |
| let mut values = event.data[values_start + 1..=values_end].to_vec(); | |
| let tag = naming::get_tag(model.namespace(), model.name()); | |
| if !db.does_entity_exist(tag.clone(), entity_id).await? { | |
| warn!( | |
| target: LOG_TARGET, | |
| tag, | |
| entity_id = format!("{:#x}", entity_id), | |
| "Entity not found, must be set before updating a member.", | |
| ); | |
| return Ok(()); | |
| } | |
| member.ty.deserialize(&mut values)?; | |
| db.set_model_member(entity_id, false, &schema.name(), &member, event_id, block_timestamp) | |
| .await | |
| } | |
| async fn process( | |
| &self, | |
| _world: &WorldContractReader<P>, | |
| db: &mut Sql, | |
| _block_number: u64, | |
| block_timestamp: u64, | |
| _transaction_receipt: &TransactionReceiptWithBlockInfo, | |
| event_id: &str, | |
| event: &Event, | |
| ) -> Result<(), Error> { | |
| let selector = event.data[MODEL_INDEX]; | |
| let entity_id = event.data[ENTITY_ID_INDEX]; | |
| let member_selector = event.data[MEMBER_INDEX]; | |
| let model = db.model(selector).await?; | |
| let schema = model.schema().await?; | |
| let mut member = schema | |
| .as_struct() | |
| .expect("model schema must be a struct") | |
| .children | |
| .iter() | |
| .find(|c| { | |
| get_selector_from_name(&c.name).expect("invalid selector for member name") | |
| == member_selector | |
| }) | |
| .context("member not found")? | |
| .clone(); | |
| info!( | |
| target: LOG_TARGET, | |
| name = %model.name(), | |
| entity_id = format!("{:#x}", entity_id), | |
| member = %member.name, | |
| "Store update member.", | |
| ); | |
| let values_start = MEMBER_INDEX + 1; | |
| let values_end: usize = | |
| values_start + event.data[values_start].to_usize().context("invalid usize")?; | |
| // Skip the length to only get the values as they will be deserialized. | |
| let mut values = event.data[values_start + 1..=values_end].to_vec(); | |
| let tag = naming::get_tag(model.namespace(), model.name()); | |
| if !db.does_entity_exist(tag.clone(), entity_id).await? { | |
| warn!( | |
| target: LOG_TARGET, | |
| tag, | |
| entity_id = format!("{:#x}", entity_id), | |
| "Entity not found, must be set before updating a member.", | |
| ); | |
| return Ok(()); | |
| } | |
| member.ty.deserialize(&mut values)?; | |
| db.set_model_member(entity_id, false, &schema.name(), &member, event_id, block_timestamp) | |
| .await | |
| } | |
| // TODO: Add unit tests to cover the new functionality. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
crates/torii/core/src/sql.rs (1)
Line range hint
591-680:
Optimize the logic for updating members.The current logic for updating members could be optimized to reduce redundancy and improve readability.
- let statement = if is_store_update_member { - // row has to exist. update it directly - format!( - "UPDATE [{table_id}] SET {updates} WHERE entity_id = ?", - table_id = table_id, - updates = columns - .iter() - // skip id column - .skip(1) - .zip(placeholders.iter().skip(1)) - .map(|(column, placeholder)| format!("{} = {}", column, placeholder)) - .collect::<Vec<String>>() - .join(", ") - ) - } else { - format!( - "INSERT OR REPLACE INTO [{table_id}] ({}) VALUES ({})", - columns.join(","), - placeholders.join(",") - ) - }; + let statement = format!( + "{} INTO [{table_id}] ({}) VALUES ({})", + if is_store_update_member { "UPDATE" } else { "INSERT OR REPLACE" }, + columns.join(","), + placeholders.join(",") + );
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
crates/torii/core/src/sql.rs (2)
418-424: Add a comment explaining the SQL query.Adding a comment to explain the SQL query would enhance code readability and maintainability.
+ // Check if the entity exists in the specified model table.
595-601: Add comments to explain complex logic.Adding comments to explain the complex logic in this function would enhance code readability and maintainability.
+ // Extract the entity ID and event message flag. + let (entity_id, is_event_message) = entity_id;
| @@ -271,6 +274,35 @@ impl Sql { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub async fn set_model_member( | |||
There was a problem hiding this comment.
Ohayo, Sensei! Reminder: Add tests.
The set_model_member function is well-implemented and crucial for updating model members. However, it lacks test coverage, which is essential for ensuring its reliability and correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
There was a problem hiding this comment.
Thanks @Larkooo for the fixes here. @lambda-0x we will plan testing of this at both integration and unit.
support store update member for store updates that only updates a single model member values
Summary by CodeRabbit
New Features
StoreUpdateMember, facilitating detailed updates within the application.Enhancements
PlayerItemstructure for managing player items.