feat(torii-indexer): add option for strict model reader block #2954
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new configuration option Changes
Sequence DiagramsequenceDiagram
participant Processor
participant World
participant Config
Processor->>Config: Check strict_model_reader
alt strict_model_reader is true
Processor->>World: model_reader_with_block(block_number)
else
Processor->>World: model_reader()
end
Possibly related PRs
Suggested reviewers
Ohayo and happy coding, sensei! 🍵🥷 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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 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 comments (1)
crates/torii/cli/src/options.rs (1)
Line range hint
201-245: Update merge method to handle strict_model_readerSensei, the
mergemethod should be updated to handle the newstrict_model_readerfield to maintain consistency with other options.Add this case to the merge method:
pub fn merge(&mut self, other: Option<&Self>) { if let Some(other) = other { + if !self.strict_model_reader { + self.strict_model_reader = other.strict_model_reader; + } + if self.events_chunk_size == DEFAULT_EVENTS_CHUNK_SIZE { self.events_chunk_size = other.events_chunk_size; }
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/mod.rs (1)
33-33: Add documentation for the strict_model_reader configuration.Ohayo! The new configuration field would benefit from documentation explaining its purpose and impact on model reading behavior.
+ /// When true, models are read from their registration block. + /// When false, models are read from the latest block. pub strict_model_reader: bool,crates/torii/indexer/src/processors/register_event.rs (1)
81-81: Update the outdated comment above model reading.The comment "Called model here by language..." should be updated to reflect the current implementation.
- // Called model here by language, but it's an event. Torii rework will make clear - // distinction. + // Retrieve the event model using block-specific reading if configured
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/torii/cli/src/options.rs(2 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(3 hunks)crates/torii/indexer/src/processors/register_model.rs(3 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(3 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (7)
crates/torii/indexer/src/processors/register_model.rs (1)
80-84: LGTM! Clean implementation of block-specific model reading.The conditional model reading implementation looks good, sensei! It properly handles both strict and non-strict modes.
crates/torii/indexer/src/processors/upgrade_model.rs (1)
89-93: Verify schema diff behavior with strict model reading.The schema diffing logic might behave differently when using strict model reading. Please verify that comparing prev_schema (from DB) with new_schema (from specific block) produces expected results in all scenarios.
✅ Verification successful
Ohayo sensei!
Schema diff implementation is safe with strict model reading
The schema comparison is implemented safely, comparing the DB schema with the new schema regardless of how the new schema was obtained. The
diff()method handles both cases equivalently, and proper null checks are in place.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering schema diff scenarios rg -A 5 "schema_diff.*test"Length of output: 29
Script:
#!/bin/bash # Find schema-related files fd schema # Look for schema comparison implementation ast-grep --pattern 'fn $_(schema1: $_Schema, schema2: $_Schema)' # Search for any test modules containing schema rg "mod.*test.*schema" -A 5Length of output: 1400
Script:
#!/bin/bash # Find schema_diff definition and usage rg "schema_diff" -A 5 crates/torii/indexer/src/processors/upgrade_model.rs # Look for schema comparison implementation rg "fn.*compare.*schema" -A 5 # Check schema struct definition rg "struct.*Schema" -A 10Length of output: 2080
crates/torii/indexer/src/processors/upgrade_event.rs (3)
8-8: LGTM! Import addition for BlockIdThe addition of
BlockIdimport is necessary for the new block-specific model reading functionality.
50-54: LGTM! Parameter renaming reflects actual usageOhayo! The parameters are now properly named without underscores since they're being used in the implementation.
91-95: LGTM! Well-implemented conditional model readingThe implementation correctly uses block-specific model reading when
strict_model_readeris true, falling back to latest state otherwise. This aligns perfectly with the PR objectives, sensei! 🎯crates/torii/cli/src/options.rs (2)
93-101: LGTM! Well-documented configuration optionOhayo! The new
strict_model_readeroption is well-integrated with clear documentation and proper attribute tags.
188-188: LGTM! Consistent default valueThe default implementation correctly initializes
strict_model_readerto false, matching the CLI default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/runner/src/lib.rs (1)
180-183: Ohayo sensei! The integration of strict_model_reader looks good! 🎋The new configuration option is properly integrated within the
EventProcessorConfigalongside other related settings likehistorical_eventsandnamespaces.However, since this is a new configuration option that affects model reading behavior, it would be good to add a code comment explaining its purpose and implications.
event_processor_config: EventProcessorConfig { + // Controls whether models should be read from their registration block (true) + // or from the latest block (false) strict_model_reader: self.args.indexing.strict_model_reader, historical_events: self.args.events.historical.into_iter().collect(), namespaces: self.args.indexing.namespaces.into_iter().collect(), },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/runner/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: build
🔇 Additional comments (1)
crates/torii/runner/src/lib.rs (1)
180-180: Verify consistent implementation across componentsLet's ensure this option is properly handled in all relevant processors.
✅ Verification successful
Ohayo sensei! Implementation verified across all components ✅
The
strict_model_readeroption is properly implemented in all four processors (register/upgrade model/event), following a consistent pattern with proper configuration flow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of strict_model_reader across the codebase echo "Checking for strict_model_reader implementations:" rg "strict_model_reader" --type rust echo -e "\nChecking for model reader calls that might need to consider the strict option:" rg "model_reader" --type rustLength of output: 5160
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2954 +/- ##
=======================================
Coverage 57.11% 57.11%
=======================================
Files 424 424
Lines 56200 56215 +15
=======================================
+ Hits 32097 32110 +13
- Misses 24103 24105 +2 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
strict_model_readeroption to control model reading behavior.Improvements