fix(torii-core): remove old check for StoreUpdateMember processor #2858
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2858 +/- ##
==========================================
- Coverage 55.82% 55.73% -0.09%
==========================================
Files 446 446
Lines 57725 57816 +91
==========================================
- Hits 32227 32226 -1
- Misses 25498 25590 +92 ☔ View full report in Codecov by Sentry. |
|
@Larkooo seems that we've an issue with the parallelization in some situations. I've updated the Here's the setup: The error it yields: The table is correctly created though when checking the DB manually. Could it be related to the batch of queries into the db's transaction? Once Torii runs and displays error for the first update, the subsequent updates are ok since the table exists. Also, if Torii is running when the transactions are sent, it works well. The issue is observed when Torii is started, and the events are processed in batch. |
WalkthroughOhayo, sensei! The changes involve updates across multiple files in the Torii core and spawn-and-move example project. The modifications primarily focus on simplifying event processing, removing some constants, and adding a new method to update player configuration names. The changes streamline code structure and introduce more direct ways of handling player-related operations. Changes
Sequence DiagramsequenceDiagram
participant Player
participant Actions
participant PlayerConfig
Player->>Actions: update_player_config_name(newName)
Actions->>PlayerConfig: Directly update name
PlayerConfig-->>Actions: Name updated
Actions-->>Player: Confirmation
The sequence diagram illustrates the new method for updating a player's configuration name, showing a direct update process without intermediate steps. 🪧 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: 0
🧹 Nitpick comments (8)
examples/spawn-and-move/src/actions.cairo (1)
135-144: Ohayo sensei! Ensure data validation before writing to the model.
While directly overwritingnameis straightforward, it might be helpful to:
- Validate the
ByteArray(e.g., ensure it isn’t empty or overly long).- Provide error handling or revert logic in case invalid data is provided.
Consider adding short, inlined checks to avoid unexpected states in your model.
crates/torii/core/src/processors/store_update_member.rs (2)
42-54: Consider more graceful handling instead of panickingOhayo sensei, this block currently panics if the event doesn't match the expected structure. While it might be unlikely in practice, panicking here could bring down the entire processor under unexpected circumstances. A more robust alternative might involve logging an error and returning early:
let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { - panic!( - "Expected {} event to be well formed.", - <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self) - ) + tracing::error!( + target: LOG_TARGET, + "Invalid event structure for: {}", + <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self) + ); + return; }) { WorldEvent::StoreUpdateMember(e) => e, _ => { unreachable!() } };
97-99: Consider concurrency implications or an optional transactionOhayo sensei, while storing entities is straightforward, if you anticipate concurrent insertions or updates, wrapping this in a transaction or verifying concurrency handling may help prevent data races or partial writes.
crates/torii/core/src/processors/store_update_record.rs (1)
27-27: Add meaningful validation logicOhayo sensei, this method unconditionally returns
true, making the validation step effectively moot. If you intend to verify the structure ofevent, consider adding checks here or removing the method entirely if it's no longer needed.crates/torii/core/src/sql/test.rs (4)
212-215: Initialize the testing environment earlier to avoid race conditions.Creating a new workspace and building config right before database initialization can occasionally race with asynchronous tasks. You might want to verify or log that the tasks have completed before proceeding.
249-256: Use meaningful data in the ByteArray to test real-world scenarios.Empty ByteArrays might not reflect actual usage patterns. Try setting initial config fields to something more representative of real data to further validate the system's correctness and resilience.
263-275: Validate concurrency for large updates.Updating the player's name works for a single short string “mimi,” but concurrency or bigger updates might expose issues. Consider concurrency tests or stress tests with random/wide updates to ensure system stability.
303-315: Ohayo sensei! Ensure table name references remain consistent.Here,
[ns-PlayerConfig]is queried based on a dynamic table name. If future refactors rename or remove the “ns” namespace, the query could become outdated. Adding a mechanism to fetch the model name from a config or schema registry will help maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/torii/core/Cargo.toml(0 hunks)crates/torii/core/src/processors/mod.rs(0 hunks)crates/torii/core/src/processors/store_update_member.rs(3 hunks)crates/torii/core/src/processors/store_update_record.rs(1 hunks)crates/torii/core/src/sql/mod.rs(0 hunks)crates/torii/core/src/sql/test.rs(2 hunks)examples/spawn-and-move/manifest_dev.json(3 hunks)examples/spawn-and-move/src/actions.cairo(3 hunks)
💤 Files with no reviewable changes (3)
- crates/torii/core/Cargo.toml
- crates/torii/core/src/processors/mod.rs
- crates/torii/core/src/sql/mod.rs
🔇 Additional comments (11)
examples/spawn-and-move/src/actions.cairo (2)
8-8: Ohayo sensei! Great addition of theupdate_player_config_namefunction.
By exposing a direct method to update the player configuration name, you simplify usage for the caller. This addition looks consistent with theIActionstrait’s responsibilities.
26-26: Ohayo sensei! Nice import for Model usage.
Importing theModeltrait is crucial for direct member writes. This is a neat addition that helps keep your code more concise and organized.examples/spawn-and-move/manifest_dev.json (3)
1255-1256: Ohayo sensei! Double-check contract redeployment references.
Updating the contract address and class hash typically requires re-deployment or upgrade steps. Ensure references in other config files and scripts remain valid post-update.
1402-1413: Ohayo sensei! The new manifest entry forupdate_player_config_namematches your Cairo code changes.
This addition to the interface allows external calls to update a player’s configuration name. Everything looks consistent and well-mapped.
1600-1600: Ohayo sensei! Good job adding the system method.
Listingupdate_player_config_nameas a system function ensures the method is externally accessible as intended.crates/torii/core/src/processors/store_update_member.rs (5)
4-4: Ohayo sensei, this import ofWorldEventfromabigenlooks clean! No compatibility issues detected.
9-9: Ohayo sensei, the addition ofuse tracing::info;is consistent with the rest of the tracing logs.
56-58: Ohayo sensei, extractingmodel_selector,entity_id, andmember_selectorin these lines is straightforward and consistent with the new event structure.
62-62: Ohayo sensei, silently ignoring missing models is aligned with an opt-in indexing strategy. This helps ignore events for untracked models without causing errors.
94-94: Ohayo sensei, copyingvaluesinto a mutable vector is a typical approach for deserialization here. No issues noted.crates/torii/core/src/sql/test.rs (1)
208-209: Ohayo sensei! Consider investigating the root cause of flakiness.Marking the test as
#[ignore]due to flakiness may conceal underlying concurrency or timing issues. If possible, gather logs and investigate whether the processor is fully caught up or if the table creation is delayed.Do you want guidance on adding logs or extra synchronization steps to diagnose the cause of flakiness?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/core/src/processors/store_update_member.rs(4 hunks)
🔇 Additional comments (4)
crates/torii/core/src/processors/store_update_member.rs (4)
4-4: Ohayo! Nice improvement using WorldEvent!The switch to using the generated WorldEvent type is a good structural improvement that makes the code more type-safe and maintainable.
28-30: Ohayo! Validation simplification looks good, sensei!The simplification of the validate method aligns with other processors since validation is now handled by auto-generated deserialization.
94-99: Clean and consistent member update implementation!The member update logic is now more direct and uses model_selector consistently. Good improvement in code clarity!
Line range hint
62-70: Verify table existence handling in parallel scenarios.The silent handling of missing models could be related to the table existence issues mentioned in the PR objectives, particularly during parallel processing.
Let's verify if this is related to the reported "no such table" errors:
| // Torii version is coupled to the world version, so we can expect the event to be well | ||
| // formed. | ||
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { | ||
| WorldEvent::StoreUpdateMember(e) => e, | ||
| _ => { | ||
| unreachable!() | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider graceful error handling for parallel processing, sensei!
The panic on malformed events could be problematic in a parallel processing context. While the comment suggests that Torii version is coupled to the world version, this assumption might not hold during parallel processing or upgrades.
Consider this more resilient approach:
- let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self)
- )
- }) {
+ let event = match WorldEvent::try_from(event) {
+ Ok(event) => match event {
+ WorldEvent::StoreUpdateMember(e) => e,
+ _ => return Err(Error::msg("Unexpected event type")),
+ },
+ Err(e) => return Err(Error::msg(format!(
+ "Malformed {} event: {}",
+ <StoreUpdateMemberProcessor 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.
| // Torii version is coupled to the world version, so we can expect the event to be well | |
| // formed. | |
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | |
| panic!( | |
| "Expected {} event to be well formed.", | |
| <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self) | |
| ) | |
| }) { | |
| WorldEvent::StoreUpdateMember(e) => e, | |
| _ => { | |
| unreachable!() | |
| } | |
| }; | |
| // Torii version is coupled to the world version, so we can expect the event to be well | |
| // formed. | |
| let event = match WorldEvent::try_from(event) { | |
| Ok(event) => match event { | |
| WorldEvent::StoreUpdateMember(e) => e, | |
| _ => return Err(Error::msg("Unexpected event type")), | |
| }, | |
| Err(e) => return Err(Error::msg(format!( | |
| "Malformed {} event: {}", | |
| <StoreUpdateMemberProcessor as EventProcessor<P>>::event_key(self), | |
| e | |
| ))), | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/sozo/tests/test_data/policies.json (1)
86-133: Consider documenting the purpose of each contract address, sensei! 📝While the permissions are correctly configured, it would be helpful to add comments or documentation explaining the purpose of each contract address and its associated permissions. This would make future maintenance easier.
Consider adding a comment block at the top of the file like this:
+ /* + * Contract Addresses: + * 0x50b1... - StoreUpdateMember processor + * 0x6bfba... - Player management system + * ... + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcrates/torii/types-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
Cargo.toml(1 hunks)bin/sozo/tests/test_data/policies.json(1 hunks)
🔇 Additional comments (4)
Cargo.toml (1)
76-77: Ohayo sensei! Let's verify the cainome dependency versions.The update from v0.4.10 to v0.4.11 for both cainome dependencies looks good, but let's ensure we're using the latest stable version.
Would you mind adding a comment explaining why these specific versions were chosen? This helps future maintainers understand the rationale behind version updates.
[workspace.dependencies] -cainome = { git = "https://github.com/cartridge-gg/cainome", tag = "v0.4.11", features = [ "abigen-rs" ] } -cainome-cairo-serde = { git = "https://github.com/cartridge-gg/cainome", tag = "v0.4.11" } +# Updated to v0.4.11 to fix StoreUpdateMember processor indexing issues +cainome = { git = "https://github.com/cartridge-gg/cainome", tag = "v0.4.11", features = [ "abigen-rs" ] } +cainome-cairo-serde = { git = "https://github.com/cartridge-gg/cainome", tag = "v0.4.11" }✅ Verification successful
Ohayo sensei! The cainome dependencies are already at the latest version!
Based on the GitHub tags, v0.4.11 is indeed the most recent version available in the cartridge-gg/cainome repository. The update from v0.4.10 to v0.4.11 is spot on!
The suggestion to add a comment explaining the version update rationale still stands as a nitpick, but the version choice itself is perfect.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check latest tags from the cainome repository gh api \ -H "Accept: application/vnd.github+json" \ repos/cartridge-gg/cainome/tags \ --jq '.[].name' | head -n 5Length of output: 146
bin/sozo/tests/test_data/policies.json (3)
86-93: Ohayo! New upgrade permissions look good, sensei! 🎯The addition of upgrade permissions for the new contract addresses aligns with the PR objectives of fixing the StoreUpdateMember processor and addressing indexing failures.
126-133: Additional upgrade permissions are properly configured, sensei! ✨The upgrade permissions for the remaining addresses look good and follow the same pattern as the earlier upgrade permissions.
94-125: Verify player-related permissions, sensei! 🔍The new permissions for address
0x6bfba7include a comprehensive set of player-related operations, including the newupdate_player_config_namemethod. This aligns with the changes mentioned in the AI summary regarding player configuration management.However, let's verify that all required permissions are properly configured to prevent any startup issues with Torii.
✅ Verification successful
All player permissions are properly configured, sensei! ✨
Ohayo! I've verified the player-related permissions and found everything in perfect order:
- No duplicate permissions were found that could cause conflicts
- All essential player methods are present, including:
- Core actions: spawn, move, enter_dungeon
- Configuration management: set_player_config, update_player_config_name, reset_player_config
- Profile and models: set_player_server_profile, set_models
- Character progression: upgrade
The permissions are well-structured and include the new
update_player_config_namemethod along with all other necessary player-related operations. This setup should work smoothly with Torii.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all required player-related methods are properly configured # and that there are no duplicate permissions that might cause conflicts # Check for any duplicate method permissions for the same target jq '[.[] | {target: .target, method: .method}] | group_by({target: .target, method: .method}) | map(select(length > 1))' bin/sozo/tests/test_data/policies.json # List all player-related methods to ensure completeness jq '.[] | select(.target == "0x6bfba78b8f4f42da469860a95291c2fad959c91ea747e2062de763ff4e62c4a") | .method' bin/sozo/tests/test_data/policies.jsonLength of output: 465
glihm
changed the title
Old checks on the
StoreUpdateMemberprocessor make the indexing fail.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests