refactor(torii): model reader without call to world #3052
Conversation
|
Ohayo, sensei! Here’s the detailed overview of the pull request: WalkthroughThis pull request refactors the initialization process for the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
🪧 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/dojo/world/src/contracts/model.rs(2 hunks)crates/dojo/world/src/contracts/world.rs(1 hunks)crates/torii/indexer/src/processors/register_model.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/world/src/contracts/world.rs
[error] 38-39: missing parameters for function definition. Help: add a parameter list.
[error] 39-39: expected one of ->, <, where, or {, found }.
crates/torii/indexer/src/processors/register_model.rs
[error] 80-80: expected ;, found world. Help: add ; here.
🔇 Additional comments (5)
crates/dojo/world/src/contracts/world.rs (2)
27-27: Ohayo! LGTM on the model_reader_with_tag refactor!The change to use
new_from_worldaligns well with the centralized instance creation pattern.
35-35: Ohayo! LGTM on the model_reader refactor!The change to use
new_from_worldmaintains consistency with the new pattern.crates/dojo/world/src/contracts/model.rs (3)
85-104: Ohayo! The new ModelRPCReader::new implementation looks great!Clean implementation with proper parameter handling and error management.
106-112: Ohayo! The new_from_world convenience method is well designed!Good use of delegation to maintain DRY principles.
114-138: Ohayo! The new_from_world_with_block implementation is solid!Excellent error handling and validation, with clean delegation to the new centralized constructor.
| pub async fn model_reader_with | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! There's a syntax error in the incomplete function definition.
The function model_reader_with is incomplete and will cause compilation errors. This appears to be an unintended fragment that should be removed.
Apply this diff to fix the issue:
- pub async fn model_reader_with
-}
+}📝 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 model_reader_with | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 38-39: missing parameters for function definition. Help: add a parameter list.
[error] 39-39: expected one of ->, <, where, or {, found }.
| @@ -77,7 +77,7 @@ where | |||
| return Ok(()); | |||
| } | |||
|
|
|||
| let model = if config.strict_model_reader { | |||
| let model = if config.strict_model_reader {s | |||
There was a problem hiding this comment.
Ohayo sensei! Found a typo in the conditional statement.
There's an extra 's' character after the opening brace that's causing a syntax error.
Apply this diff to fix the issue:
- let model = if config.strict_model_reader {s
+ let model = if config.strict_model_reader {📝 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.
| let model = if config.strict_model_reader {s | |
| let model = if config.strict_model_reader { |
🧰 Tools
🪛 GitHub Actions: ci
[error] 80-80: expected ;, found world. Help: add ; here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/indexer/src/processors/register_model.rs (1)
80-83: Ohayo sensei! Please address the pipeline's formatting concern.The pipeline flagged a readability warning for this line. Splitting it will improve maintainability and adhere to the style guidelines.
- let mut model = ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world).await?; + let mut model = ModelRPCReader::new( + &namespace, + &name, + event.address.0, + event.class_hash.0, + world + ).await?;crates/torii/indexer/src/processors/upgrade_event.rs (1)
91-94: Ohayo sensei! Consider splitting the instantiation for readability.Similar to the pipeline’s recommendation, a multiline approach for
ModelRPCReader::newcan improve legibility.- let mut model = ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world).await?; + let mut model = ModelRPCReader::new( + &namespace, + &name, + event.address.0, + event.class_hash.0, + world + ).await?;crates/torii/indexer/src/processors/register_event.rs (1)
82-85: Ohayo sensei! Split the new call for better readability.Addressing the pipeline’s formatting warning will keep things consistent and easier on the eyes.
- let mut model = ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world).await?; + let mut model = ModelRPCReader::new( + &namespace, + &name, + event.address.0, + event.class_hash.0, + world + ).await?;crates/torii/indexer/src/processors/upgrade_model.rs (1)
86-86: Let's improve the code formatting, sensei!The pipeline detected a formatting issue. Consider splitting the long line for better readability.
- let mut model = ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world).await?; + let mut model = ModelRPCReader::new( + &namespace, + &name, + event.address.0, + event.class_hash.0, + world, + ) + .await?;🧰 Tools
🪛 GitHub Actions: ci
[warning] 86-86: Formatting issue detected: Line split for 'let mut model = ModelRPCReader::new(...)' for better readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/dojo/world/src/contracts/model.rs(3 hunks)crates/dojo/world/src/contracts/world.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(2 hunks)crates/torii/indexer/src/processors/register_model.rs(2 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(2 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/world/src/contracts/world.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/register_model.rs
[warning] 77-77: Formatting issue detected: Line split for 'let mut model = ModelRPCReader::new(...)' for better readability.
crates/torii/indexer/src/processors/register_event.rs
[warning] 79-79: Formatting issue detected: Line split for 'let mut model = ModelRPCReader::new(...)' for better readability.
crates/torii/indexer/src/processors/upgrade_event.rs
[warning] 88-88: Formatting issue detected: Line split for 'let mut model = ModelRPCReader::new(...)' for better readability.
crates/torii/indexer/src/processors/upgrade_model.rs
[warning] 86-86: Formatting issue detected: Line split for 'let mut model = ModelRPCReader::new(...)' for better readability.
🔇 Additional comments (8)
crates/torii/indexer/src/processors/register_model.rs (1)
6-6: Ohayo sensei! Import looks good.No issues here. The addition of
ModelRPCReaderseems consistent with the overall refactoring approach.crates/torii/indexer/src/processors/upgrade_event.rs (1)
6-6: Ohayo sensei! Great import usage.Bringing in
ModelRPCReaderaligns with the new approach in other files. No issues here.crates/torii/indexer/src/processors/register_event.rs (1)
6-6: Ohayo sensei! The new import is consistent with the refactor.No concerns here; it matches the revised model-reading strategy.
crates/torii/indexer/src/processors/upgrade_model.rs (2)
6-6: Ohayo! Nice import organization, sensei!The addition of
ModelRPCReaderalongsideModelReadershows good foresight in maintaining both interfaces during the refactor.
89-92: Clean refactor using ModelRPCReader::new, sensei!The direct instantiation of
ModelRPCReaderwith event address and class hash simplifies the code and removes the dependency on world for these values.crates/dojo/world/src/contracts/model.rs (3)
85-104: Ohayo! Nice constructor implementation, sensei!The new
newmethod provides a clean way to directly instantiateModelRPCReaderwith address and class hash, reducing coupling with the world contract.
106-129: Clean refactor of world-based initialization, sensei!The renamed
new_from_worldmethod now:
- Uses a more descriptive name
- Reuses the new constructor
- Properly handles the zero address case
158-160: Nice extraction of block setting functionality, sensei!The new
set_blockmethod provides a clean way to update the block ID after initialization, improving the API's flexibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/register_event.rs (2)
82-84: Ohayo sensei! Consider improving error handling for ModelRPCReader initialization.The
.await?propagates errors directly. Consider adding context to these errors to make debugging easier.- let mut model = - ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world) - .await?; + let mut model = ModelRPCReader::new(&namespace, &name, event.address.0, event.class_hash.0, world) + .await + .map_err(|e| anyhow::anyhow!("Failed to initialize ModelRPCReader: {}", e))?;
85-87: Add error handling for block setting.The
set_blockcall should handle potential errors and provide feedback.if config.strict_model_reader { - model.set_block(BlockId::Number(block_number)).await; + model.set_block(BlockId::Number(block_number)) + .await + .map_err(|e| anyhow::anyhow!("Failed to set block for model reader: {}", e))?; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/dojo/world/src/contracts/world.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(2 hunks)crates/torii/indexer/src/processors/register_model.rs(2 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(2 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/indexer/src/processors/register_model.rs
- crates/dojo/world/src/contracts/world.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (3)
crates/torii/indexer/src/processors/upgrade_model.rs (2)
89-91: Similar improvements needed for ModelRPCReader initialization.The initialization pattern here matches register_event.rs and would benefit from the same error handling improvements.
92-94: Similar improvements needed for block setting.The block setting pattern here matches register_event.rs and would benefit from the same error handling improvements.
crates/torii/indexer/src/processors/upgrade_event.rs (1)
91-96: Ohayo! Consider addressing the model vs event distinction.The comment on line 70-71 mentions "Called model here by language, but it's an event. Torii rework will make clear distinction." This seems like a good opportunity to start making this distinction clearer.
Would you like me to help create an issue to track the model/event distinction refactoring? This could help improve code clarity and reduce confusion.
Also, the ModelRPCReader initialization and block setting patterns here match the other processors and would benefit from the same error handling improvements.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3052 +/- ##
==========================================
+ Coverage 56.88% 57.56% +0.68%
==========================================
Files 439 439
Lines 59762 59445 -317
==========================================
+ Hits 33995 34222 +227
+ Misses 25767 25223 -544 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
ModelRPCReaderfor model data access across various processors, enhancing the overall workflow.