feat(torii): read only torii #2795
Conversation
WalkthroughOhayo! This pull request introduces significant changes 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
|
![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
🧹 Outside diff range and nitpick comments (2)
crates/torii/cli/src/options.rs (1)
93-95: Ohayo! Consider enhancing the help text for better clarity, sensei.The help text could be more descriptive about what happens when indexing is disabled. Consider updating it to explain that this makes torii operate in read-only mode.
- #[arg(long, default_value_t = true, help = "If indexing should be enabled.")] + #[arg(long, default_value_t = true, help = "If indexing should be enabled. When disabled, torii operates in read-only mode without processing new events or blocks.")]bin/torii/src/main.rs (1)
253-258: Clean implementation of the engine handle spawn!The error handling is appropriate, but consider adding a debug log when indexing is disabled for better observability.
let engine_handle = tokio::spawn(async move { if let Some(engine) = &mut engine { return engine.start().await; } + tracing::debug!(target: LOG_TARGET, "Engine not started as indexing is disabled"); Ok(()) });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/torii/src/main.rs(2 hunks)crates/torii/cli/src/options.rs(3 hunks)
🔇 Additional comments (3)
crates/torii/cli/src/options.rs (2)
169-169: LGTM! Default implementation is correct.
The default value of true maintains backward compatibility.
185-187: Verify the merge logic intention for the enabled field.
The merge logic for enabled differs from other fields. While other fields check for default values, enabled only updates if it's currently true. This suggests that once disabled, it cannot be re-enabled through merging. Is this the intended behavior?
✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the context better and verify the merge behavior intention.
Let's look for the complete merge implementation to understand the pattern for other fields.
The merge logic for enabled field is intentionally designed to be one-way disabling
Ohayo sensei! After analyzing the codebase, I can confirm that the merge behavior for enabled is intentionally different from other fields. The IndexingOptions struct shows that:
enableddefaults totruein theDefaultimplementation- The merge logic only updates when
self.enabledis true, effectively making it a one-way switch that can only be disabled through merging
This is a valid pattern when you want to ensure that once indexing is disabled (e.g., through a config file or command line), it cannot be re-enabled through subsequent merge operations. This provides a reliable way to disable indexing that persists across configuration merges.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this behavior is documented or discussed elsewhere
rg -A 5 "indexing.*enabled" --type md
Length of output: 39
Script:
#!/bin/bash
# Let's check the struct definition and other usages of enabled field
ast-grep --pattern 'struct $_ {
$$$
enabled: $_,
$$$
}'
# Also check for any comments or documentation around the enabled field
rg -B 2 -A 2 "enabled: bool"
# Look for merge implementation tests
rg -A 10 "fn test.*merge"
Length of output: 844
Script:
#!/bin/bash
# Let's check the implementation of merge for this struct
ast-grep --pattern 'impl $_ {
$$$
fn merge($$$) {
$$$
}
$$$
}'
# Let's also check for any default implementation or trait implementations
rg -B 5 -A 15 "impl.*for.*Options"
Length of output: 10427
bin/torii/src/main.rs (1)
149-174: Ohayo! Clean implementation of conditional Engine initialization, sensei!
The separation of world initialization from Engine and the use of Option for conditional Engine creation is well-structured. This allows other services to use the world reader while respecting the indexing configuration.
| event_processor_config: EventProcessorConfig { | ||
| historical_events: args.events.historical.into_iter().collect(), | ||
| namespaces: args.indexing.namespaces.into_iter().collect(), | ||
| // Get world address |
There was a problem hiding this comment.
| // Get world address |
| /// If indexing should be enabled | ||
| #[arg(long, default_value_t = true, help = "If indexing should be enabled.")] | ||
| pub enabled: bool, |
There was a problem hiding this comment.
Why would we want this to be disabled? For replicates syncing from a single indexing Torii?
| /// If indexing should be enabled | ||
| #[arg(long, default_value_t = true, help = "If indexing should be enabled.")] | ||
| pub enabled: bool, |
There was a problem hiding this comment.
| /// If indexing should be enabled | |
| #[arg(long, default_value_t = true, help = "If indexing should be enabled.")] | |
| pub enabled: bool, | |
| /// If indexing should be enabled | |
| #[arg(long, default_value_t = true, help = "If indexing should be enabled.")] | |
| #[serde(default)] | |
| pub enabled: bool, |
Add serde default to ensure it's not required to be given in configuration files.
Summary by CodeRabbit
New Features
Bug Fixes