feat(torii): indexing cartridge controllers #2959
Conversation
WalkthroughOhayo, sensei! This pull request introduces support for indexing Cartridge controllers in the Torii system. The changes span multiple crates, adding a new boolean flag to control Cartridge indexing, creating a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/Runner
participant Indexer as Indexer
participant Processor as ControllerProcessor
participant Database as SQLite Database
CLI->>Indexer: Start indexing with cartridge option
Indexer->>Processor: Process contract deployment event
Processor->>Processor: Validate event
Processor->>Processor: Extract controller details
Processor->>Database: Add/Update controller record
Possibly related PRs
Suggested reviewers
Sensei, the changes look comprehensive and well-structured! The new Cartridge controller indexing feature seems to be implemented with careful attention to modularity and extensibility. Ohayo and happy coding! 🍵🖥️ 🪧 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 (4)
crates/torii/runner/src/constants.rs (1)
3-5: Ohayo sensei! Consider splitting long lines for better readability.The Cartridge paymaster address declarations exceed the line length limit. Let's improve readability:
-pub(crate) const CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS: Felt = felt!("0x359a81f67140632ec91c7f9af3fc0b5bca0a898ae0be3f7682585b0f40119a7"); -pub(crate) const CARTRIDGE_PAYMASTER_SEA1_ADDRESS: Felt = felt!("0x07a0f23c43a291282d093e85f7fb7c0e23a66d02c10fead324ce4c3d56c4bd67"); -pub(crate) const CARTRIDGE_PAYMASTER_USEAST4_ADDRESS: Felt = felt!("0x2d2e564dd4faa14277fefd0d8cb95e83b13c0353170eb6819ec35bf1bee8e2a"); +pub(crate) const CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS: Felt = felt!( + "0x359a81f67140632ec91c7f9af3fc0b5bca0a898ae0be3f7682585b0f40119a7" +); +pub(crate) const CARTRIDGE_PAYMASTER_SEA1_ADDRESS: Felt = felt!( + "0x07a0f23c43a291282d093e85f7fb7c0e23a66d02c10fead324ce4c3d56c4bd67" +); +pub(crate) const CARTRIDGE_PAYMASTER_USEAST4_ADDRESS: Felt = felt!( + "0x2d2e564dd4faa14277fefd0d8cb95e83b13c0353170eb6819ec35bf1bee8e2a" +);🧰 Tools
🪛 GitHub Actions: ci
[warning] 3-5: Formatting issue: Long lines need to be split for better readability
crates/torii/sqlite/src/types.rs (1)
159-159: Add trailing comma for better git diffs.Adding a trailing comma after the CARTRIDGE variant will make future additions cleaner:
- CARTRIDGE + CARTRIDGE,crates/torii/runner/src/lib.rs (2)
47-47: Split import statement for better readability.The import statement is too long. Let's improve readability:
-use crate::constants::{LOG_TARGET, CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS, CARTRIDGE_PAYMASTER_SEA1_ADDRESS, CARTRIDGE_PAYMASTER_USEAST4_ADDRESS}; +use crate::constants::{ + CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS, CARTRIDGE_PAYMASTER_SEA1_ADDRESS, + CARTRIDGE_PAYMASTER_USEAST4_ADDRESS, LOG_TARGET, +};
72-76: Format contract push statements for better readability.The contract push statements could be formatted better:
- if self.args.indexing.cartridge { - self.args.indexing.contracts.push(Contract { address: CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS, r#type: ContractType::CARTRIDGE }); - self.args.indexing.contracts.push(Contract { address: CARTRIDGE_PAYMASTER_SEA1_ADDRESS, r#type: ContractType::CARTRIDGE }); - self.args.indexing.contracts.push(Contract { address: CARTRIDGE_PAYMASTER_USEAST4_ADDRESS, r#type: ContractType::CARTRIDGE }); - } + if self.args.indexing.cartridge { + self.args.indexing.contracts.extend([ + Contract { + address: CARTRIDGE_PAYMASTER_EUWEST3_ADDRESS, + r#type: ContractType::CARTRIDGE, + }, + Contract { + address: CARTRIDGE_PAYMASTER_SEA1_ADDRESS, + r#type: ContractType::CARTRIDGE, + }, + Contract { + address: CARTRIDGE_PAYMASTER_USEAST4_ADDRESS, + r#type: ContractType::CARTRIDGE, + }, + ]); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/torii/cli/src/options.rs(3 hunks)crates/torii/indexer/src/constants.rs(1 hunks)crates/torii/indexer/src/engine.rs(2 hunks)crates/torii/runner/src/constants.rs(1 hunks)crates/torii/runner/src/lib.rs(3 hunks)crates/torii/sqlite/src/executor/erc.rs(1 hunks)crates/torii/sqlite/src/types.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/indexer/src/constants.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/types.rs
[warning] 156-156: Formatting issue: Missing trailing comma after CARTRIDGE in enum
crates/torii/runner/src/lib.rs
[warning] 44-44: Formatting issue: Import statement needs to be split into multiple lines
[warning] 70-72: Formatting issue: Contract push statements need to be split into multiple lines for better readability
crates/torii/runner/src/constants.rs
[warning] 3-5: Formatting issue: Long lines need to be split for better readability
🔇 Additional comments (4)
crates/torii/cli/src/options.rs (2)
173-181: LGTM! Clean implementation of cartridge indexing option.The cartridge field is well-documented and follows the existing patterns in the codebase.
239-242: LGTM! Proper merge behavior implementation.The merge behavior for the cartridge field is consistent with other fields.
crates/torii/sqlite/src/executor/erc.rs (1)
59-59: Ohayo sensei! Consider handling the CARTRIDGE case properly.The
unreachable!()macro will cause a runtime panic if this code path is ever reached. Consider either:
- Implementing proper balance diff handling for CARTRIDGE type, or
- Adding a comment explaining why this case is unreachable
Let's verify if there are any CARTRIDGE events that could trigger this code path:
✅ Verification successful
Ohayo sensei! The unreachable!() is correct, but needs documentation.
The CARTRIDGE type represents paymaster contracts that don't track balances like ERC tokens do. While the
unreachable!()is appropriate here, please add a comment explaining why this case can never occur, similar to:ContractType::CARTRIDGE => unreachable!(), // CARTRIDGE contracts don't track token balances🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any CARTRIDGE-related event handling or balance operations rg -l "ContractType::CARTRIDGE.*balance"Length of output: 42
Script:
#!/bin/bash # Search for all occurrences of CARTRIDGE rg "CARTRIDGE" -A 3 -B 3 # Find the ContractType enum definition ast-grep --pattern 'enum ContractType { $$$ }' # Search for balance-related operations rg "balance" -A 3 -B 3 crates/torii/sqlite/src/executor/erc.rsLength of output: 9949
Script:
#!/bin/bash # Search for any balance-related operations or events for CARTRIDGE contracts rg -B 5 -A 5 "paymaster.*balance|balance.*paymaster" # Look for any CARTRIDGE-specific implementations or handlers ast-grep --pattern 'impl $_ for $_ { $$$ CARTRIDGE => $_ $$$ }'Length of output: 126
crates/torii/indexer/src/engine.rs (1)
42-42: Ohayo! LGTM - Transaction processor integration looks good.The addition of
StoreTransactionProcessorto the default set of processors aligns well with the indexing enhancement objectives.Let's verify the processor's implementation:
Also applies to: 66-66
✅ Verification successful
Ohayo! The StoreTransactionProcessor implementation is verified and looks solid, sensei!
The processor has a clean, focused implementation that:
- Generates unique transaction IDs using block number and hash
- Properly stores transactions with timestamps
- Follows error handling best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation of StoreTransactionProcessor ast-grep --pattern 'struct StoreTransactionProcessor { $$$ }'Length of output: 65
Script:
#!/bin/bash # First locate the processor file fd store_transaction.rs # Then examine its contents rg -A 10 "struct StoreTransactionProcessor" --type rustLength of output: 1091
Script:
#!/bin/bash # Examine the full implementation with more context rg -A 30 "struct StoreTransactionProcessor" --type rust # Also check for any associated tests rg -A 10 "#\[test\]" crates/torii/indexer/src/processors/store_transaction.rsLength of output: 1740
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/torii/indexer/src/processors/controller.rs (1)
30-33: Consider additional checks on event structure.
Currently,validatechecks thatevent.keys.is_empty()andevent.datais non-empty, which is correct for theContractDeployedevent. However, for added defensive coding, consider verifying the data's length to ensure both address and username are present inevent.data.crates/torii/indexer/src/processors/mod.rs (1)
28-28: Reorder module declarations to resolve pipeline warning.
The CI complains about the ordering. Keep modules sorted or follow the project’s style for consistency. For instance, placepub mod controller;near related modules.- pub mod controller; ... + pub mod controller;crates/torii/sqlite/src/lib.rs (1)
845-845: Fix formatting lint.
The pipeline warns that method calls need to be on a single line. Apply this diff to comply:- Argument::String(utc_dt_string_from_timestamp(block_timestamp)), + Argument::String( + utc_dt_string_from_timestamp(block_timestamp) + ),🧰 Tools
🪛 GitHub Actions: ci
[warning] 845-845: Formatting issue: Method call needs to be on a single line
crates/torii/migrations/20250128051146_controllers.sql (1)
2-7: Ohayo sensei! Consider optimizing the table structure.The current schema has redundant columns - both
idandusernamestore the same information. Since the username is used as the primary key, we can simplify the schema.Consider this optimization:
CREATE TABLE controllers ( - id TEXT PRIMARY KEY NOT NULL, -- Username as primary key - username TEXT NOT NULL, -- Username + username TEXT PRIMARY KEY NOT NULL, -- Username as primary key address TEXT NOT NULL, -- Wallet address deployed_at TIMESTAMP NOT NULL -- Block timestamp of deployment );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/indexer/src/processors/controller.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/migrations/20250128051146_controllers.sql(1 hunks)crates/torii/runner/src/constants.rs(1 hunks)crates/torii/sqlite/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/runner/src/constants.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/mod.rs
[warning] 9-25: Module ordering issue: 'controller' module declaration moved from bottom to top of module list
crates/torii/sqlite/src/lib.rs
[warning] 845-845: Formatting issue: Method call needs to be on a single line
🔇 Additional comments (3)
crates/torii/indexer/src/processors/controller.rs (2)
1-2: Ohayo sensei! Good start with the crate imports.
No immediate concerns here. Just keep an eye on potential usage and references to ensure nothing is unused.
45-79: Prevent potential out-of-bounds indexing.
Insideprocess, lines 56 (event.data[event.data.len() - 1]) and 59 (event.data[0]) assumeevent.datahas at least 2 items. Even thoughvalidateensuresevent.dataisn't empty, you might want to further guard for scenarios whereevent.datamight have only one felt.crates/torii/sqlite/src/lib.rs (1)
825-854: Verify that 'id' is effectively the username.
Currently, the SQL query uses the sameusernameargument for bothidandusernamein thecontrollerstable. If this is intentional—using the username as a primary identifier—ensure you won’t need to store a separate primary key later. Otherwise, you might want to use theaddressor a composite key.✅ Verification successful
Ohayo sensei! Username is indeed intentionally used as the primary key (
id)The migration file at
crates/torii/migrations/20250128051146_controllers.sqlexplicitly documents this design choice with the comment "Username as primary key". The schema and implementation are consistent with this intention.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check references to `controllers` table creation and usage to confirm `id` usage. rg -A 5 "CREATE TABLE" | rg "controllers"Length of output: 631
🧰 Tools
🪛 GitHub Actions: ci
[warning] 845-845: Formatting issue: Method call needs to be on a single line
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/indexer/src/processors/controller.rs (1)
29-32: Consider enhancing event validation.While the current validation checks for empty keys and non-empty data, consider adding a length check for
event.datato ensure it contains at least two elements (address and username).fn validate(&self, event: &Event) -> bool { // ContractDeployed event has no keys and contains username in data - event.keys.is_empty() && !event.data.is_empty() + event.keys.is_empty() && event.data.len() >= 2 }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/engine.rs(4 hunks)crates/torii/indexer/src/processors/controller.rs(1 hunks)
🔇 Additional comments (3)
crates/torii/indexer/src/processors/controller.rs (1)
17-19: LGTM! Clean and focused struct implementation.The
ControllerProcessorstruct is well-designed for handling controller deployment events.crates/torii/indexer/src/engine.rs (2)
110-113: LGTM! Clean integration of CARTRIDGE processor.The CARTRIDGE contract type is properly initialized with the ControllerProcessor.
110-113: Verify CARTRIDGE event processing configuration.Let's ensure that CARTRIDGE events are properly handled in the event processing pipeline.
✅ Verification successful
Ohayo sensei! CARTRIDGE event processing configuration is properly implemented
The configuration is correct with ControllerProcessor handling CARTRIDGE events, proper type system integration, and clean separation from ERC processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CARTRIDGE event processing configuration # Check for any existing CARTRIDGE event processors rg -l "ContractType::CARTRIDGE" . # Check for any potential conflicts in event processing ast-grep --pattern 'ContractType::CARTRIDGE'Length of output: 1036
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/controller.rs (2)
25-32: Enhance validation robustness, sensei!The validation could be more thorough by checking the exact data length and structure.
Consider this enhancement:
fn validate(&self, event: &Event) -> bool { - // ContractDeployed event has no keys and contains username in data - event.keys.is_empty() && !event.data.is_empty() + // ContractDeployed event has: + // - no keys + // - data[0]: contract address + // - data[last]: username as short string + event.keys.is_empty() && event.data.len() >= 2 }
34-43: Document priority levels and identifier uniqueness, sensei!The implementation is correct, but could benefit from documentation explaining:
- The significance of priority level 3
- How the task identifier ensures uniqueness across events
Consider adding these doc comments:
+ /// Priority 3: Medium priority for controller deployments fn task_priority(&self) -> TaskPriority { 3 } + /// Generates a unique identifier using the contract address (first data element) + /// This ensures unique tasks per controller deployment fn task_identifier(&self, event: &Event) -> TaskId {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/processors/controller.rs(1 hunks)
🔇 Additional comments (3)
crates/torii/indexer/src/processors/controller.rs (3)
1-15: Ohayo! The imports and constants look good, sensei!The imports are well-organized and the LOG_TARGET constant follows the standard logging pattern.
17-18: Clean and minimal struct definition, sensei!The unit-like struct with appropriate derives is perfect for this processor implementation.
56-57: Add error handling for username parsing, sensei!The username parsing could fail if the salt is not a valid short string.
| let username_felt = event.data[event.data.len() - 1]; | ||
| let username = parse_cairo_short_string(&username_felt)?; | ||
| // Address is the first felt in data | ||
| let address = event.data[0]; |
There was a problem hiding this comment.
Add bounds checking for data access, sensei!
The code assumes data elements exist without validation, which could panic if the event data is malformed.
Consider this safer implementation:
+ if event.data.len() < 2 {
+ return Err(anyhow::anyhow!("Invalid event data length for ContractDeployed event"));
+ }
let username_felt = event.data[event.data.len() - 1];
let username = parse_cairo_short_string(&username_felt)?;
let address = event.data[0];📝 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 username_felt = event.data[event.data.len() - 1]; | |
| let username = parse_cairo_short_string(&username_felt)?; | |
| // Address is the first felt in data | |
| let address = event.data[0]; | |
| if event.data.len() < 2 { | |
| return Err(anyhow::anyhow!("Invalid event data length for ContractDeployed event")); | |
| } | |
| let username_felt = event.data[event.data.len() - 1]; | |
| let username = parse_cairo_short_string(&username_felt)?; | |
| // Address is the first felt in data | |
| let address = event.data[0]; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/runner/src/lib.rs (1)
74-87: Consider making paymaster addresses configurable, sensei!While the implementation is correct, hardcoding region-specific paymaster addresses might limit flexibility. Consider moving these addresses to a configuration file or environment variables for easier maintenance and deployment across different environments.
Example configuration structure:
pub struct CartridgeConfig { pub paymaster_addresses: HashMap<String, FieldElement>, // region -> address mapping }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/runner/src/constants.rs(1 hunks)crates/torii/runner/src/lib.rs(3 hunks)crates/torii/sqlite/src/lib.rs(1 hunks)crates/torii/sqlite/src/types.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/sqlite/src/lib.rs
- crates/torii/sqlite/src/types.rs
- crates/torii/runner/src/constants.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (2)
crates/torii/runner/src/lib.rs (2)
45-50: Ohayo sensei! Nice modularization of constants!Moving constants to a dedicated module improves code organization and maintainability.
169-169: Clean processor initialization, sensei!Using
Processors::default()is a cleaner approach that centralizes processor management.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2959 +/- ##
==========================================
- Coverage 57.10% 57.05% -0.06%
==========================================
Files 424 425 +1
Lines 56215 56301 +86
==========================================
+ Hits 32104 32123 +19
- Misses 24111 24178 +67 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/indexer/src/processors/controller.rs (1)
64-66:⚠️ Potential issueAdd bounds checking for data access, sensei!
The code assumes data elements exist without validation.
+ if event.data.len() < 2 { + return Err(anyhow::anyhow!("Invalid event data length for ContractDeployed event")); + } let address = event.data[0]; let deployer = event.data[1];
🧹 Nitpick comments (3)
crates/torii/indexer/src/processors/controller.rs (1)
37-40: Enhance event validation robustness, sensei!The validation could be more thorough by checking the exact number of expected data fields.
- fn validate(&self, event: &Event) -> bool { - // ContractDeployed event has no keys and contains username in data - event.keys.len() == 1 && !event.data.is_empty() - } + fn validate(&self, event: &Event) -> bool { + // ContractDeployed event has one key and exactly 3 data fields (address, deployer, username) + event.keys.len() == 1 && event.data.len() == 3 + }crates/torii/runner/src/lib.rs (1)
72-77: Add documentation for the cartridge indexing feature, sensei!Consider adding a comment explaining what cartridge indexing does and when it should be enabled.
+ // Enable indexing of Universal Deployer Contract (UDC) events when cartridge support is needed + // This allows tracking controller deployments made through cartridge paymasters if self.args.indexing.cartridge { self.args .indexing .contracts .push(Contract { address: UDC_ADDRESS, r#type: ContractType::UDC }); }crates/torii/indexer/src/engine.rs (1)
67-67: Document the StoreTransactionProcessor's purpose, sensei!Consider adding a comment explaining what this processor does.
+ // Store transaction processor handles persisting transaction data to the database transaction: vec![Box::new(StoreTransactionProcessor)],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/torii/indexer/src/engine.rs(4 hunks)crates/torii/indexer/src/processors/controller.rs(1 hunks)crates/torii/runner/src/constants.rs(1 hunks)crates/torii/runner/src/lib.rs(4 hunks)crates/torii/sqlite/src/executor/erc.rs(1 hunks)crates/torii/sqlite/src/types.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/torii/sqlite/src/executor/erc.rs
- crates/torii/runner/src/constants.rs
- crates/torii/sqlite/src/types.rs
🔇 Additional comments (4)
crates/torii/indexer/src/processors/controller.rs (3)
16-23: Ohayo, sensei! The paymaster address constants look good!The constants are well-defined with clear naming for different regions.
25-32: Clean trait implementation, sensei!The struct definition and trait bounds are well-structured and follow Rust best practices.
77-79:⚠️ Potential issueAdd bounds checking for username extraction, sensei!
The code assumes the username salt exists at the end of the data array.
+ if event.data.len() < 3 { + return Err(anyhow::anyhow!("Missing username salt in ContractDeployed event")); + } let username_felt = event.data[event.data.len() - 1]; let username = parse_cairo_short_string(&username_felt)?;Likely invalid or redundant comment.
crates/torii/indexer/src/engine.rs (1)
110-113: Clean integration of the UDC processor, sensei!The addition of the UDC contract type and its processor follows the established pattern consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/controller/mod.rs (2)
20-26: Ohayo sensei! Consider externalizing paymaster addresses to config.
Hardcoding addresses in code is workable for a quick approach, but might be less flexible if you plan to add or change addresses frequently. Storing them in a more dynamic config or environment variables would simplify updates.
93-96: Ohayo sensei! Defensive error handling for short string parse.
Currently, a parsing failure returns an error. If partial data is acceptable, consider a fallback or an error-specific log message to debug short string parse issues quickly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/torii/indexer/Cargo.toml(1 hunks)crates/torii/indexer/src/processors/controller/mod.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/controller/mod.rs
[error] 4-13: Code formatting error: Incorrect import organization and placement of 'lazy_static', 'Event', and other imports
🔇 Additional comments (5)
crates/torii/indexer/src/processors/controller/mod.rs (4)
28-42: Ohayo sensei! Validate CSV entries before adding to map.
Here, you skip the first line and assume the rest are valid addresses. It's helpful to handle any CSV parsing errors or unexpected formatting to avoid silent failures or potential data complexities later.
56-59: Ohayo sensei! Re-check logic for event key length.
The comment says "ContractDeployed event has no keys," but you validate withevent.keys.len() == 1. Confirm that’s correct or update the comment for consistency.
82-91: Ohayo sensei! Confirm ignoring non-paymaster deployers is intended.
You’re returning early if the deployer isn’t recognized. If that’s the intended behavior, all good. Otherwise, log or handle unknown deployers in a different way to avoid silent skipping.
112-115: Ohayo sensei! Excellent insertion approach.
Your call todb.add_controlleris straightforward and awaited properly, ensuring concurrency safety. This looks good.crates/torii/indexer/Cargo.toml (1)
41-41: Ohayo sensei!lazy_staticaddition is aligned with usage.
Includinglazy_staticin the workspace dependencies makes sense for storing and caching these paymaster addresses. Ensure no extraneous overhead if usage remains limited.
| use anyhow::{Error, Result}; | ||
| use async_trait::async_trait; | ||
| use dojo_world::contracts::world::WorldContractReader; | ||
| use starknet::core::utils::parse_cairo_short_string; | ||
| use starknet::providers::Provider; | ||
| use starknet::{core::types::Event, macros::felt}; | ||
| use starknet_crypto::Felt; | ||
| use torii_sqlite::Sql; | ||
| use tracing::{debug, info}; | ||
| use lazy_static::lazy_static; |
There was a problem hiding this comment.
Ohayo sensei! Formatting needs adjustment for imports.
To appease the pipeline, organize your imports so they follow Rust guidelines or your project's style rules, ensuring external crates (lazy_static) typically appear before local modules.
You could apply the following minimal snippet or run cargo fmt:
-use anyhow::{Error, Result};
-use async_trait::async_trait;
-use dojo_world::contracts::world::WorldContractReader;
-use starknet::core::utils::parse_cairo_short_string;
-use starknet::providers::Provider;
-use starknet::{core::types::Event, macros::felt};
-use starknet_crypto::Felt;
-use torii_sqlite::Sql;
-use tracing::{debug, info};
-use lazy_static::lazy_static;
+use lazy_static::lazy_static;
+use anyhow::{Error, Result};
+use async_trait::async_trait;
+use dojo_world::contracts::world::WorldContractReader;
+use starknet::{core::types::Event, macros::felt};
+use starknet::core::utils::parse_cairo_short_string;
+use starknet::providers::Provider;
+use starknet_crypto::Felt;
+use torii_sqlite::Sql;
+use tracing::{debug, info};📝 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.
| use anyhow::{Error, Result}; | |
| use async_trait::async_trait; | |
| use dojo_world::contracts::world::WorldContractReader; | |
| use starknet::core::utils::parse_cairo_short_string; | |
| use starknet::providers::Provider; | |
| use starknet::{core::types::Event, macros::felt}; | |
| use starknet_crypto::Felt; | |
| use torii_sqlite::Sql; | |
| use tracing::{debug, info}; | |
| use lazy_static::lazy_static; | |
| use lazy_static::lazy_static; | |
| use anyhow::{Error, Result}; | |
| use async_trait::async_trait; | |
| use dojo_world::contracts::world::WorldContractReader; | |
| use starknet::{core::types::Event, macros::felt}; | |
| use starknet::core::utils::parse_cairo_short_string; | |
| use starknet::providers::Provider; | |
| use starknet_crypto::Felt; | |
| use torii_sqlite::Sql; | |
| use tracing::{debug, info}; |
🧰 Tools
🪛 GitHub Actions: ci
[error] 4-13: Code formatting error: Incorrect import organization and placement of 'lazy_static', 'Event', and other imports
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/indexer/src/processors/controller.rs (1)
85-87:⚠️ Potential issueOhayo sensei! Strengthen the data length check to prevent out-of-bounds.
Currently,event.data.len() >= 29is insufficient: you slice[3..25]after skipping the first 5 elements, which needs at least 30 elements total to avoid a potential panic. This matches the essence of a previous “bounds checking” concern.A safer approach:
-if event.data.len() < 29 { +if event.data.len() < 30 { return Ok(()); }
🧹 Nitpick comments (4)
crates/torii/indexer/src/processors/controller.rs (4)
4-9: Ohayo sensei! Please reorganize imports to address pipeline complaints.
The pipeline flagged import formatting issues. Consider grouping and ordering external crates (likeasync_trait,lazy_static, andstarknet) together for readability.Below is an example of reorganized imports:
use std::hash::{DefaultHasher, Hash, Hasher}; +use anyhow::{Error, Result}; +use async_trait::async_trait; +use dojo_world::contracts::world::WorldContractReader; +use lazy_static::lazy_static; +use starknet::{core::types::Event, macros::felt}; +use starknet::core::utils::parse_cairo_short_string; +use starknet::providers::Provider; +use starknet_crypto::Felt; +use torii_sqlite::Sql; +use tracing::{debug, info};🧰 Tools
🪛 GitHub Actions: ci
[warning] 4-9: Code formatting issue: Imports are not properly organized. The formatter suggests separating and reordering the starknet imports.
59-61: Ohayo sensei! Align the comment and code logic for event keys.
The comment says “ContractDeployed event has no keys,” yet the validation check requires exactly one key (event.keys.len() == 1). Update the comment or the check to maintain consistency.
90-91: Ohayo sensei! Remove extraneous whitespace.
Pipeline formatting checks flagged the extra blank line after definingaddress.89 // Address is the first felt in data 90 let address = event.data[0]; -91 +// (remove blank line) 92 let calldata = event.data[5..].to_vec();🧰 Tools
🪛 GitHub Actions: ci
[warning] 90-90: Code formatting issue: Extra whitespace found. The formatter suggests removing unnecessary blank line.
127-127: Ohayo sensei! Add tests fordb.add_controller(...).
This new method for storing controller data deserves coverage to ensure correct usage and reliability over time.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/processors/controller.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/controller.rs
[warning] 4-9: Code formatting issue: Imports are not properly organized. The formatter suggests separating and reordering the starknet imports.
[warning] 90-90: Code formatting issue: Extra whitespace found. The formatter suggests removing unnecessary blank line.
🔇 Additional comments (2)
crates/torii/indexer/src/processors/controller.rs (2)
1-2: Ohayo sensei! Congratulations on the new file.
These initial lines simply import hashing utilities from the standard library. Looks fine.
108-110: Ohayo sensei! Confirm no privacy concerns when logging usernames.
Exposing usernames in logs might be sensitive. If it’s okay to treat them as public data, then proceed. Otherwise, consider redacting or hashing them.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/controller.rs (2)
23-49: Ohayo sensei! Let's enhance the magic constant documentation!While the implementation is correct, consider adding more context and a constant for the length:
+/// Length of the Cartridge magic constant +pub(crate) const CARTRIDGE_MAGIC_LENGTH: u8 = 22; + lazy_static! { - // https://x.cartridge.gg/ + /// Magic constant used to identify Cartridge controllers. + /// This represents the encoded form of "https://x.cartridge.gg/" + /// which is used as a signature to validate controller deployments. pub(crate) static ref CARTRIDGE_MAGIC: [Felt; 22] = [
60-63: Ohayo sensei! Let's make the validation more robust!The current validation could be enhanced to check for the minimum required data length since we know we'll need at least 29 elements later in process().
fn validate(&self, event: &Event) -> bool { // ContractDeployed event has no keys and contains username in data - event.keys.len() == 1 && !event.data.is_empty() + event.keys.len() == 1 && event.data.len() >= 29 }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/engine.rs(4 hunks)crates/torii/indexer/src/processors/controller.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/indexer/src/engine.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/torii/indexer/src/processors/controller.rs (3)
1-19: Ohayo sensei! The imports and constants look good!All necessary dependencies are properly imported and the log target constant follows the standard naming convention.
90-92: Ohayo sensei! Let's add bounds checking for data access!Add validation before accessing array elements to prevent potential panics:
+if event.data.is_empty() { + return Ok(()); +} let address = event.data[0];
103-103: Avoid unwrap() to prevent runtime panics!Replace unwrap() with proper error handling:
-let cartridge_magic: [Felt; 22] = calldata[3..25].try_into().unwrap(); +let cartridge_magic: [Felt; 22] = calldata[3..25] + .try_into() + .map_err(|_| anyhow::anyhow!("Invalid Cartridge magic slice"))?;
| let calldata = event.data[5..].to_vec(); | ||
|
|
||
| // check for this sequence of felts | ||
| let cartridge_magic_len = calldata[2]; | ||
| // length has to be 22 | ||
| if cartridge_magic_len != Felt::from(22) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking and constants for magic numbers!
The code uses magic numbers for array indices and lacks proper bounds checking.
+const CALLDATA_START_INDEX: usize = 5;
+const MAGIC_LENGTH_INDEX: usize = 2;
-let calldata = event.data[5..].to_vec();
+let calldata = event.data.get(CALLDATA_START_INDEX..).ok_or_else(||
+ anyhow::anyhow!("Invalid calldata length")
+)?.to_vec();
-let cartridge_magic_len = calldata[2];
+let cartridge_magic_len = calldata.get(MAGIC_LENGTH_INDEX).ok_or_else(||
+ anyhow::anyhow!("Invalid magic length index")
+)?;📝 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 calldata = event.data[5..].to_vec(); | |
| // check for this sequence of felts | |
| let cartridge_magic_len = calldata[2]; | |
| // length has to be 22 | |
| if cartridge_magic_len != Felt::from(22) { | |
| return Ok(()); | |
| } | |
| const CALLDATA_START_INDEX: usize = 5; | |
| const MAGIC_LENGTH_INDEX: usize = 2; | |
| let calldata = event.data.get(CALLDATA_START_INDEX..).ok_or_else(|| | |
| anyhow::anyhow!("Invalid calldata length") | |
| )?.to_vec(); | |
| // check for this sequence of felts | |
| let cartridge_magic_len = calldata.get(MAGIC_LENGTH_INDEX).ok_or_else(|| | |
| anyhow::anyhow!("Invalid magic length index") | |
| )?; | |
| // length has to be 22 | |
| if cartridge_magic_len != Felt::from(22) { | |
| return Ok(()); | |
| } |
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/indexer/src/processors/controller.rs (1)
128-128: Ohayo sensei, consider logging skipped events or providing debug info.
Whenever you return early (e.g., mismatched magic length or length < 25), you might want to log a debug message to help track why certain events are not processed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/processors/controller.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (7)
crates/torii/indexer/src/processors/controller.rs (7)
1-14: Ohayo sensei, your imports appear well-chosen!
They cover all required crates and functionality, and there are no apparent redundancy or unused imports at a glance.
15-19: Ohayo sensei, good job on configuring the log target!
Defining a dedicated log target for this processor is handy for quick filtering, and centralized debugging.
23-49: Ohayo sensei, lazy_static usage looks fine!
Storing these magic bytes in a static reference is a neat approach to avoid re-allocating the array on each call, especially if you need consistent access across your application.
60-63: Ohayo sensei, please add stricter data length checks to align with future slicing.
While you do ensure!event.data.is_empty(), slicing at line 89 with[5..]can still panic if there are fewer than 6 items. This is similar to a prior suggestion.
89-93: Ohayo sensei, ensure there are at least 5 elements before slicing[5..].
Skipping events is valid, but an early boundary check helps avoid potential panics. You could do something like:if event.data.len() < 6 { return Ok(()); } let calldata = event.data[5..].to_vec();
101-102: Ohayo sensei, replaceunwrap()with graceful error handling!
Usingtry_into().unwrap()can panic if the slice conversion fails. This is identical to a previously flagged item. Use a question mark operator or custom error case for safer code:-let cartridge_magic: [Felt; 22] = calldata[3..25].try_into().unwrap(); +let cartridge_magic: [Felt; 22] = calldata[3..25] + .try_into() + .map_err(|_| anyhow::anyhow!("Invalid Cartridge magic slice"))?;
109-111: Ohayo sensei, add fallback checks when parsing username!
This logic matches a past recommendation to avoid out-of-bounds or incorrect data. Make sure you have enough length inevent.dataand handle parsing errors gracefully.
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 (5)
crates/torii/indexer/src/processors/controller.rs(1 hunks)crates/torii/indexer/src/processors/erc20_legacy_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc20_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc721_legacy_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc721_transfer.rs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- crates/torii/indexer/src/processors/erc20_legacy_transfer.rs
- crates/torii/indexer/src/processors/erc721_transfer.rs
- crates/torii/indexer/src/processors/erc721_legacy_transfer.rs
- crates/torii/indexer/src/processors/erc20_transfer.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (9)
crates/torii/indexer/src/processors/controller.rs (9)
1-14: Ohayo sensei!
All these imports and dependencies look appropriate and well-structured. No issues here.
18-22: Looks good, sensei!
DefiningLOG_TARGETand a simpleControllerProcessorstruct keeps things modular and clear.
23-49: Neat approach for storing the Cartridge magic, sensei!
Usinglazy_static!for the magic constant is a clean solution, though consider an alternative likeonce_cell::Lazyif you want a more modern pattern.
51-58: Event key method is straightforward, sensei!
Returning a hardcoded string is perfectly fine here. No changes needed.
65-67: Ohayo sensei, the priority looks consistent!
A hardcoded integer is adequate here unless you need dynamic prioritization.
69-74: Unique task identifier is well-implemented, sensei!
UsingDefaultHasheron the first felt is straightforward and sufficient.
76-93: Add a brief log or comment for early returns, sensei!
Currently, you returnOk(())ifcalldata.len() < 25. Consider logging a debug message indicating you’re ignoring the event due to insufficient data. Also, watch out for potential out-of-bounds ifevent.datais shorter than 5 items, as pointed out in thevalidatesuggestion.
102-102: Avoid.unwrap()here, sensei!
Using.unwrap()can cause a panic if the slice fixation fails, even though your checks suggest it shouldn't. For maximum safety and clarity, consider using a question-mark operator or providing an explicit error.
113-124: Ohayo sensei! Controller deployment logic looks good.
Storing the username/address with a timestamp meets typical indexing requirements. Great job!
| fn validate(&self, event: &Event) -> bool { | ||
| // ContractDeployed event has no keys and contains username in data | ||
| event.keys.len() == 1 && !event.data.is_empty() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider tightening bounds checks in validate, sensei!
You rely on event.data[5..] slicing later in process. If event.data has fewer than 5 items, slicing will silently panic. It may be safer to validate the minimal number of felts here or handle it gracefully in process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/controller.rs (2)
15-22: Ohayo sensei! Consider adding a docstring for clarity.
Adding a brief docstring can help future developers understand what theControllerProcessoris responsible for.+/// Processes contract deployment events for Cartridge controllers. #[derive(Default, Debug)] pub struct ControllerProcessor;
23-49: Ohayo sensei! Magic constant seems fine.
Although this approach is perfectly valid, you might consider alternative representations like a single hex string or dedicated constant for improved maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/processors/controller.rs(1 hunks)crates/torii/migrations/20250128051146_controllers.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/migrations/20250128051146_controllers.sql
🔇 Additional comments (15)
crates/torii/indexer/src/processors/controller.rs (15)
1-2: Ohayo sensei! No issues here.
These initialusestatements look good.
3-14: Ohayo sensei! Import block is consistent.
Everything is neatly grouped, and there are no obvious conflicts or redundancy issues.
51-58: Ohayo sensei! Event key definition looks correct.
This is straightforward and well-named.
60-63: Ohayo sensei! Ensure data length is sufficient invalidate.
Currently, the check only ensuresevent.datais not empty. Because we later sliceevent.data[5..], consider verifyingevent.data.len() >= 6. This concern was previously raised, so marking it as duplicate.
65-67: Ohayo sensei! Priority definition is straightforward.
No changes needed.
69-74: Ohayo sensei! Hashing the first felt is a valid identifier approach.
No further suggestions.
76-85: Ohayo sensei! Process method signature is well-structured.
Returning aResultis good for error propagation.
86-88: Ohayo sensei! Using the first felt in data as the address is consistent with the event.
We already verified there's at least one data item. No issues here.
95-99: Ohayo sensei! Cartridge magic length check is correct.
No issues here since you do a safe early return if it doesn't match.
101-103: Ohayo sensei!.unwrap()is logically safe here.
The precedingcalldata.len() < 25check ensures a valid slice. No further action needed.
105-107: Ohayo sensei! Cartridge magic comparison is clear.
Returning early if it doesn't match is a valid approach.
109-112: Ohayo sensei! Username parsing looks good.
Your usage of?for error propagation ensures we handle parse failures gracefully.
113-118: Ohayo sensei! Logging is concise and useful.
No issues with the log statement and structured fields.
120-123: Ohayo sensei! Database insertion strategy is appropriate.
You return an error if insertion fails, enabling upstream handling. Everything looks good.
89-93:⚠️ Potential issueOhayo sensei! Potential out-of-bounds slicing.
You sliceevent.data[5..]without confirming thatevent.data.len() >= 5. Although you skip processing ifcalldata.len() < 25later, there could be a panic ifevent.data.len()is less than 5.fn validate(&self, event: &Event) -> bool { - event.keys.len() == 1 && !event.data.is_empty() + event.keys.len() == 1 && event.data.len() >= 6 }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/cli/src/options.rs (1)
174-181: Ohayo! Consider improving naming consistency and documentation.The field name
controllersand CLI argument--indexing.cartridgeare inconsistent. Consider either:
- Renaming the field to
cartridgeto match the CLI argument, or- Renaming the CLI argument to
--indexing.controllersto match the field nameAlso, the help message could be more descriptive about what Cartridge controllers are and why someone might want to index them.
/// Whether or not to index Cartridge controllers. #[arg( - long = "indexing.cartridge", + long = "indexing.controllers", default_value_t = false, - help = "Whether or not to index Cartridge controllers." + help = "Whether or not to index Cartridge controllers. When enabled, Torii will track and index controller contracts deployed through the Cartridge system." )] #[serde(default)] pub controllers: bool,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/cli/src/options.rs(3 hunks)
🔇 Additional comments (2)
crates/torii/cli/src/options.rs (2)
196-196: LGTM, sensei! Default implementation looks good.The default value of
falseis appropriate for this opt-in feature.
240-242: LGTM! Merge behavior is consistent.The merge logic follows the established pattern for boolean fields, correctly implementing the "override if false" behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/runner/src/lib.rs (1)
72-77: Add address validation for UDC_ADDRESS, sensei.The UDC address should be validated before creating the Contract instance to prevent runtime issues.
if self.args.indexing.controllers { + // Validate UDC address format + if !starknet::core::utils::is_valid_eth_address(UDC_ADDRESS) { + return Err(anyhow::anyhow!("Invalid UDC address format")); + } self.args .indexing .contracts .push(Contract { address: UDC_ADDRESS, r#type: ContractType::UDC }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/runner/src/lib.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/torii/runner/src/lib.rs (2)
20-20: Ohayo! Verify the UDC address value, sensei.The
UDC_ADDRESSconstant is imported but its value is not visible in the provided code. Let's verify its correctness.Also applies to: 46-48
✅ Verification successful
Ohayo! The UDC address is properly defined and used, sensei!
The constant is correctly defined in
constants.rswith a valid Starknet address value and is appropriately used in the runner for UDC contract configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the UDC address value and its usage rg -p 'UDC_ADDRESS.*=.*".*"' crates/torii/Length of output: 44
Script:
#!/bin/bash # Find the constants module fd constants.rs crates/torii/ # Search for UDC_ADDRESS with broader pattern rg "UDC_ADDRESS" crates/torii/ -A 2Length of output: 878
159-159: Verify default processor configuration, sensei.The change to
Processors::default()assumes all necessary processors are included in the default implementation. Let's verify this assumption.✅ Verification successful
Ohayo! The default processor configuration is properly set, sensei! ✨
The
Processors::default()implementation includes theStoreTransactionProcessoralong with other necessary processors, maintaining all required functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the default processors implementation rg -A 10 'impl Default for Processors' crates/torii/indexer/src/Length of output: 66
Script:
#!/bin/bash # First check the file where Processors is used cat crates/torii/runner/src/lib.rsLength of output: 11973
Script:
#!/bin/bash # Search for Processors in the indexer crate rg -A 10 'struct Processors' crates/torii/indexer/src/Length of output: 912
Script:
#!/bin/bash # Check Engine implementation for processor usage ast-grep --pattern 'impl$$$Engine$$$new($$$)' crates/torii/indexer/src/engine.rsLength of output: 82
Script:
#!/bin/bash # Search for processor registration and StoreTransactionProcessor rg -A 5 'StoreTransactionProcessor|register.*processor' crates/torii/indexer/src/Length of output: 1997
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/torii/cli/src/options.rs(3 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/runner/src/lib.rs(4 hunks)crates/torii/sqlite/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/sqlite/src/lib.rs
- crates/torii/cli/src/options.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
crates/torii/runner/src/lib.rs (2)
20-20: Ohayo! Nice modularization of constants, sensei!Moving constants to a dedicated module improves code organization and maintainability.
Also applies to: 46-48
159-159: Verify default processors configuration, sensei!The change to use
Processors::default()simplifies the code, but let's ensure all necessary processors are included in the default implementation.Run this script to verify the default processors configuration:
✅ Verification successful
Ohayo! Default processors implementation is safe to use, sensei! ✨
The change to
Processors::default()follows the established pattern used throughout the codebase, including test files. Multiple test modules rely on this default implementation, indicating it provides the necessary processor configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify default processors configuration # Test: Check Processors::default() implementation ast-grep --pattern 'impl Default for Processors { fn default() -> Self { $$$ } }'Length of output: 91
Script:
#!/bin/bash # Find Processors struct definition and implementation echo "=== Searching for Processors struct ===" rg "struct Processors" -A 5 echo -e "\n=== Searching for Default implementation ===" rg "impl Default for Processors" -A 10Length of output: 787
Script:
#!/bin/bash # Look for Processors initialization patterns echo "=== Searching for Processors initialization ===" rg "Processors\s*::\s*" -A 5 echo -e "\n=== Searching for new processor methods ===" rg "fn.*new.*Processors" -A 10 echo -e "\n=== Searching for processor builder patterns ===" rg "ProcessorsBuilder" -A 5Length of output: 2156
| if self.args.indexing.controllers { | ||
| self.args | ||
| .indexing | ||
| .contracts | ||
| .push(Contract { address: UDC_ADDRESS, r#type: ContractType::UDC }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! UDC address format inconsistency detected, sensei!
The UDC address has two different formats in the codebase:
0x041a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf(with leading '0')0x41a78e741e5af2fec34b695679bc6891742439f7afb8484ecd7766661ad02bf(without leading '0')
While both represent the same value, consider standardizing the format across all files for better maintainability.
crates/dojo/utils/src/tx/deployer.rs: Update to include leading '0' to match other occurrences
🔗 Analysis chain
Verify UDC_ADDRESS constant value, sensei!
The logic for adding the UDC contract looks good, but let's ensure the UDC_ADDRESS constant is valid.
Run this script to verify the UDC address format and usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify UDC_ADDRESS format and usage
# Test 1: Check if UDC_ADDRESS is a valid Starknet address
ast-grep --pattern 'pub const UDC_ADDRESS: &str = "$_";'
# Test 2: Check if UDC_ADDRESS is used consistently across the codebase
rg "UDC_ADDRESS" -A 2
Length of output: 4969
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates