fix(sozo): ensure correct event fetching + namespace whitelisting + guest mode #2937
Conversation
…espaces whitelisting
WalkthroughThe pull request introduces enhancements to Dojo's migration and event processing capabilities. Key changes include adding support for guest migrations, implementing namespace whitelisting, and refactoring event handling logic. The modifications span multiple files across the Dojo ecosystem, focusing on improving flexibility in world migration, event filtering, and error handling. Changes
Possibly related PRs
Suggested Labels
Ohayo, sensei! This PR looks like a solid enhancement to Dojo's migration and event processing capabilities. The changes introduce more flexible world migration options and improve the robustness of event handling. Keep coding awesome! 🚀 🪧 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 (6)
bin/sozo/src/commands/options/world.rs (2)
18-21: Ohayo! Consider enhancing the guest mode documentation, sensei.The help text could be more explicit about security implications and limitations of guest mode migrations.
Consider updating the help text to:
- #[arg(help = "Whether the migration is a guest migration, which means the migration is \ - performed on a world you are not the owner of.")] + #[arg(help = "Enable guest mode to perform migrations on worlds you don't own. Note: Guest mode \ + skips world ownership verification and only affects local resources. Use with caution.")]
23-24: The namespaces help text could use more detail, sensei!The current help text doesn't explain the purpose or benefits of namespace whitelisting.
Consider updating the help text to:
- #[arg(long, default_value = "", help = "Whitelisted namespaces.")] + #[arg(long, default_value = "", help = "Comma-separated list of namespaces to whitelist. \ + Optimizes RAM usage and prevents DDoS risks by filtering events to specified namespaces only.")]xtask/generate-test-db/src/main.rs (1)
65-74: Consider reducing code duplication, sensei!The initialization of
whitelisted_namespacesandis_guestis duplicated in both migration functions. Consider extracting these into a helper function.+fn create_migration( + world_address: Felt, + world_local: WorldLocal, + runner: &KatanaRunner, + txn_config: TxnConfig, + profile_config: ProfileConfig, +) -> Result<Migration> { + let whitelisted_namespaces = vec![]; + let is_guest = false; + + let world_diff = WorldDiff::new_from_chain( + world_address, + world_local, + &runner.provider(), + None, + &whitelisted_namespaces, + ).await?; + + Ok(Migration::new( + world_diff, + WorldContract::new(world_address, &runner.account(0)), + txn_config, + profile_config, + runner.url().to_string(), + is_guest, + )) +}Also applies to: 75-83, 123-132, 133-141
bin/sozo/src/commands/events.rs (1)
382-386: Clean helper function implementation, sensei!The
get_tagfunction nicely consolidates the tag retrieval logic. Consider adding documentation to explain the "external-" prefix for unknown selectors./// Returns the tag for a selector, or the selector itself if it's not found. +/// If the selector is not found in the tags map, returns "external-{selector}" +/// to indicate that it's an external selector. #[inline] fn get_tag(selector: Felt, tags: &HashMap<&Felt, String>) -> String {crates/sozo/ops/src/migrate/mod.rs (2)
95-95: Consider adding documentation for guest mode behavior, sensei!While the implementation is correct, it would be helpful to add documentation explaining:
- The purpose of guest mode
- When to use guest mode
- The implications of skipping world verification
Add a doc comment like this:
+ /// When running in guest mode, world verification is skipped. This is useful when: + /// - Migrating in environments where the user is not the owner + /// - Focusing solely on local resource verification + /// - Bypassing world ownership checks let world_has_changed = if !self.guest { self.ensure_world(ui).await? } else { false };
Line range hint
1-1000: Consider adding integration tests for guest mode, sensei!The changes would benefit from integration tests that verify:
- Migration behavior in guest mode
- Error handling when world verification is skipped
- Interaction with other migration features
Would you like me to help create integration tests for guest mode functionality?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bin/sozo/src/commands/events.rs(10 hunks)bin/sozo/src/commands/migrate.rs(2 hunks)bin/sozo/src/commands/options/world.rs(1 hunks)bin/sozo/src/utils.rs(2 hunks)crates/dojo/world/src/diff/mod.rs(3 hunks)crates/dojo/world/src/remote/events_to_remote.rs(28 hunks)crates/sozo/ops/src/migrate/mod.rs(3 hunks)crates/sozo/ops/src/tests/migration.rs(2 hunks)xtask/generate-test-db/src/main.rs(2 hunks)
🔇 Additional comments (13)
bin/sozo/src/commands/migrate.rs (1)
61-62: LGTM! Clean implementation of guest mode parameter passing, sensei.The code correctly extracts the guest flag from options and passes it through to the Migration constructor.
Also applies to: 84-84
crates/sozo/ops/src/tests/migration.rs (1)
37-46: Consider adding test cases for the new features, sensei!While the implementation is correct, we should add test cases to verify:
- Guest mode migrations
- Namespace whitelisting functionality
Would you like me to help generate additional test cases for these scenarios?
Also applies to: 63-64, 71-71
bin/sozo/src/utils.rs (2)
58-58: Ohayo! Nice guest mode handling, sensei!The addition of the
!world.guestcheck elegantly suppresses the address discrepancy warning during guest migrations, which is exactly what we want for this mode.
169-169: Looking good, sensei! Namespace whitelisting properly integrated.The addition of
&world.namespacesparameter correctly implements the namespace whitelisting feature at the world diff creation level.bin/sozo/src/commands/events.rs (2)
262-275: Nice error handling improvement, sensei!The addition of proper error handling for
model_getwith a fallback to "NOT_AVAILABLE" makes the code more robust and user-friendly.
Line range hint
36-40: Verify Katana's continuation token behavior, sensei.The special handling for Katana's event fetching looks reasonable, but let's verify the behavior as noted in the TODO comment.
Also applies to: 98-102
✅ Verification successful
Ohayo! Katana's continuation token implementation is solid, sensei!
The special handling for Katana's event fetching is well-implemented with proper support for both native and forked continuation tokens. The code includes:
- Proper null token handling through Rust's Option type
- Clear distinction between native and forked tokens
- Comprehensive test coverage validating the behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Katana returns null continuation token for empty pages # Note: This is a theoretical check as we can't actually run Katana in the sandbox # Search for related tests or documentation rg -A 5 "continuation_token.*Katana|Katana.*continuation_token" # Look for any existing issues about this behavior gh issue list --search "Katana continuation token"Length of output: 118
Script:
#!/bin/bash # Search for Katana-related event handling patterns rg -l "Katana.*events|events.*Katana" # Look for TODO comments in events.rs cat bin/sozo/src/commands/events.rs | grep -i "todo" # Search for event-related implementations ast-grep --pattern 'match $_.chain_id { $$$ }'Length of output: 391
Script:
#!/bin/bash # Check the event primitives implementation cat crates/katana/primitives/src/event.rs # Look for test files related to events fd -g "*event*test*.rs" # Search for continuation token implementations rg -A 5 "continuation" crates/katana/primitives/src/event.rsLength of output: 8817
crates/dojo/world/src/diff/mod.rs (2)
174-180: Great logging enhancement, sensei!The addition of detailed trace logging for the "World not deployed" case improves debugging capabilities.
191-195: Clean namespace whitelisting implementation, sensei!The conversion of empty whitelist to None is elegant and maintains good semantics - empty whitelist means no filtering.
crates/dojo/world/src/remote/events_to_remote.rs (2)
161-166: Excellent namespace filtering implementation, sensei!The consistent pattern of namespace whitelisting checks with debug logging provides good visibility into filtered events while maintaining clean control flow with early returns.
Also applies to: 171-179, 196-203, 220-228
361-370: Clean helper function, sensei!The
is_whitelistedfunction is well-documented and handles the None case elegantly, defaulting to true when no whitelist is provided.crates/sozo/ops/src/migrate/mod.rs (3)
61-61: Ohayo! The guest field addition looks good, sensei!The boolean field is appropriately placed in the Migration struct and follows Rust naming conventions.
81-83: Constructor update maintains clean initialization, sensei!The
guestparameter is correctly added to the constructor and properly initialized in the struct.
Line range hint
1-1000: Verify error handling for guest mode migrations, sensei!Let's ensure that guest mode migrations handle errors appropriately when world verification is skipped.
✅ Verification successful
Ohayo sensei! After reviewing the guest mode implementation, I can confirm that the error handling is properly implemented. Here's why:
Guest mode error handling is correctly implemented ✅
- World verification is cleanly skipped while maintaining proper error handling for all other operations
- Error propagation is intact for resource syncing, permissions, and contract initialization
- The implementation matches the documented behavior for guest migrations on worlds you don't own
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in guest mode context rg -A 5 "guest.*migrate|migrate.*guest" # Look for potential error cases in test files rg -g "test_*.rs" "guest.*mode|mode.*guest"Length of output: 87
Script:
#!/bin/bash # Search for Migration struct definition and guest field ast-grep --pattern 'struct Migration { $$$ guest: bool, $$$ }' # Search for guest field usage in conditional statements ast-grep --pattern 'if $$.guest { $$$ }' # Search for guest field usage in negated conditions ast-grep --pattern 'if !$$.guest { $$$ }' # Look for test files with guest mode setup rg -g "**/*test*.rs" -B 2 -A 5 "guest.*=.*true|guest:.*true"Length of output: 224
Script:
#!/bin/bash # Search for guest mode usage with context rg -B 3 -A 3 "guest" # Search for error handling in migration rg -B 2 -A 2 "MigrationError::" # Look for ensure_world error handling rg -B 3 -A 3 "ensure_world" # Search for error type definitions rg -B 1 -A 5 "pub enum MigrationError"Length of output: 8989
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2937 +/- ##
==========================================
- Coverage 55.81% 55.78% -0.04%
==========================================
Files 444 444
Lines 57329 57433 +104
==========================================
+ Hits 32000 32040 +40
- Misses 25329 25393 +64 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/sozo/src/commands/options/world.rs (1)
17-29: Consider adding tests for the new fields, sensei! 📝While the existing tests cover world_address functionality well, we should add tests for:
- Guest mode default value and CLI override
- Namespace parsing with various inputs (empty, single, multiple)
Here's a suggested test implementation:
#[test] fn guest_mode_defaults_to_false() { let cmd = Command::parse_from([""]); assert!(!cmd.inner.guest); } #[test] fn guest_mode_can_be_enabled() { let cmd = Command::parse_from(["sozo", "--guest"]); assert!(cmd.inner.guest); } #[test] fn namespaces_empty_by_default() { let cmd = Command::parse_from([""]); assert!(cmd.inner.namespaces.is_empty()); } #[test] fn namespaces_single_value() { let cmd = Command::parse_from(["sozo", "--namespaces", "namespace1"]); assert_eq!(cmd.inner.namespaces, vec!["namespace1"]); } #[test] fn namespaces_multiple_values() { let cmd = Command::parse_from(["sozo", "--namespaces", "namespace1,namespace2"]); assert_eq!(cmd.inner.namespaces, vec!["namespace1", "namespace2"]); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/options/world.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: docs
🔇 Additional comments (2)
bin/sozo/src/commands/options/world.rs (2)
18-21: Ohayo! The guest mode implementation looks clean, sensei! 🎎The boolean flag with its default value and descriptive help message is well-implemented. The documentation clearly explains its purpose for non-owner world migrations.
23-29: Nice implementation of namespace whitelisting, sensei! 🎋The Vec implementation with comma-separated CLI input is a good choice. The help message clearly explains the fallback behavior when no namespaces are provided.
Description
In Sozo, there was a specific return case for Katana, where the continuation token of
get_eventsis never null (for Katana).However, testing that the page is empty, can happen on a public network like starknet mainnet, when filters are applied and the targeted contract doesn't receive any event for a long period of time.
The current fix is still using a Katana special case, @kariy I don't have in mind if the continuation token was fixed. I'll prepare a MRE to ensure we can have the same behavior as the spec, which is returning null when no more pages are expected.
This PR also adds two things:
Summary by CodeRabbit
New Features
Improvements
Internal Changes