feat(torii): configutation file for all torii cli options #2646
Conversation
WalkthroughOhayo, sensei! This pull request introduces 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
bin/torii/Cargo.toml(1 hunks)bin/torii/src/main.rs(4 hunks)crates/torii/core/src/types.rs(0 hunks)
💤 Files with no reviewable changes (1)
- crates/torii/core/src/types.rs
🔇 Additional comments (2)
bin/torii/Cargo.toml (1)
43-43: Ohayo! LGTM - Clean dependency addition for TOML support.
The addition of the toml crate with workspace-level version management aligns perfectly with the project's dependency management pattern, sensei!
Let's verify the workspace-level toml dependency:
✅ Verification successful
Ohayo! The toml dependency is properly configured in the workspace, sensei!
The verification shows that:
- The root
Cargo.tomlhastoml = "0.8"declared - Multiple workspace crates are already using toml, including other torii-related crates
- The dependency is actively used in various Rust files across the workspace
The .workspace = true usage in bin/torii/Cargo.toml is correct and follows the established pattern for shared dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify toml dependency is properly declared in workspace
# Expected: Find toml dependency declaration in workspace Cargo.toml
# Check workspace root Cargo.toml for toml dependency
rg 'toml.*=.*"[0-9]+\.' Cargo.toml
# Check if other workspace members also use toml
fd -e toml -x rg 'toml.*=.*"[0-9]+\.' {}
Length of output: 107
Script:
#!/bin/bash
# Let's verify the workspace setup and toml dependency usage more thoroughly
# Check workspace configuration in root Cargo.toml
echo "=== Workspace Configuration ==="
rg -A5 '\[workspace\]' Cargo.toml
# List all Cargo.toml files to understand workspace structure
echo -e "\n=== Workspace Structure ==="
fd Cargo.toml
# Check for toml dependency declarations in all Cargo.toml files
echo -e "\n=== TOML Dependencies Across Workspace ==="
fd Cargo.toml -x rg -l 'toml.*=' {}
# Check actual usage of toml in Rust files
echo -e "\n=== TOML Usage in Code ==="
rg -l 'use.*toml'
Length of output: 2788
bin/torii/src/main.rs (1)
52-52: Ohayo sensei! The trait additions look good!
The addition of Serialize and Deserialize traits to the Args struct is appropriate for TOML configuration file support while maintaining backward compatibility.
bin/torii/src/main.rs
Outdated
| fn from_file(path: &PathBuf) -> anyhow::Result<Self> { | ||
| let content = std::fs::read_to_string(path) | ||
| .with_context(|| format!("Failed to read config file: {}", path.display()))?; | ||
|
|
||
| toml::from_str(&content) | ||
| .with_context(|| "Failed to parse TOML config") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's enhance the error handling and validation!
The from_file implementation could benefit from some improvements:
- Add file existence check before reading
- Trim whitespace from content before parsing
- Provide more specific error context for TOML parsing failures
Here's a suggested improvement:
fn from_file(path: &PathBuf) -> anyhow::Result<Self> {
+ if !path.exists() {
+ anyhow::bail!("Config file not found: {}", path.display());
+ }
let content = std::fs::read_to_string(path)
.with_context(|| format!("Failed to read config file: {}", path.display()))?;
- toml::from_str(&content)
- .with_context(|| "Failed to parse TOML config")
+ toml::from_str(content.trim())
+ .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))
}📝 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.
| fn from_file(path: &PathBuf) -> anyhow::Result<Self> { | |
| let content = std::fs::read_to_string(path) | |
| .with_context(|| format!("Failed to read config file: {}", path.display()))?; | |
| toml::from_str(&content) | |
| .with_context(|| "Failed to parse TOML config") | |
| } | |
| fn from_file(path: &PathBuf) -> anyhow::Result<Self> { | |
| if !path.exists() { | |
| anyhow::bail!("Config file not found: {}", path.display()); | |
| } | |
| let content = std::fs::read_to_string(path) | |
| .with_context(|| format!("Failed to read config file: {}", path.display()))?; | |
| toml::from_str(content.trim()) | |
| .with_context(|| format!("Failed to parse TOML config at: {}", path.display())) | |
| } |
bin/torii/src/main.rs
Outdated
| let args = if let Some(path) = args.config { | ||
| Args::from_file(&path)? | ||
| } else { | ||
| let mut config = ToriiConfig::default(); | ||
|
|
||
| if let Some(contracts) = args.contracts { | ||
| config.contracts = VecDeque::from(contracts); | ||
| } | ||
|
|
||
| config | ||
| args |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Your configuration concerns are valid and need attention!
The codebase analysis reveals:
- No documentation exists about the TOML config file format
- The
Args::from_fileimplementation simply deserializes TOML without precedence rules - No config examples or tests are present in the codebase
- The torii README lacks configuration documentation
Action items:
- Add documentation for the TOML config format matching the
Argsstruct fields:world_address = "0x..." rpc = "http://..." database = "path/to/db" addr = "0.0.0.0:8080" relay_port = 9090
- Implement and document clear precedence rules between CLI args and config
- Add config file examples in the documentation
- Consider adding a
--config-onlyflag for exclusive config file usage
🔗 Analysis chain
Ohayo sensei! Let's address some configuration concerns!
The configuration loading logic could benefit from some improvements:
- Consider adding a config-only mode where other CLI args are ignored
- Document the precedence between CLI args and config file values
- Add documentation about the expected TOML config file format
Let's check if there's any existing documentation about the config file format:
Consider adding a README section or documentation file explaining:
- The TOML configuration file format
- Examples of valid configuration
- Precedence rules between CLI args and config file
Let me analyze the codebase more thoroughly to verify the concerns about configuration documentation and handling. The previous search was too narrow and didn't check the repository structure properly.
I need to verify how the config file is handled and if there's any existing documentation in the torii-specific README before making a final determination about the configuration concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for config file documentation
echo "Checking for config documentation..."
rg -i "config.*file.*format|toml.*config|configuration.*file" README.md docs/
Length of output: 208
Script:
#!/bin/bash
# Let's check for any documentation about configuration in the repository
echo "Checking for any documentation files..."
fd -e md
# Look for TOML-related code and comments
echo -e "\nChecking for TOML-related code..."
rg -i "toml" --type rust
# Check the Args struct definition to understand config handling
echo -e "\nChecking Args struct definition..."
rg "struct Args" -A 20 bin/torii/src/
Length of output: 9376
Script:
#!/bin/bash
# Let's check the implementation of Args::from_file to understand config handling
echo "Checking Args::from_file implementation..."
rg -A 10 "impl.*Args" bin/torii/src/
# Look for any existing config examples or tests
echo -e "\nChecking for config examples or tests..."
rg "config.*toml" bin/torii/
# Check the bin/torii README for any documentation
echo -e "\nChecking torii README..."
cat bin/torii/README.md
Length of output: 1063
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
bin/torii/Cargo.toml(1 hunks)bin/torii/src/main.rs(7 hunks)crates/torii/core/src/types.rs(1 hunks)
🔇 Additional comments (9)
bin/torii/Cargo.toml (1)
43-43: Ohayo! LGTM on the toml dependency.
The addition of the toml dependency with workspace version management is well-structured and follows the project's conventions.
crates/torii/core/src/types.rs (3)
124-128: Ohayo! The Contract struct changes look good, sensei!
Adding the Serialize derive macro is a good enhancement that enables serialization capabilities while maintaining the existing functionality.
Line range hint 130-134: The ContractType enum changes align well with the Contract struct!
Adding the Serialize derive macro here complements the Contract struct changes, enabling consistent serialization capabilities across related types.
Line range hint 1-180: Verify the impact of ToriiConfig removal
The removal of the ToriiConfig struct and its load_from_path method represents a significant architectural change in configuration management. Let's verify that this removal doesn't leave any dangling references in the codebase.
Additionally, please ensure that:
- All existing systems that previously relied on
ToriiConfighave been updated to use the new CLI-based configuration - Documentation has been updated to reflect this architectural change
- Migration guides are provided for users who were using the TOML configuration file
✅ Verification successful
The initial search didn't show any ToriiConfig references, but let's verify how configuration is now handled in the torii crate to ensure the architectural change is complete and properly implemented.
The initial searches show configuration-related code but we need to verify how configuration is now handled after ToriiConfig removal. Let's check for CLI arguments and main entry points.
The previous searches revealed EngineConfig but we need to find where the actual CLI configuration is now handled after ToriiConfig's removal. Let's check the binary crates and configuration-related imports.
Ohayo sensei! Based on our investigation, I can now provide the final response:
ToriiConfig removal is safe and represents a positive architectural change
The removal of ToriiConfig is verified to be safe as:
- No direct references to ToriiConfig exist in the codebase
- Configuration is now handled through
EngineConfigstruct which provides a more focused, runtime-based configuration approach - The only config-related imports are for World metadata and profile configurations, which are separate concerns
- No TOML config file loading patterns remain in the torii crates
The architectural change moves away from file-based configuration to a more maintainable programmatic configuration model, which is a positive improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to ToriiConfig
echo "Checking for ToriiConfig usage..."
rg "ToriiConfig" -l
# Search for any remaining TOML config file loading
echo "Checking for TOML config file loading patterns..."
rg "load_from_path|\.toml" --type rust
Length of output: 4736
Script:
#!/bin/bash
# Let's check for any config-related code in the codebase
echo "Checking for config-related code in torii..."
rg -A 5 "config" --type rust crates/torii/
# Check for any CLI argument handling that might have replaced ToriiConfig
echo "Checking for CLI argument handling..."
ast-grep --pattern 'struct $name {
$$$
}' crates/torii/
Length of output: 23812
Script:
#!/bin/bash
# Let's check for any CLI-related code that might have replaced ToriiConfig
echo "Checking for CLI-related code in torii..."
rg -A 5 "struct.*Args|struct.*Config" crates/torii/
# Let's also check for any main.rs files that might contain the new configuration logic
echo "Checking main.rs files in torii..."
fd "main.rs$" crates/torii/ --exec cat {}
Length of output: 644
Script:
#!/bin/bash
# Let's check for the binary crate that might contain the new CLI configuration
echo "Checking for binary crates in torii..."
fd "Cargo.toml$" bin/ crates/torii/ --exec rg -A 5 "\[\[bin\]\]|name.*=.*\"torii\"" {}
# Let's also check for any new configuration-related imports that might have replaced ToriiConfig
echo "Checking for configuration-related imports..."
rg "use.*config|use.*args" crates/torii/
Length of output: 744
bin/torii/src/main.rs (5)
142-148: Configuration handling improvements look good!
The changes improve the configuration handling:
- Default empty string for contracts is a clean approach
- The config field is properly marked to skip from clap_config
203-205: Clean world address handling!
The changes simplify the world address handling by making it non-optional, and the contract mapping is clean and efficient.
250-250: GRPC server changes look good!
The world address handling in GRPC server initialization is consistent with the new non-optional approach.
343-356: Ohayo sensei! Good defensive programming!
The changes add important validations:
- Clean handling of empty string input
- Prevention of world address misuse as ERC contract
Let's verify no existing code tries to use world address as ERC contract:
#!/bin/bash
# Check for potential world address misuse
rg "ContractType::WORLD.*address" --type rust51-56:
Ohayo sensei! Breaking change alert in Args struct!
The world_address field is now non-optional, which is a breaking change that requires all users to provide a world address. This aligns with the core functionality but needs to be documented.
Let's verify the impact:
|
|
||
| tower-http.workspace = true | ||
| tracing-subscriber.workspace = true | ||
| tracing.workspace = true | ||
| url.workspace = true | ||
| webbrowser = "0.8" | ||
| tempfile.workspace = true | ||
| clap_config = "0.1.1" |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Consider using clap directly instead of clap_config, sensei!
The verification reveals:
clap_configis only used in one place (bin/torii/src/main.rs) for a simpleskipattribute- The package is relatively new (last updated July 2024) with low adoption (~1.8K total downloads)
- The codebase already extensively uses
clapdirectly, as seen in multiple files
The functionality can be replaced with clap's built-in attributes, making the dependency unnecessary:
- Replace
#[clap_config(skip)]with#[arg(skip)]
🔗 Analysis chain
Consider workspace version management and package stability, sensei!
The clap_config dependency has a few concerns:
- It's not using workspace version management (
.workspace = true) like other dependencies - Version 0.1.1 is quite early and might be unstable
- Being a small community package, we should verify its maintenance status
Let's check the package's health:
Consider:
- Moving the version to workspace-level management
- Evaluating more mature alternatives or waiting for a stable release
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify clap_config package health
# Get package info from crates.io API
curl -s https://crates.io/api/v1/crates/clap_config | jq -r '.crate | {max_version, updated_at, downloads, recent_downloads}'
Length of output: 257
Script:
#!/bin/bash
# Check if clap_config is used in workspace dependencies
rg -U "clap_config.*workspace\s*=\s*true" Cargo.toml
# Check workspace root Cargo.toml for any clap_config version specification
rg "clap_config.*version" Cargo.toml
# Find all usages of clap_config in the codebase
rg "clap_config" -A 2 -B 2
# Check for alternative crates that might be used
rg "clap::" -A 2 -B 2
Length of output: 15413
bin/torii/src/main.rs
Outdated
| let matches = <Args as CommandFactory>::command().get_matches(); | ||
| let args = if let Some(path) = matches.get_one::<PathBuf>("config") { | ||
| let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?; | ||
| Args::from_merged(matches, Some(config)) | ||
| } else { | ||
| let mut config = ToriiConfig::default(); | ||
|
|
||
| if let Some(contracts) = args.contracts { | ||
| config.contracts = VecDeque::from(contracts); | ||
| } | ||
|
|
||
| config | ||
| Args::from_arg_matches(&matches)? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Let's enhance the config file handling!
The config file handling could be more robust:
- Add file existence check
- Provide more specific error context for TOML parsing
Consider this improvement:
- let args = if let Some(path) = matches.get_one::<PathBuf>("config") {
- let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?;
- Args::from_merged(matches, Some(config))
+ let args = if let Some(path) = matches.get_one::<PathBuf>("config") {
+ if !path.exists() {
+ anyhow::bail!("Config file not found: {}", path.display());
+ }
+ let content = std::fs::read_to_string(path)
+ .with_context(|| format!("Failed to read config file: {}", path.display()))?;
+ let config: ArgsConfig = toml::from_str(&content)
+ .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))?;
+ Args::from_merged(matches, Some(config))📝 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 matches = <Args as CommandFactory>::command().get_matches(); | |
| let args = if let Some(path) = matches.get_one::<PathBuf>("config") { | |
| let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?; | |
| Args::from_merged(matches, Some(config)) | |
| } else { | |
| let mut config = ToriiConfig::default(); | |
| if let Some(contracts) = args.contracts { | |
| config.contracts = VecDeque::from(contracts); | |
| } | |
| config | |
| Args::from_arg_matches(&matches)? | |
| let matches = <Args as CommandFactory>::command().get_matches(); | |
| let args = if let Some(path) = matches.get_one::<PathBuf>("config") { | |
| if !path.exists() { | |
| anyhow::bail!("Config file not found: {}", path.display()); | |
| } | |
| let content = std::fs::read_to_string(path) | |
| .with_context(|| format!("Failed to read config file: {}", path.display()))?; | |
| let config: ArgsConfig = toml::from_str(&content) | |
| .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))?; | |
| Args::from_merged(matches, Some(config)) | |
| } else { | |
| Args::from_arg_matches(&matches)? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2646 +/- ##
==========================================
+ Coverage 57.17% 57.53% +0.35%
==========================================
Files 399 400 +1
Lines 49731 50272 +541
==========================================
+ Hits 28433 28922 +489
- Misses 21298 21350 +52 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
crates/torii/grpc/src/server/tests/entities_test.rs (1)
108-108: Consider reducing contract information duplication, sensei!The same contract vector is being passed to both
Sql::new()andEngine::new(). Consider extracting this to a constant or setup variable to maintain DRY principles.+ let contracts = vec![Contract { address: world_address, r#type: ContractType::WORLD }]; - let db = Sql::new(pool.clone(), sender, &vec![Contract { address: world_address, r#type: ContractType::WORLD }]) + let db = Sql::new(pool.clone(), sender, &contracts) .await .unwrap(); // ... - &vec![Contract { address: world_address, r#type: ContractType::WORLD }], + &contracts,crates/torii/graphql/src/tests/mod.rs (1)
361-361: Consider extracting contract configuration!The Engine initialization correctly uses the new vector of Contract instances. However, since both Sql::new and Engine::new use the same contract configuration, consider extracting it into a test helper function to maintain DRY principles.
+ fn create_test_contracts(world_address: FieldElement) -> Vec<Contract> { + vec![Contract { + address: world_address, + r#type: ContractType::WORLD + }] + } // Usage in spinup_types_test - let db = Sql::new(pool.clone(), sender, &vec![Contract { address: world_address, r#type: ContractType::WORLD }]) + let db = Sql::new(pool.clone(), sender, &create_test_contracts(world_address)) // Later in the function... - &vec![Contract { address: world_address, r#type: ContractType::WORLD }], + &create_test_contracts(world_address),bin/torii/src/main.rs (3)
142-143: Ohayo sensei! Consider documenting the contracts format!The contracts field now accepts a string format. Please add documentation about the expected format:
--contracts "erc20:0x123,erc721:0x456"
161-162: Ohayo sensei! Consider improving error messages!The world contract is automatically added to the contracts list. Consider adding a comment explaining this behavior to prevent confusion.
+// Add the world contract to the list of contracts to index contracts.push(Contract { address: args.world_address, r#type: ContractType::WORLD });
345-358: Ohayo sensei! Consider enhancing error messages!The function now handles empty strings and prevents world address specification correctly. Consider improving the error message to guide users:
- return Err(anyhow::anyhow!("World address cannot be specified as an ERC contract")); + return Err(anyhow::anyhow!( + "World address cannot be specified as an ERC contract. Use --world flag instead" + ));crates/torii/core/src/sql/test.rs (2)
48-48: Ohayo! Consider adding documentation for the contract parameter, sensei!The change from HashMap to a vector of Contract structs improves type safety. However, it would be helpful to document that this vector is expected to contain the world contract.
pub async fn bootstrap_engine<P>( world: WorldContractReader<P>, db: Sql, provider: P, + /// Vector of contracts, expected to contain the world contract contracts: &[Contract], ) -> Result<Engine<P>, Box<dyn std::error::Error>>
Line range hint
295-299: Ohayo! Important TODOs need tracking, sensei!These TODOs highlight potential issues:
- Record deletion behavior might be incorrect (only zeroing values)
- Need for chronological testing with Torii re-syncing
Would you like me to create GitHub issues to track these improvements?
crates/torii/graphql/src/tests/subscription_test.rs (2)
34-34: Ohayo sensei! The Contract initialization looks good but could be enhanced.The change from HashMap to vec![Contract] makes the contract representation more explicit and type-safe.
Consider adding a test case with multiple contracts to ensure the vector-based storage handles multiple contracts correctly:
- Sql::new(pool.clone(), sender, &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }]) + Sql::new(pool.clone(), sender, &vec![ + Contract { address: Felt::ZERO, r#type: ContractType::WORLD }, + Contract { address: Felt::ONE, r#type: ContractType::WORLD } + ])
178-178: Ohayo sensei! Consider reducing code duplication in test setup.The Contract initialization is repeated across multiple test functions. This could be extracted into a test helper function.
Consider adding a helper function:
fn create_test_contracts() -> Vec<Contract> { vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }] }Then use it in all test functions:
- Sql::new(pool.clone(), sender, &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }]) + Sql::new(pool.clone(), sender, &create_test_contracts())Also applies to: 302-302, 376-376, 451-451
crates/torii/core/src/engine.rs (2)
220-223: Consider optimizing the contract mapping creation.Ohayo, sensei! While the change from accepting a HashMap to creating it from a Vec is functionally correct, we could potentially optimize the contract mapping creation:
Consider this more idiomatic approach:
-let contracts = Arc::new(contracts.iter().map(|contract| (contract.address, contract.r#type)).collect()); +let contracts = Arc::new(contracts + .iter() + .map(|contract| (contract.address, contract.r#type)) + .collect::<HashMap<_, _>>());This explicitly specifies we're collecting into a HashMap and improves readability.
Line range hint
1-750: Consider implementing graceful shutdown handling.Ohayo, sensei! The engine implementation is robust, but there's room for improvement in shutdown handling. Currently, while the engine does handle shutdown signals, it might benefit from a more graceful shutdown process.
Consider implementing these improvements:
- Add a timeout for ongoing tasks during shutdown
- Implement proper cleanup of resources
- Add status reporting during shutdown
This would ensure that no data is lost during shutdown and provide better operational visibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
bin/torii/src/main.rs(7 hunks)crates/torii/core/src/engine.rs(2 hunks)crates/torii/core/src/sql/mod.rs(2 hunks)crates/torii/core/src/sql/test.rs(5 hunks)crates/torii/graphql/src/tests/metadata_test.rs(3 hunks)crates/torii/graphql/src/tests/mod.rs(3 hunks)crates/torii/graphql/src/tests/subscription_test.rs(6 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)
🔇 Additional comments (17)
crates/torii/graphql/src/tests/metadata_test.rs (3)
9-9: Ohayo sensei! Import change looks good!
The addition of the Contract type import aligns well with the codebase's transition from HashMap to structured Contract representations.
121-123: Ohayo sensei! Empty content test changes look perfect!
The modification maintains consistency with the new Contract structure while preserving the important empty content edge case testing.
60-62: Ohayo sensei! Test modification looks solid!
The transition to using Contract struct improves type safety and clarity. Let's verify that this pattern is consistently applied across other test files.
✅ Verification successful
Ohayo sensei! The Contract struct usage is perfectly consistent across all test files!
The verification shows that all Sql::new() calls in test files follow the same pattern using vec![Contract] with proper initialization of the address and r#type fields. This consistent implementation across the codebase confirms that the test modification is correctly aligned with the overall testing strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent Contract struct usage pattern across test files
# Expect: All Sql::new() calls should use Vec<Contract> pattern
# Search for Sql::new calls in test files
rg -t rust "Sql::new\(" --glob "**/tests/**"
Length of output: 1517
crates/torii/grpc/src/server/tests/entities_test.rs (2)
28-28: Ohayo! Import change looks good, sensei!
The addition of Contract to the import statement aligns well with the new contract representation approach.
95-97: Ohayo! The Sql initialization looks clean, sensei!
The switch to using a vector of Contract instances provides a more structured approach to contract representation.
Let's verify the consistency of this pattern across other test files:
✅ Verification successful
Ohayo! The Sql initialization pattern is consistently used across all test files, sensei!
All test files are using the same pattern of initializing Sql with a vector containing a single Contract of type WORLD. The implementation in entities_test.rs perfectly aligns with this established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files using Sql::new to ensure consistent usage
rg -t rust "Sql::new\(" --glob "**/*test.rs" -A 3
Length of output: 4126
crates/torii/graphql/src/tests/mod.rs (2)
33-33: Ohayo sensei! Import changes look good!
The addition of the Contract type import aligns well with the new contract representation approach.
348-350: Ohayo sensei! Let's verify the contract initialization!
The change from HashMap to vector of Contract instances looks good, but let's verify that this is the only instance of Sql::new in the test suite.
✅ Verification successful
Ohayo sensei! All Sql::new instances are consistently updated!
I've verified all instances of Sql::new across the codebase, and they all follow the new vector-based contract initialization pattern. Every occurrence has been properly migrated from the old HashMap style to the new vector format with Contract instances. The changes are consistent across:
- Main application code (
bin/torii/src/main.rs) - Test suites in multiple modules (graphql, libp2p, grpc, core)
- All test instances properly initialize with a world contract using either the provided world_address or Felt::ZERO
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of Sql::new to ensure consistent updates
rg "Sql::new" --type rust
Length of output: 1907
bin/torii/src/main.rs (2)
147-147: Ohayo sensei! Previous error handling concerns are still valid!
The config file handling implementation could benefit from the improvements suggested in the previous review.
Also applies to: 153-158
51-56:
Ohayo sensei! Breaking changes detected in Args struct!
The removal of Option from world_address is a breaking change that requires all users to provide this value. This change improves type safety but requires careful migration planning.
Let's verify the impact on existing users:
crates/torii/core/src/sql/test.rs (2)
27-27: Ohayo! Import change looks good, sensei!
The addition of Contract to the imports aligns well with the new approach of using Contract structs instead of HashMaps.
130-130: Ohayo! Test changes are consistent and well-structured, sensei!
The consistent replacement of HashMap with Contract structs across all test functions maintains test coverage while improving type safety. The world contract is correctly initialized in each test case.
However, we should verify that these changes don't affect test isolation.
Also applies to: 288-288, 374-374
✅ Verification successful
Ohayo! Test isolation is properly maintained, sensei!
The Contract struct usage is consistent and isolated across test cases, with each test creating its own instance. There's no shared state between tests, and each test uses a fresh world_reader.address. The changes maintain proper test isolation while improving type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential test dependencies
# Look for shared state or resources between tests that might affect isolation
# Search for static or shared variables
rg -l "static|lazy_static|thread_local" crates/torii/core/src/sql/
# Search for potential cleanup code to ensure proper test isolation
rg -A 5 -B 5 "#\[tear_down\]|cleanup|drop" crates/torii/core/src/sql/
Length of output: 174
Script:
#!/bin/bash
# Let's check the test module structure and setup/teardown patterns
rg -A 10 "#\[tokio::test\]" crates/torii/core/src/sql/test.rs
# Check for any shared state in the test module
ast-grep --pattern 'mod tests {
$$$
}' crates/torii/core/src/sql/test.rs
# Look for any test helper functions that might handle cleanup
rg "async fn .*(setup|tear|clean|before|after|init)" crates/torii/core/src/sql/test.rs
# Check how Contract struct is used across tests
rg -A 2 "Contract \{" crates/torii/core/src/sql/test.rs
Length of output: 700
crates/torii/libp2p/src/tests.rs (2)
543-543: LGTM!
Ohayo! The import statement is properly placed and correctly scoped to the test module.
581-582: LGTM! Clean transition from HashMap to Contract struct.
Ohayo sensei! The change from HashMap to a vector of Contract instances improves type safety while maintaining the same functionality. The initialization with Felt::ZERO address and ContractType::WORLD type is appropriate for the test context.
crates/torii/core/src/engine.rs (1)
42-42: LGTM! Clean import addition.
The addition of the Contract type import is well-placed and necessary for the refactoring.
crates/torii/core/src/sql/mod.rs (3)
23-23: LGTM! Import change is consistent with the new Contract type.
The change from ContractType to Contract aligns with the broader refactoring effort to standardize contract representation.
70-72: LGTM! Field access is more explicit and readable.
The direct field access using contract.address and contract.r#type improves code clarity compared to tuple indexing.
62-62: Consider performance implications of using Vec instead of HashMap.
Ohayo, sensei! While the change to Vec<Contract> simplifies the data structure, it could impact performance when looking up contracts by address. With a HashMap, lookups are O(1), but with a Vec, they become O(n).
Let's check if there are any contract lookups in the codebase:
Consider maintaining the HashMap for efficient lookups if contract address-based searches are common:
-pub async fn new(
- pool: Pool<Sqlite>,
- executor: UnboundedSender<QueryMessage>,
- contracts: &Vec<Contract>,
-) -> Result<Self> {
+pub async fn new(
+ pool: Pool<Sqlite>,
+ executor: UnboundedSender<QueryMessage>,
+ contracts: &Vec<Contract>,
+) -> Result<Self> {
+ // Create a HashMap for efficient lookups
+ let contract_map: HashMap<Felt, &Contract> = contracts
+ .iter()
+ .map(|c| (c.address, c))
+ .collect();
crates/torii/core/src/engine.rs
Outdated
| contracts: &Vec<Contract>, | ||
| ) -> Self { | ||
| let contracts = Arc::new(contracts.iter().map(|contract| (contract.address, contract.r#type)).collect()); | ||
|
|
There was a problem hiding this comment.
Consider adding contract validation.
Ohayo, sensei! The contract initialization lacks validation for duplicate addresses.
Add validation to ensure no duplicate contract addresses exist in the input vector. This could lead to undefined behavior if multiple contracts share the same address but have different types.
+let mut seen_addresses = HashSet::new();
+for contract in contracts {
+ if !seen_addresses.insert(contract.address) {
+ return Err(anyhow::anyhow!("Duplicate contract address: {:#x}", contract.address));
+ }
+}
let contracts = Arc::new(contracts
.iter()
.map(|contract| (contract.address, contract.r#type))
.collect::<HashMap<_, _>>());Also applies to: 32-38
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
crates/torii/graphql/src/tests/metadata_test.rs (1)
Line range hint
60-123: Consider extracting common test setup, sensei!Both test functions share identical setup code for creating the Sql instance. Consider extracting this into a helper function to improve maintainability and reduce duplication.
Here's a suggested implementation:
async fn setup_test_db(pool: SqlitePool) -> (Sql, Schema) { let (shutdown_tx, _) = broadcast::channel(1); let (mut executor, sender) = Executor::new(pool.clone(), shutdown_tx.clone()).await.unwrap(); tokio::spawn(async move { executor.run().await.unwrap(); }); let db = Sql::new( pool.clone(), sender, &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }] ) .await .unwrap(); let schema = build_schema(&pool).await.unwrap(); (db, schema) }crates/torii/grpc/src/server/tests/entities_test.rs (1)
Line range hint
98-109: Consider using builder pattern for Engine initialization, sensei!While the contract representation change looks good, the Engine constructor has many parameters which could benefit from a builder pattern for better readability and maintainability.
Example refactor:
let mut engine = Engine::builder() .with_world_reader(world_reader) .with_db(db.clone()) .with_provider(Arc::clone(&provider)) .with_processors(Processors::default()) .with_config(EngineConfig::default()) .with_shutdown(shutdown_tx) .with_contracts(vec![Contract { address: world_address, r#type: ContractType::WORLD }]) .build();bin/torii/src/main.rs (2)
141-146: Ohayo sensei! Let's document the contracts field format!The contracts field now accepts a comma-separated string, but its format should be documented in the field's help text.
Add help text to clarify the expected format:
- #[arg(long, default_value = "")] + #[arg(long, default_value = "", help = "Comma-separated list of ERC contracts in format 'type:address' (e.g., 'ERC20:0x123,ERC721:0x456')")] contracts: String,
350-363: Ohayo sensei! Let's improve the error messages!While the contract parsing logic is solid, the error messages could be more helpful.
Consider enhancing the error messages:
- _ => return Err(anyhow::anyhow!("Invalid contract format")), + _ => return Err(anyhow::anyhow!( + "Invalid contract format. Expected 'type:address' (e.g., 'ERC20:0x123' or 'ERC721:0x456')" + )),crates/torii/core/src/sql/test.rs (1)
Line range hint
293-295: TODO comments need attention, sensei!These TODO comments highlight potential issues with record deletion behavior that should be addressed:
- Record deletion vs. value zeroing behavior needs clarification
- Chronological testing with Torii re-syncing needs implementation
Would you like me to help create GitHub issues to track these TODOs?
crates/torii/core/src/engine.rs (1)
220-223: Ohayo sensei! Consider optimizing the contract collection transformation.The change from
HashMaptoVecas input is good for flexibility, but the current implementation creates a new collection on every instantiation. Consider these improvements:
- The
.iter().map().collect()chain could be optimized- The transformation could be moved to a const or static initialization
Here's a more efficient approach:
- let contracts = Arc::new(contracts.iter().map(|contract| (contract.address, contract.r#type)).collect()); + let contracts = Arc::new(contracts + .iter() + .map(|Contract { address, r#type, .. }| (*address, *r#type)) + .collect());The suggested changes:
- Use destructuring in the closure for better readability
- Dereference values directly in the map operation
crates/torii/core/src/sql/mod.rs (1)
64-75: Consider adding explicit error handling for contract data.Ohayo, sensei! While the new implementation is more straightforward, it might benefit from explicit validation of contract data before insertion. Consider adding checks for:
- Valid contract address format
- Valid contract type string
- Duplicate contract entries in the Vec
Here's a suggested improvement:
pub async fn new( pool: Pool<Sqlite>, executor: UnboundedSender<QueryMessage>, contracts: &Vec<Contract>, ) -> Result<Self> { + // Validate contracts + let mut seen = std::collections::HashSet::new(); + for contract in contracts { + if !seen.insert(contract.address) { + return Err(anyhow!("Duplicate contract address: {:#x}", contract.address)); + } + if contract.r#type.is_empty() { + return Err(anyhow!("Empty contract type for address: {:#x}", contract.address)); + } + } + for contract in contracts { executor.send(QueryMessage::other(crates/torii/libp2p/src/tests.rs (2)
543-543: Ohayo, sensei! Verify the necessity of importingContractType.It appears that
ContractTypeis imported but may not be used elsewhere in the code. If it's not required, consider removing it to keep the codebase clean.
581-581: Ohayo, sensei! Simplify the vector reference when callingSql::new.Instead of using
&vec![...], you can pass a slice directly using&[...], which is more idiomatic and efficient in Rust.Apply this diff to simplify the code:
- Sql::new(pool.clone(), sender, &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }]) + Sql::new(pool.clone(), sender, &[Contract { address: Felt::ZERO, r#type: ContractType::WORLD }])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
bin/torii/src/main.rs(7 hunks)crates/torii/core/src/engine.rs(2 hunks)crates/torii/core/src/sql/mod.rs(2 hunks)crates/torii/core/src/sql/test.rs(5 hunks)crates/torii/graphql/src/tests/metadata_test.rs(3 hunks)crates/torii/graphql/src/tests/mod.rs(3 hunks)crates/torii/graphql/src/tests/subscription_test.rs(6 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)
🔇 Additional comments (19)
crates/torii/graphql/src/tests/metadata_test.rs (3)
9-9: Ohayo! New import looks good, sensei!
The addition of Contract type from torii_core::types aligns well with the codebase's transition from HashMap to Vec for contract management.
121-123: Consistent pattern application, nice work sensei!
The same Vec initialization pattern is correctly applied in the second test function, maintaining consistency throughout the test suite.
60-62: Contract initialization change looks solid, sensei!
The transition from HashMap to Vec maintains the same functionality while providing a more structured approach to contract data management. The use of a single-element vector with Felt::ZERO address is consistent with the test's requirements.
Let's verify this pattern is consistently applied across other test files:
✅ Verification successful
Ohayo! The Vec pattern is consistently applied across the codebase, sensei!
The verification shows the same pattern is used consistently in all test files:
crates/torii/libp2p/src/tests.rscrates/torii/graphql/src/tests/subscription_test.rscrates/torii/graphql/src/tests/metadata_test.rscrates/torii/grpc/src/server/tests/entities_test.rs
All instances initialize Sql::new with a vector containing a single Contract with either Felt::ZERO or world_address as the address and ContractType::WORLD as the type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar Sql::new initializations in test files
rg -t rust "Sql::new.*vec!\[Contract \{" "crates/torii" --glob "*test*.rs"
Length of output: 1536
crates/torii/grpc/src/server/tests/entities_test.rs (2)
28-28: Ohayo! Import changes look good, sensei!
The addition of Contract import and its grouping with ContractType aligns well with the new contract representation approach.
95-97: Ohayo! Let's verify the contract initialization, sensei!
The change to Vec<Contract> looks good, but let's verify that this is the only test affected by the contract representation change.
✅ Verification successful
Ohayo! All clear on the contract initialization front, sensei!
I've thoroughly checked all Sql::new usages across the codebase, and I'm happy to report that all test instances are already using the new Vec<Contract> format with ContractType::WORLD. No instances of the old HashMap initialization pattern were found in any test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might need similar updates
rg -l "Sql::new.*HashMap" "crates/torii"
Length of output: 40
Script:
#!/bin/bash
# Let's search for all Sql::new usages to ensure we catch all test cases
rg "Sql::new" "crates/torii" -A 2
# Also search for any remaining HashMap imports that might be related
rg "use.*HashMap" "crates/torii"
# And let's specifically look for test files
fd "test\.rs$" "crates/torii" --exec rg "Sql::new" {} -A 2
Length of output: 6594
crates/torii/graphql/src/tests/mod.rs (2)
33-33: Ohayo! Import looks good, sensei!
The addition of Contract type alongside ContractType aligns well with the new contract representation approach.
348-348: Ohayo! The contract representation changes look solid, sensei!
The shift from HashMap to Vec<Contract> for both Sql::new and Engine::new is implemented consistently. The world contract is properly configured with ContractType::WORLD.
Let's verify that this change is consistent across other test files:
Also applies to: 361-361
✅ Verification successful
Ohayo! The contract representation is consistently implemented across all test files, sensei!
All test files are using the new Vec<Contract> pattern with ContractType::WORLD consistently:
subscription_test.rsusesvec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }]metadata_test.rsusesvec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }]mod.rsusesvec![Contract { address: world_address, r#type: ContractType::WORLD }]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other test files use the same contract representation pattern
# Expected: All test files should use Vec<Contract> instead of HashMap for contract configuration
rg -A 2 "Sql::new|Engine::new" "crates/torii/graphql/src/tests/"
Length of output: 2745
bin/torii/src/main.rs (3)
21-22: Ohayo sensei! The configuration enhancements look good!
The addition of ClapConfig and CommandFactory provides structured configuration file support while maintaining CLI flexibility.
152-158: Ohayo sensei! Let's enhance the config file handling!
The config file handling could be more robust with better error handling.
A previous review comment already suggested improvements for file existence checks and error handling. Those suggestions are still valid and should be implemented.
160-167: Ohayo sensei! The world address handling looks solid!
The implementation properly enforces the world address requirement and correctly adds it to the contracts list.
crates/torii/core/src/sql/test.rs (3)
27-27: Ohayo sensei! Import changes look good!
The addition of Contract import aligns well with the new vector-based contract representation approach.
130-130: Ohayo! SQL initialization changes are consistently applied!
The vector-based contract initialization is uniformly implemented across all test functions, maintaining consistency with the new architecture.
Also applies to: 288-288, 374-374
48-48: Verify backwards compatibility with existing consumers, sensei!
The shift from HashMap to Vec for contract representation is a significant architectural change. While the new approach is cleaner, we should ensure all consumers of this API are updated accordingly.
Consider documenting this architectural change in the codebase to help future maintainers understand the rationale behind using Vec over HashMap.
✅ Verification successful
The change is consistent across all Engine initializations
All instances of Engine::new() in the codebase are already using the new Vec<Contract> pattern consistently. No remaining usages of HashMap for contract initialization were found, and all test files and the main application use the same vector-based approach.
Key findings:
bin/torii/src/main.rs: Production code usingcontractsvectorcrates/torii/graphql/src/tests/mod.rs: Test using vector initializationcrates/torii/grpc/src/server/tests/entities_test.rs: Test using vector initializationcrates/torii/core/src/sql/test.rs: The file under review, using vector initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining HashMap-based contract initialization patterns
rg -l 'HashMap::from\(\[\(.*Contract.*\)\]\)'
# Search for existing Engine::new calls to verify migration
ast-grep --pattern 'Engine::new($_,$_,$_,$_,$_,$_,$_,$_)'
Length of output: 3444
crates/torii/graphql/src/tests/subscription_test.rs (3)
19-19: LGTM! Clean import addition
Ohayo! The import of Contract and ContractType aligns well with the new vector-based contract handling approach.
34-34: Consistent migration from HashMap to Vec
The change from HashMap to Vec is consistently applied across all test cases. However, let's verify that this change doesn't impact the contract lookup performance in the actual implementation.
Also applies to: 178-178, 302-302, 376-376, 451-451
Line range hint 236-239: Verify type change impact
Sensei, I noticed a potential inconsistency in the test data types. The record_id field is defined as U32(Some(0)) here, but in the test_entity_subscription function it's defined as U8(Some(0)). This might lead to type mismatches.
Consider aligning the types to be consistent, preferably using U8 if that's the intended type for record IDs.
crates/torii/core/src/engine.rs (1)
42-42: LGTM! Import added for new Contract type.
The addition of the Contract type import aligns with the changes in the function signature.
crates/torii/core/src/sql/mod.rs (2)
23-23: LGTM! Import aligns with the new contract handling.
The addition of the Contract type import is necessary for the new parameter type in the new method.
61-63: Verify performance impact of switching from HashMap to Vec.
The change from HashMap<Felt, ContractType> to Vec<Contract> changes the access pattern from O(1) to O(n). While this might be acceptable for small contract lists or sequential access patterns, it could impact performance for large contract sets with frequent random access.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
crates/torii/core/src/processors/erc20_transfer.rs (1)
45-45: Consider documenting future plans for _config parameter.Ohayo sensei! While the addition of
_config: &EventProcessorConfigmaintains consistency with other processors, it's currently unused (as indicated by the underscore prefix). Consider adding a TODO comment describing the intended future usage of this configuration parameter to help other developers understand the roadmap.+ // TODO: Implement historical events processing using _config.historical_events _config: &EventProcessorConfig,crates/torii/core/src/processors/store_del_record.rs (1)
38-38: Consider documenting the future use of _config parameter.While adding the
_configparameter maintains consistency with other processors, it's currently unused (as indicated by the underscore prefix). Consider adding a TODO comment explaining the intended future use of this configuration parameter to help other developers understand its purpose.+ // TODO: Implement configuration-driven behavior for historical events _config: &EventProcessorConfig,crates/torii/core/src/processors/erc721_legacy_transfer.rs (1)
45-45: Consider documenting the future use of _config parameterThe
_configparameter is currently unused (indicated by the underscore prefix), but it's part of a broader configuration management update. Consider adding a TODO comment to document the intended future use of this parameter.+ // TODO: Implement configuration usage for ERC721 legacy transfer processing _config: &EventProcessorConfig,crates/torii/core/src/processors/mod.rs (1)
27-30: Ohayo! Consider adding documentation for EventProcessorConfig.The new configuration struct would benefit from documentation explaining:
- The purpose of the
historical_eventsfield- The expected format of the event strings
- Example usage
Add this documentation:
+/// Configuration for event processors +/// +/// # Fields +/// * `historical_events` - List of event identifiers to process historical events. +/// Each string should be in the format "event_key1,event_key2,...". #[derive(Clone, Debug, Default)] pub struct EventProcessorConfig { pub historical_events: Vec<String>, }crates/torii/core/src/processors/event_message.rs (1)
76-77: Consider enhancing the historical events configuration robustness, sensei!The current implementation could benefit from some improvements:
- Move the format string to a constant to ensure consistency across the codebase
- Consider using a more unique separator than '-' to prevent potential naming conflicts
- Add validation for namespace and name format
Here's a suggested improvement:
+ const HISTORICAL_EVENT_KEY_SEPARATOR: &str = "::"; + const HISTORICAL_EVENT_KEY_FORMAT: &str = "{}{}{}"; - let historical = config.historical_events.contains(&format!("{}-{}", model.namespace, model.name)); + let historical = config.historical_events.contains(&format!( + HISTORICAL_EVENT_KEY_FORMAT, + model.namespace, + HISTORICAL_EVENT_KEY_SEPARATOR, + model.name + ));crates/torii/core/src/processors/register_event.rs (1)
Line range hint
33-97: Ohayo! Consider enhancing error handling and validation, senseiThe implementation is robust, but there are a few architectural considerations:
- The comment on line 35 suggests that validation might not be needed anymore. Consider either removing the
validatemethod or documenting why it's retained.- The error handling could be more specific than using the generic
anyhow::Error.- Consider adding configuration options for:
- Custom logging levels
- Validation rules
- Database retry policies
These improvements would make the processor more configurable and maintainable in the future.
crates/torii/core/src/processors/store_update_member.rs (1)
53-53: Consider documenting the future use of _config parameter.While the unused parameter is expected in this WIP PR, it would be helpful to add a TODO comment describing the intended future use of this configuration parameter.
+ // TODO: Implement configuration usage for member updates _config: &EventProcessorConfig,crates/torii/core/src/processors/metadata_update.rs (1)
50-50: Consider utilizing the _config parameter.Ohayo sensei! While adding the config parameter aligns with the codebase-wide changes, it's currently unused (indicated by the _ prefix). Consider either:
- Documenting why this parameter is required but unused
- Utilizing the config for potential customization of metadata processing (e.g., configurable IPFS endpoints, retry counts)
Example enhancement:
- const IPFS_URL: &str = "https://cartridge.infura-ipfs.io/ipfs/"; - const MAX_RETRY: u8 = 3; + fn get_ipfs_url(config: &EventProcessorConfig) -> &str { + config.ipfs_url.as_deref().unwrap_or("https://cartridge.infura-ipfs.io/ipfs/") + } + fn get_max_retry(config: &EventProcessorConfig) -> u8 { + config.max_retry.unwrap_or(3) + }bin/torii/src/main.rs (3)
166-169: Ohayo sensei! Consider usingcontextfor better error messages.The world address validation could provide more helpful error context.
Consider this improvement:
- return Err(anyhow::anyhow!("Please specify a world address.")); + return Err(anyhow::anyhow!("World address is required. Please specify it using --world flag or in the config file."));
279-284: Consider simplifying allowed origins handling.The current implementation could be more concise.
Consider this improvement:
- if args.allowed_origins.is_empty() { None } else { Some(args.allowed_origins) }, + (!args.allowed_origins.is_empty()).then_some(args.allowed_origins),
362-377: Ohayo sensei! Nice improvement to contract parsing!The new
parse_erc_contractfunction is well-structured with proper validation. However, consider adding more specific error messages.Consider this improvement:
match part.split(':').collect::<Vec<&str>>().as_slice() { [r#type, address] => { let r#type = r#type.parse::<ContractType>() + .with_context(|| format!("Invalid contract type: {}", r#type))?; if r#type == ContractType::WORLD { return Err(anyhow::anyhow!( "World address cannot be specified as an ERC contract" )); } let address = Felt::from_str(address) .with_context(|| format!("Expected address, found {}", address))?; Ok(Contract { address, r#type }) } - _ => Err(anyhow::anyhow!("Invalid contract format")), + _ => Err(anyhow::anyhow!("Invalid contract format. Expected format: <type>:<address>")), }crates/torii/core/src/engine.rs (3)
145-145: Ohayo, sensei! Consider adding documentation forevent_processor_config.Adding a doc comment to
event_processor_configinEngineConfigwill improve code readability and help other developers understand its purpose.
222-225: Ohayo, sensei! Optimize the parameter type forcontracts.In the
Engine::newmethod, consider acceptingcontractsas a slice&[Contract]instead of&Vec<Contract>. This allows for greater flexibility and can accept both vectors and arrays.
581-581: Ohayo, sensei! Avoid unnecessary cloning ofevent_processor_config.In the
process_tasksmethod, cloningevent_processor_configmay be unnecessary if it's not modified within the task. Passing a reference&self.config.event_processor_configcould improve performance by avoiding unnecessary clones.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
bin/torii/src/main.rs(9 hunks)crates/torii/core/Cargo.toml(0 hunks)crates/torii/core/src/engine.rs(8 hunks)crates/torii/core/src/processors/erc20_legacy_transfer.rs(2 hunks)crates/torii/core/src/processors/erc20_transfer.rs(2 hunks)crates/torii/core/src/processors/erc721_legacy_transfer.rs(2 hunks)crates/torii/core/src/processors/erc721_transfer.rs(2 hunks)crates/torii/core/src/processors/event_message.rs(3 hunks)crates/torii/core/src/processors/metadata_update.rs(2 hunks)crates/torii/core/src/processors/mod.rs(2 hunks)crates/torii/core/src/processors/raw_event.rs(2 hunks)crates/torii/core/src/processors/register_event.rs(2 hunks)crates/torii/core/src/processors/register_model.rs(2 hunks)crates/torii/core/src/processors/store_del_record.rs(2 hunks)crates/torii/core/src/processors/store_set_record.rs(2 hunks)crates/torii/core/src/processors/store_update_member.rs(2 hunks)crates/torii/core/src/processors/store_update_record.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/torii/core/Cargo.toml
🔇 Additional comments (29)
crates/torii/core/src/processors/raw_event.rs (2)
7-7: LGTM! Clean import addition.
Ohayo! The EventProcessorConfig import is properly grouped with related imports from the same module.
34-34: Consider documenting the intended usage of _config parameter.
Ohayo sensei! While the addition of the _config parameter aligns with the broader changes across other processors, its current unused state (indicated by the underscore prefix) suggests future implementation plans.
Consider adding a TODO comment or documentation to clarify the intended usage of this configuration parameter for future development.
Let's check if other processors provide insights into the intended usage:
crates/torii/core/src/processors/erc20_transfer.rs (1)
9-9: LGTM! Clean import addition.
Ohayo! The import of EventProcessorConfig is well-organized with related imports.
crates/torii/core/src/processors/erc20_legacy_transfer.rs (2)
9-9: Ohayo! Clean import addition looks good, sensei!
The EventProcessorConfig import is properly grouped with related imports from the same module.
45-45: Consider utilizing the config parameter for optimization, sensei!
The _config parameter is currently unused. Since EventProcessorConfig contains historical_events, you could potentially use this to optimize the processing of historical vs. real-time events.
Let's check how other processors are using this config:
crates/torii/core/src/processors/erc721_transfer.rs (2)
9-9: Ohayo! Clean import addition, sensei!
The EventProcessorConfig import is properly grouped with related imports.
45-45: Nice preparation for future configuration capabilities!
The addition of _config parameter aligns with the project-wide pattern of enabling configuration-driven behavior. The underscore prefix correctly indicates that while we're not using the config yet, we're preparing for future extensibility.
Let's confirm this pattern across other processors:
✅ Verification successful
These commands will help us get a better understanding of the processor implementations and verify if the EventProcessorConfig parameter pattern is consistent across the codebase.
Based on the codebase analysis, I can now generate the final response:
Ohayo sensei! The _config parameter is indeed part of a consistent pattern!
The codebase shows that all processor implementations consistently include the _config: &EventProcessorConfig parameter in their process method signatures, following the trait definition in mod.rs. This is a well-established pattern across:
- StoreSetRecordProcessor
- StoreUpdateRecordProcessor
- StoreDelRecordProcessor
- RegisterModelProcessor
- RegisterEventProcessor
- EventMessageProcessor
- All ERC transfer processors
The EventMessageProcessor even actively uses the config parameter for historical events functionality, showing this isn't just preparation but an active feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent EventProcessorConfig parameter addition across processors
# Expected: All process methods should have the config parameter
ast-grep --pattern 'async fn process($$$, _config: &EventProcessorConfig$$$)'
Length of output: 79
Script:
#!/bin/bash
# Let's try a different approach to find process methods in processor files
# First, find all processor files
fd "processor" -e rs -x rg -B 2 -A 5 "async fn process.*EventProcessorConfig" {}
Length of output: 82
Script:
#!/bin/bash
# Let's also check for any process methods that might not have the config parameter
fd "processor" -e rs -x rg -B 2 -A 5 "async fn process" {}
Length of output: 60
Script:
#!/bin/bash
# Let's specifically look in the processors directory
fd . "crates/torii/core/src/processors" -t f -e rs -x cat {}
Length of output: 35396
crates/torii/core/src/processors/store_del_record.rs (1)
9-9: Ohayo! Clean import addition, sensei!
The import of EventProcessorConfig is properly grouped with related imports.
crates/torii/core/src/processors/erc721_legacy_transfer.rs (1)
9-9: Ohayo! Clean import addition, sensei!
The import of EventProcessorConfig is well-organized and follows Rust's module system conventions.
crates/torii/core/src/processors/store_set_record.rs (2)
9-9: Ohayo! Clean import addition, sensei!
The EventProcessorConfig import is properly grouped with related imports from the same module.
39-39: Verify consistent config parameter usage across processors.
The addition of _config parameter aligns with the broader changes in the event processing framework. The underscore prefix correctly indicates intentional non-usage at this stage.
Let's verify consistent implementation across other processors:
✅ Verification successful
Ohayo sensei! After analyzing the codebase, I can now provide a definitive response:
The _config parameter is correctly aligned with the trait definition
The EventProcessor trait in mod.rs explicitly defines the process method with _config: &EventProcessorConfig parameter, making it a required part of the interface. This matches perfectly with the implementation in the reviewed file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all event processors have been updated with the config parameter
# Expected: All process methods should have the EventProcessorConfig parameter
# Search for all processor implementations
ast-grep --pattern 'impl$_ EventProcessor<$_> for $_'
# Search for process method signatures to verify config parameter
rg -U "async fn process.*EventProcessorConfig"
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to find processor implementations and their process methods
# Find all processor files
fd "processor" -e rs
# Search for process method implementations with more context
rg -U "async fn process.*&" -A 3
# Search specifically for EventProcessor trait implementations with context
rg -U "impl.*EventProcessor" -A 5
Length of output: 8821
Script:
#!/bin/bash
# Let's check the process method implementations in these processor files
# Search for process method signatures in all processor files
rg -U "async fn process.*&.*EventProcessorConfig" crates/torii/core/src/processors/
rg -U "fn process.*&.*EventProcessorConfig" crates/torii/core/src/processors/
# Let's also check the EventProcessor trait definition
rg -U "trait EventProcessor" -A 10 crates/torii/core/src/
Length of output: 978
Script:
#!/bin/bash
# Let's check the actual process method in the trait and its implementations
# Search for process method in trait definition with more context
rg -U "fn process.*&" crates/torii/core/src/processors/mod.rs -A 5
# Search for actual process method implementations in processor files
rg -U "async fn process.*&.*config" crates/torii/core/src/processors/ -A 3
Length of output: 145
Script:
#!/bin/bash
# Let's try a more specific search in the processors directory
# Look for process method implementations with any signature
rg -U "async fn process.*&.*Db.*&.*Provider" crates/torii/core/src/processors/ -A 5
# Also check the trait definition file content
cat crates/torii/core/src/processors/mod.rs
Length of output: 2185
crates/torii/core/src/processors/event_message.rs (2)
9-9: Ohayo! Clean import addition, sensei!
The EventProcessorConfig import is well-placed with related imports and properly scoped.
38-38: Configuration parameter addition looks solid!
The new config parameter is properly integrated into the process method signature, maintaining consistency with other processor implementations.
crates/torii/core/src/processors/store_update_record.rs (2)
10-10: Ohayo! Clean import addition looks good, sensei!
The EventProcessorConfig import is properly grouped with related imports.
39-39: Consider utilizing the EventProcessorConfig parameter
The _config parameter is currently unused (prefixed with underscore). Since EventProcessorConfig contains historical_events information, consider implementing historical event processing logic if needed.
Let's check how other processors utilize this config:
crates/torii/core/src/processors/register_model.rs (2)
10-10: Ohayo! Clean import addition, sensei!
The EventProcessorConfig import is properly grouped with related imports from the same module.
41-41: Consider documenting the config parameter's purpose.
While the _config parameter addition aligns with the standardization across processors, it's currently unused (as indicated by the underscore prefix). Consider adding a comment explaining its intended future use or remove it if it's not needed.
Let's check if other processors are actively using this parameter:
crates/torii/core/src/processors/register_event.rs (2)
10-10: Ohayo! Import change looks good, sensei!
The addition of EventProcessorConfig to the imports is well-placed and follows Rust conventions.
41-41: Ohayo sensei! Verify the unused config parameter
The _config parameter is currently unused in the implementation. Since this PR is marked as WIP, I assume this is intentional and will be utilized in future updates.
Would you like assistance with implementing the configuration usage in this processor? I can help identify which config options might be relevant for the event registration process.
crates/torii/core/src/processors/store_update_member.rs (1)
12-12: Ohayo! Import addition looks good, sensei!
The addition of EventProcessorConfig import aligns with the changes in other processors and prepares for future configuration integration.
crates/torii/core/src/processors/metadata_update.rs (2)
18-18: LGTM! Clean import addition.
Ohayo! The EventProcessorConfig import aligns well with the broader changes across the codebase.
Line range hint 35-52: LGTM! Clean method signature update.
The process method signature change is well-structured and maintains consistency with other processors in the codebase.
bin/torii/src/main.rs (3)
21-22: Ohayo sensei! Nice enhancement with ClapConfig!
The addition of ClapConfig trait provides a clean separation between CLI arguments and configuration file handling.
Also applies to: 52-52
254-256: LGTM! Clean implementation of historical events configuration.
The EventProcessorConfig integration is well-structured and maintains separation of concerns.
Also applies to: 260-260
172-173: Verify world contract addition logic.
The commented-out code and direct push of world contract needs verification to ensure it doesn't cause duplicate entries.
✅ Verification successful
Let me gather more context about the contract handling logic.
Let me check one more thing about the args structure and contract parsing.
Ohayo sensei!
The world contract addition is safe and correct
The commented-out parse_erc_contracts function is replaced with a direct push of the world contract, which is actually safer because:
- The world contract is added only once, right after validating
world_addressis present - The code shows
ContractType::WORLDis specifically handled in validation checks args.contractsis not populated anywhere else before this push
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential duplicate world contract handling
echo "Checking for world contract handling..."
rg -A 5 "Contract \{ address: world_address" bin/torii/src/
Length of output: 597
Script:
#!/bin/bash
# Check the Contract struct definition and related code
echo "Checking Contract struct and related code..."
rg -B 5 -A 10 "struct Contract" bin/torii/src/
# Check for any contract parsing or validation logic
echo -e "\nChecking contract parsing and validation..."
rg -B 3 -A 5 "parse_erc_contracts" bin/torii/src/
# Check for any other world contract handling
echo -e "\nChecking other world contract handling..."
rg "ContractType::WORLD" bin/torii/src/
Length of output: 1192
Script:
#!/bin/bash
# Check the CLI args structure and contract parsing
echo "Checking CLI args structure..."
rg -B 10 -A 5 "struct Args" bin/torii/src/
# Check the parse_erc_contracts function implementation
echo -e "\nChecking parse_erc_contracts implementation..."
ast-grep --pattern 'fn parse_erc_contracts($_) {
$$$
}'
Length of output: 1135
crates/torii/core/src/engine.rs (4)
594-594: Ohayo, sensei! Ensure thread safety of event_processor_config.
When passing event_processor_config to the process method in an asynchronous context, verify that it is thread-safe. If EventProcessorConfig contains any interior mutability, ensure proper synchronization to prevent race conditions.
804-804: Ohayo, sensei! Consistent usage of event_processor_config.
Great to see event_processor_config being passed to the processor's process method. Ensure all processors that depend on this configuration are updated accordingly.
864-864: Ohayo, sensei! Verify processor compatibility with event_processor_config.
As event_processor_config is now passed to processors, confirm that all necessary processors have been updated to accept and utilize this new parameter.
224-224:
Ohayo, sensei! Potential issue with duplicate contract addresses.
When converting contracts into a HashMap, duplicate addresses may silently overwrite previous entries. Consider adding validation to check for duplicate contract addresses to prevent unexpected behavior.
| @@ -46,6 +51,7 @@ where | |||
| block_timestamp: u64, | |||
| event_id: &str, | |||
| event: &Event, | |||
| _config: &EventProcessorConfig, | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! The trait modification needs attention, sensei!
The search results reveal multiple issues with the _config parameter addition:
- There are 12+ implementations of the
EventProcessortrait that need updating - The trait actually has multiple
processmethod signatures in the module, which suggests potential trait inheritance or specialization - Current implementations like
StoreDelRecordProcessorandErc20TransferProcessorhave different process method signatures than what's shown in the trait
This indicates a more complex trait hierarchy that needs careful consideration before adding the new parameter.
Please:
- Clarify which
processmethod signature is being modified - Update all affected implementations consistently
- Document the purpose and usage of the config parameter
🔗 Analysis chain
Verify the impact of the trait modification, sensei!
The addition of the _config parameter is a breaking change that affects all implementors of the EventProcessor trait. The underscore prefix suggests this parameter might be unused in some implementations.
Let's check for existing implementations that need updating:
Consider:
- Adding documentation explaining how implementations should use this configuration
- Removing the underscore prefix if the parameter is intended to be used by all processors
- Making this change behind a feature flag if backward compatibility is needed
The previous ast-grep pattern might have been too specific. Let's try with ripgrep to find implementations and understand the trait definition better.
Let's verify the process method signature in both the trait and implementations to understand the impact of this change better, sensei!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of EventProcessor trait
ast-grep --pattern 'impl$_ EventProcessor<$_> for $_'
Length of output: 55
Script:
#!/bin/bash
# Let's try a different pattern to find EventProcessor implementations
rg "impl.*EventProcessor.*for" -A 5
# Also search for the trait definition to understand the context better
rg "trait EventProcessor" -A 10
Length of output: 7354
Script:
#!/bin/bash
# Let's check the process method implementation in the trait
rg "fn process" crates/torii/core/src/processors/mod.rs -A 5
# And check a few implementations to see if they're using the config parameter
rg "fn process" crates/torii/core/src/processors/store_del_record.rs -A 5
rg "fn process" crates/torii/core/src/processors/erc20_transfer.rs -A 5
Length of output: 965
| let matches = <Args as CommandFactory>::command().get_matches(); | ||
| let mut args = if let Some(path) = matches.get_one::<PathBuf>("config") { | ||
| let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?; | ||
| Args::from_merged(matches, Some(config)) | ||
| } else { | ||
| let mut config = ToriiConfig::default(); | ||
|
|
||
| if let Some(contracts) = args.contracts { | ||
| config.contracts = VecDeque::from(contracts); | ||
| } | ||
| Args::from_arg_matches(&matches)? | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling for config file loading.
The current implementation could benefit from more robust error handling and validation.
Consider this improvement:
let matches = <Args as CommandFactory>::command().get_matches();
let mut args = if let Some(path) = matches.get_one::<PathBuf>("config") {
- let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?;
+ let content = std::fs::read_to_string(path)
+ .with_context(|| format!("Failed to read config file: {}", path.display()))?;
+ let config: ArgsConfig = toml::from_str(&content)
+ .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))?;
Args::from_merged(matches, Some(config))
} else {
Args::from_arg_matches(&matches)?
};📝 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 matches = <Args as CommandFactory>::command().get_matches(); | |
| let mut args = if let Some(path) = matches.get_one::<PathBuf>("config") { | |
| let config: ArgsConfig = toml::from_str(&std::fs::read_to_string(path)?)?; | |
| Args::from_merged(matches, Some(config)) | |
| } else { | |
| let mut config = ToriiConfig::default(); | |
| if let Some(contracts) = args.contracts { | |
| config.contracts = VecDeque::from(contracts); | |
| } | |
| Args::from_arg_matches(&matches)? | |
| }; | |
| let matches = <Args as CommandFactory>::command().get_matches(); | |
| let mut args = if let Some(path) = matches.get_one::<PathBuf>("config") { | |
| let content = std::fs::read_to_string(path) | |
| .with_context(|| format!("Failed to read config file: {}", path.display()))?; | |
| let config: ArgsConfig = toml::from_str(&content) | |
| .with_context(|| format!("Failed to parse TOML config at: {}", path.display()))?; | |
| Args::from_merged(matches, Some(config)) | |
| } else { | |
| Args::from_arg_matches(&matches)? | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
bin/torii/torii.toml (1)
2-4: Ohayo sensei! Consider using environment-specific configuration.The hardcoded values for
world_address,addr, andrpcappear to be development defaults. This could cause issues in different environments (development, staging, production).Consider:
- Using environment variables for these values
- Providing a template file (e.g.,
torii.example.toml) with placeholder values- Adding
torii.tomlto.gitignoreto prevent committing environment-specific valuesbin/torii/src/main.rs (5)
95-96: Consider adding documentation for the new configuration options.The new features like
historical_eventsand the comma-separatedallowed_originswould benefit from:
- Example values in the help text
- Documentation about the format and impact of historical events
- Example configuration file showing all options
Also applies to: 142-148, 152-152
166-169: Enhance the world address error message.Consider providing more context in the error message:
- return Err(anyhow::anyhow!("Please specify a world address.")); + return Err(anyhow::anyhow!("World address is required. Use --world option or specify it in the config file."));
172-173: Remove the commented code.The commented line
// let mut contracts = parse_erc_contracts(&args.contracts)?;should be removed as it's no longer needed.
279-284: Consider validating allowed origins format.The allowed origins are used without validation. Consider:
- Validating URL format for non-wildcard values
- Ensuring wildcard (*) is the only value when used
362-376: Enhance contract parsing error messages.Consider providing more helpful error messages:
- _ => Err(anyhow::anyhow!("Invalid contract format")), + _ => Err(anyhow::anyhow!("Invalid contract format. Expected 'type:address' where type is a valid contract type and address is a hex string.")),crates/torii/graphql/src/tests/subscription_test.rs (1)
Line range hint
249-252: Ohayo sensei! There's an inconsistency in record_id types between tests.In
test_entity_subscription, record_id usesPrimitive::U8(Some(0)), while here it usesPrimitive::U32(Some(0)). This inconsistency could lead to confusion and potential issues if the actual implementation expects a specific type.Please verify the correct type and update accordingly:
Member { name: "record_id".to_string(), key: false, - ty: Ty::Primitive(Primitive::U32(Some(0))), + ty: Ty::Primitive(Primitive::U8(Some(0))), },crates/torii/core/src/engine.rs (1)
585-585: Optimize EventProcessorConfig sharing.Ohayo, sensei! The EventProcessorConfig is being cloned for each task. Consider wrapping it in an Arc to avoid unnecessary cloning:
-let event_processor_config = self.config.event_processor_config.clone(); +let event_processor_config = Arc::new(self.config.event_processor_config.clone());This would reduce memory usage when processing many events in parallel.
Also applies to: 808-808, 868-868
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
bin/torii/src/main.rs(9 hunks)bin/torii/torii.toml(1 hunks)crates/torii/core/src/engine.rs(8 hunks)crates/torii/core/src/processors/event_message.rs(3 hunks)crates/torii/core/src/processors/mod.rs(3 hunks)crates/torii/core/src/sql/test.rs(5 hunks)crates/torii/graphql/src/tests/metadata_test.rs(3 hunks)crates/torii/graphql/src/tests/mod.rs(3 hunks)crates/torii/graphql/src/tests/subscription_test.rs(6 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(3 hunks)crates/torii/libp2p/src/tests.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/torii/core/src/processors/event_message.rs
- crates/torii/core/src/processors/mod.rs
- crates/torii/core/src/sql/test.rs
- crates/torii/graphql/src/tests/metadata_test.rs
- crates/torii/graphql/src/tests/mod.rs
- crates/torii/grpc/src/server/tests/entities_test.rs
- crates/torii/libp2p/src/tests.rs
🔇 Additional comments (7)
bin/torii/torii.toml (1)
6-6: Document the historical events configuration, sensei!
The historical_events array contains namespace-specific events, but their purpose and format aren't documented.
Let's verify these event names in the codebase:
Consider adding:
- Comments explaining the purpose of historical events
- Documentation about the "ns-" prefix convention
- Validation to ensure event names follow the expected format
bin/torii/src/main.rs (3)
21-22: Ohayo sensei! Nice enhancement to configuration handling!
The addition of ClapConfig and its derivation for Args enables structured configuration file support, improving the flexibility of the CLI.
Also applies to: 52-52
158-164: Previous review comment about error handling is still applicable.
254-256: Consider validating historical events format.
The historical events are collected without validation. Consider validating that each event follows the expected "namespace-name" format.
crates/torii/graphql/src/tests/subscription_test.rs (1)
Line range hint 1-545: Ohayo sensei! The test structure looks solid!
The tests are well-organized with good coverage of different subscription scenarios. Each test follows a clear pattern of setup, execution, and verification, making them easy to understand and maintain.
crates/torii/core/src/engine.rs (2)
40-42: LGTM! Clean config integration.
The EventProcessorConfig integration is well-structured and properly initialized in the default implementation.
Also applies to: 147-147, 160-160
Line range hint 585-598: Consider potential race conditions in parallel event processing.
Ohayo, sensei! While the event processor config integration looks good, there's a potential race condition in the parallel event processing. Multiple tasks could potentially process events for the same contract simultaneously without proper synchronization.
Let's verify if there are any synchronization mechanisms in place:
Consider adding contract-level synchronization to prevent concurrent modifications to the same contract's state. This could be implemented using a mutex per contract address in the task processing logic.
| type="ERC20" | ||
| address="0x0" | ||
|
|
||
| [[contracts]] | ||
| type="ERC721" | ||
| address="0x123" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance contract configuration validation and documentation.
The contract definitions use placeholder addresses and lack documentation about supported types.
Consider adding:
- Address validation (proper length and format)
- Documentation of supported contract types
- Schema validation for the entire configuration file
Here's a suggested format with documentation:
# Contract Definitions
# Supported types: ERC20, ERC721
# Address format: 0x-prefixed hex string of appropriate length
[[contracts]]
# ERC20 token contract
type = "ERC20"
address = "0x1234567890123456789012345678901234567890" # Example format
[[contracts]]
# ERC721 NFT contract
type = "ERC721"
address = "0x1234567890123456789012345678901234567890" # Example format| let mut db = Sql::new( | ||
| pool.clone(), | ||
| sender, | ||
| &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }], | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider extracting repeated Sql initialization to a helper function.
The same Contract initialization code is repeated across all test functions. This violates the DRY principle and makes maintenance more challenging.
Consider extracting this to a helper function:
async fn create_test_sql(pool: SqlitePool, sender: mpsc::Sender<()>) -> anyhow::Result<Sql> {
Sql::new(
pool,
sender,
&vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }],
)
.await
}Then use it in all test functions:
- let mut db = Sql::new(
- pool.clone(),
- sender,
- &vec![Contract { address: Felt::ZERO, r#type: ContractType::WORLD }],
- )
- .await
- .unwrap();
+ let mut db = create_test_sql(pool.clone(), sender).await.unwrap();Also applies to: 180-186, 307-313, 384-390, 462-468
| contracts: &[Contract], | ||
| ) -> Self { | ||
| let contracts = Arc::new( | ||
| contracts.iter().map(|contract| (contract.address, contract.r#type)).collect(), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add validation for duplicate contract addresses.
Ohayo, sensei! The contract initialization still lacks validation for duplicate addresses. This could lead to undefined behavior if multiple contracts share the same address but have different types.
Apply this diff to add validation:
pub fn new(
world: WorldContractReader<P>,
db: Sql,
provider: P,
processors: Processors<P>,
config: EngineConfig,
shutdown_tx: Sender<()>,
block_tx: Option<BoundedSender<u64>>,
contracts: &[Contract],
) -> Self {
+ let mut seen_addresses = HashSet::new();
+ for contract in contracts {
+ if !seen_addresses.insert(contract.address) {
+ panic!("Duplicate contract address: {:#x}", contract.address);
+ }
+ }
let contracts = Arc::new(
contracts.iter().map(|contract| (contract.address, contract.r#type)).collect(),
);📝 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.
| contracts: &[Contract], | |
| ) -> Self { | |
| let contracts = Arc::new( | |
| contracts.iter().map(|contract| (contract.address, contract.r#type)).collect(), | |
| ); | |
| contracts: &[Contract], | |
| ) -> Self { | |
| let mut seen_addresses = HashSet::new(); | |
| for contract in contracts { | |
| if !seen_addresses.insert(contract.address) { | |
| panic!("Duplicate contract address: {:#x}", contract.address); | |
| } | |
| } | |
| let contracts = Arc::new( | |
| contracts.iter().map(|contract| (contract.address, contract.r#type)).collect(), | |
| ); | |
Larkooo
changed the title
steebchen
changed the title
| @@ -72,7 +73,8 @@ where | |||
| entity.deserialize(&mut keys_and_unpacked)?; | |||
|
|
|||
| // TODO: this must come from some torii's configuration. | |||
| let historical = false; | |||
| let historical = | |||
There was a problem hiding this comment.
To make it more effective, we should compute the selector when torii loads the configuration, like so we only have 1 felt to compare.
But not blocking at all and can be done later. 👍
Summary by CodeRabbit
New Features
EventProcessorConfigto allow configuration-driven behavior in event processing.torii.tomlfor defining contracts and historical events.Bug Fixes
ToriiConfigstruct, enhancing overall application performance.Chores
tomlandclap_configcrates for better configuration handling.HashMapwith vectors ofContractstructs.