refactor(torii): fragment into different modules #2856
Conversation
Larkooo
changed the title
Larkooo
changed the title
WalkthroughOhayo, sensei! This pull request represents a significant architectural refactoring of the Torii project, primarily focusing on modularizing and reorganizing the codebase. The key transformation involves splitting the monolithic Changes
Sequence DiagramsequenceDiagram
participant Core as torii-core
participant SQLite as torii-sqlite
participant Indexer as torii-indexer
Core ->> SQLite: Migrate core functionality
Core ->> Indexer: Extract indexing logic
SQLite -->> Core: Specialized SQLite implementation
Indexer -->> Core: Dedicated indexing module
Note over SQLite, Indexer: Modular architecture achieved
The sequence diagram illustrates the transformation from a monolithic core module to specialized, focused modules that handle specific responsibilities more effectively. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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: 1
🧹 Nitpick comments (2)
crates/torii/sql/Cargo.toml (2)
15-15: Consider using workspace versioning for consistencyThese dependencies have explicit versions while others use workspace versioning:
- bitflags = "2.6.0"
- futures-channel = "0.3.0"
- slab = "0.4.2"
- tokio = { version = "1.32.0", ... }
Consider moving these versions to the workspace-level configuration for consistent dependency management across the project.
Also applies to: 23-23, 31-31, 36-36
37-37: Remove commented dependencyThe commented
tokio-streamdependency should be removed if it's not needed. If it will be needed in the future, consider tracking this in a GitHub issue instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/core/src/lib.rs(0 hunks)crates/torii/sql/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- crates/torii/core/src/lib.rs
🔇 Additional comments (3)
crates/torii/sql/Cargo.toml (3)
1-10: Ohayo! Package metadata looks clean, sensei!The package metadata is well-structured and properly uses workspace inheritance for common fields.
42-48: Dev dependencies look good, sensei!The development dependencies are appropriately configured using workspace versioning.
36-36: Verify tokio feature flagsLet's verify that the tokio feature flags are sufficient for the SQL implementation.
✅ Verification successful
Tokio feature flags are correctly configured
Ohayo sensei! Based on the codebase analysis, the tokio feature flags in
crates/torii/sql/Cargo.tomlare perfectly aligned with the SQL implementation's requirements:
macros: Used extensively for#[tokio::test]and#[tokio::main]attributes across the codebasesync: Heavily utilized through various synchronization primitives:
tokio::sync::RwLockin SQL cache and server componentstokio::sync::broadcast,mpsc, andSemaphorein SQL executor- Multiple other sync primitives used throughout the SQL-related modules
The current feature set provides all the necessary tokio functionality required by the SQL implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tokio usage patterns to verify feature requirements echo "Searching for tokio::sync usage..." rg "tokio::sync::" -t rust echo "Searching for tokio macro usage..." rg "^[[:space:]]*#\\[tokio::" -t rustLength of output: 10995
crates/torii/sql/Cargo.toml
Outdated
| [dependencies] | ||
| anyhow.workspace = true | ||
| async-trait.workspace = true | ||
| base64.workspace = true | ||
| bitflags = "2.6.0" | ||
| cainome.workspace = true | ||
| chrono.workspace = true | ||
| crypto-bigint.workspace = true | ||
| data-url.workspace = true | ||
| dojo-types.workspace = true | ||
| dojo-utils.workspace = true | ||
| dojo-world.workspace = true | ||
| futures-channel = "0.3.0" | ||
| futures-util.workspace = true | ||
| hashlink.workspace = true | ||
| num-traits.workspace = true | ||
| once_cell.workspace = true | ||
| reqwest.workspace = true | ||
| serde.workspace = true | ||
| serde_json.workspace = true | ||
| slab = "0.4.2" | ||
| sqlx.workspace = true | ||
| starknet-crypto.workspace = true | ||
| starknet.workspace = true | ||
| thiserror.workspace = true | ||
| tokio = { version = "1.32.0", features = [ "macros", "sync" ], default-features = true } | ||
| # tokio-stream = "0.1.11" | ||
| ipfs-api-backend-hyper.workspace = true | ||
| tokio-util.workspace = true | ||
| tracing.workspace = true | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Security advisory check completed - action needed for tokio version
The security scan revealed that tokio v1.32.0 (used in torii/sql) is vulnerable to a "ReadHalf::unsplit is Unsound" issue (LOW severity). The workspace is using tokio v1.39.2, which is the patched version.
- Update tokio in
crates/torii/sql/Cargo.tomlto use workspace version instead of pinned v1.32.0:
tokio = { workspace = true, features = [ "macros", "sync" ], default-features = true }For other dependencies:
- sqlx v0.8.2 is safe (patches vulnerability found in <=0.8.0)
- No known vulnerabilities found for bitflags, futures-channel, slab, and reqwest
🔗 Analysis chain
Verify security advisories for direct dependencies
Let's check for any known security vulnerabilities in the direct dependencies.
Let me check the actual versions being used in the workspace before making the final assessment, since we found some security advisories for tokio and sqlx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories for key dependencies
for pkg in "bitflags" "futures-channel" "slab" "tokio" "sqlx" "reqwest"; do
echo "Checking $pkg..."
gh api graphql -f query="
query {
securityVulnerabilities(first: 5, ecosystem: RUST, package: \"$pkg\") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}"
done
Length of output: 4480
Script:
#!/bin/bash
# Let's check the workspace dependencies to get the actual versions being used
echo "Checking workspace dependencies..."
rg -A 1 '^\[workspace\.dependencies\]' -g 'Cargo.toml'
# Also check the root Cargo.toml for version specifications
echo "Checking root Cargo.toml for tokio and sqlx versions..."
rg "^(tokio|sqlx)\s*=" -g 'Cargo.toml'
Length of output: 1489
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/torii/engine/src/constants.rs (1)
2-2: Ohayo, sensei! Consider usingpub constfor all related constants to ensure uniform availability.
TheLOG_TARGETis declared aspub(crate), whereas other constants in this file arepub. If you need consistent visibility across crates, consider usingpubforLOG_TARGETtoo.crates/torii/server/Cargo.toml (1)
Line range hint
1-40: Consider standardizing dependency version managementSome dependencies use workspace versions while others have fixed versions. For consistency, consider moving these to workspace management:
http-body = "0.4.5"tokio-util = "0.7.7"form_urlencoded = "1.2.1"async-trait = "0.1.83"tokio-tungstenite = "0.20.0"hyper-tungstenite = "0.11.1"crates/torii/sqlite/Cargo.toml (1)
Line range hint
1-53: Clean up commented code and standardize dependency versions
- Remove the commented out dependency:
- # tokio-stream = "0.1.11"
- Consider moving these dependencies to workspace management:
bitflags = "2.6.0"futures-channel = "0.3.0"slab = "0.4.2"tokio = { version = "1.32.0", ... }crates/torii/graphql/Cargo.toml (1)
Line range hint
1-53: Consider standardizing dependency version managementFor better maintainability, consider moving these dependencies to workspace management:
async-graphql = { version = "7.0.11", features = [...] }async-graphql-warp = "7.0.11"async-recursion = "1.0.5"convert_case = "0.6.0"tokio-stream = "0.1.11"serial_test = "2.0.0"crates/torii/libp2p/Cargo.toml (1)
Line range hint
1-1: Consider documenting the new architecture, sensei!The split of
torii-coreintotorii-engineandtorii-sqliterepresents a significant architectural change. Consider adding architecture documentation that explains:
- The responsibilities of each new component
- The interaction patterns between engine and sqlite
- The rationale behind the split
- Future extensibility points (e.g., supporting other databases)
This will help future contributors understand the new structure better.
Would you like me to help create a template for this documentation?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Cargo.toml(1 hunks)bin/torii/Cargo.toml(1 hunks)crates/torii/cli/Cargo.toml(1 hunks)crates/torii/engine/Cargo.toml(1 hunks)crates/torii/engine/src/constants.rs(1 hunks)crates/torii/engine/src/engine.rs(1 hunks)crates/torii/engine/src/lib.rs(1 hunks)crates/torii/graphql/Cargo.toml(1 hunks)crates/torii/grpc/Cargo.toml(2 hunks)crates/torii/libp2p/Cargo.toml(1 hunks)crates/torii/server/Cargo.toml(1 hunks)crates/torii/sqlite/Cargo.toml(1 hunks)crates/torii/sqlite/src/constants.rs(1 hunks)crates/torii/sqlite/src/mod.rs(1 hunks)crates/torii/sqlite/src/utils.rs(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- crates/torii/engine/src/lib.rs
- crates/torii/sqlite/src/constants.rs
- crates/torii/engine/src/engine.rs
- crates/torii/engine/Cargo.toml
🔇 Additional comments (12)
crates/torii/sqlite/src/utils.rs (3)
10-14: Ohayo, sensei! Watch for potential large timestamp handling.
Currently, timestamps are cast toi64and may overflow for values beyond year 2038 (on 32-bit systems). If you'd like to handle extremely large timestamps gracefully, consider returning aResultinstead of panicking.
16-18: Ohayo, sensei! These functions look good for converting timestamps to RFC 3339 strings.
Kudos on implementing a straightforward conversion flow.
127-156: Ohayo, sensei! Test coverage is well structured.
You've tested both valid and overflow-ish timestamps, and also validated the string output. This is thorough. Great job!crates/torii/sqlite/src/mod.rs (1)
36-39: Ohayo, sensei! Nice modular grouping.
Adding dedicated modules (constants,executor,types,simple_broker) helps isolate responsibilities, boosting clarity and maintainability.crates/torii/cli/Cargo.toml (1)
16-17: Ohayo, sensei! Confirm the version references to maintain consistency.
These new workspace references replacetorii-corewithtorii-engineandtorii-sqlite. Ensure matching versions across the entire workspace to avoid unexpected dependency resolution issues.crates/torii/server/Cargo.toml (1)
27-27: Ohayo sensei! The dependency change looks good!The replacement of
torii-corewithtorii-sqlitealigns well with the project's modularization goals.crates/torii/sqlite/Cargo.toml (1)
2-2: Ohayo! Package metadata changes look perfect!The renaming from
torii-coretotorii-sqliteand the updated description accurately reflect the new modular structure.Also applies to: 5-5
crates/torii/graphql/Cargo.toml (2)
31-31: Ohayo! The torii-sqlite dependency change looks good!The replacement of
torii-corewithtorii-sqliteis consistent with the project's modularization strategy.
Line range hint
1-53: Verify the impact of torii-core removalLet's verify that all references to torii-core have been properly updated in the codebase.
✅ Verification successful
Ohayo sensei!
torii-corereferences still exist in Cargo.lock but not in dependenciesThe verification shows that while
torii-coreappears in Cargo.lock, there are no direct dependencies on it in any Cargo.toml files. This is expected because:
- Cargo.lock often retains historical entries even after dependencies are removed
- The absence of
torii-corein any Cargo.toml files confirms that it's no longer a dependency- The entries in Cargo.lock will be automatically cleaned up when the lock file is regenerated
No action is needed as this is the expected state after removing the
torii-corepackage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to torii-core that might have been missed # Search for torii-core in all files echo "Searching for remaining torii-core references..." rg -l "torii-core" || echo "No references found" # Check Cargo.lock for any remaining torii-core dependencies echo "Checking Cargo.lock for torii-core dependencies..." rg "torii-core" Cargo.lock || echo "No dependencies found"Length of output: 384
Script:
#!/bin/bash # Let's check the actual content of Cargo.lock to understand if these are just old entries echo "Checking the detailed entries in Cargo.lock..." rg -A 5 -B 5 '"torii-core"' Cargo.lock # Let's also check if there are any direct dependencies on torii-core echo "Checking for direct dependencies on torii-core in Cargo.toml files..." fd Cargo.toml --exec rg "torii-core" {}Length of output: 1448
crates/torii/grpc/Cargo.toml (1)
17-17: Ohayo! The dependency changes look good, sensei!The replacement of
torii-corewithtorii-sqliteas an optional dependency and the corresponding feature update are correctly configured. The wasm32 incompatibility note is preserved appropriately.Let's verify that no other features are referencing the old
torii-core:Also applies to: 64-64
✅ Verification successful
Ohayo! All clear on the dependency front, sensei!
The verification shows that
torii-corereferences inCargo.lockare expected as it tracks the dependency tree, but there are no feature declarations usingtorii-corein anyCargo.tomlfiles. The change fromtorii-coretotorii-sqlitein the server feature is complete and consistent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to torii-core in Cargo.toml files rg -l "torii-core" --type tomlLength of output: 41
Script:
#!/bin/bash # Let's check the actual content of references to torii-core in Cargo.lock rg "torii-core" Cargo.lock -C 2Length of output: 595
Script:
#!/bin/bash # Let's also check if there are any references to torii-core in feature declarations across all Cargo.toml files rg "torii-core" --type toml -g "Cargo.toml" -C 3Length of output: 46
Script:
#!/bin/bash # Let's check all Cargo.toml files for any feature declarations fd Cargo.toml -x cat {} | grep -A 5 "\[features\]"Length of output: 3149
bin/torii/Cargo.toml (1)
39-40: The new dependencies are properly configured, sensei!The addition of both
torii-engineandtorii-sqliteas workspace dependencies aligns well with the core splitting objective. The default features already include 'sqlite' which is good.Let's verify the workspace configuration for these new dependencies:
Cargo.toml (1)
122-123: Ohayo sensei! The modular approach looks promising!The split of
torii-coreintotorii-engineandtorii-sqlitefollows good architectural practices by separating concerns between core engine logic and storage implementation.Let's verify the dependency structure with this script:
✅ Verification successful
Ohayo sensei! The modular split is clean and well-executed! 🎯
The verification shows a proper separation of concerns:
torii-corehas been completely removed with no lingering references- New modules are properly set up in
crates/torii/engineandcrates/torii/sqlitetorii-enginedepends ontorii-sqliteas expected for storage operations- Both modules have their own well-defined Cargo.toml with appropriate dependencies
The refactoring appears to be complete and consistent across the codebase. The modular structure will indeed improve maintainability and separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new modular structure and dependencies # Check if the old torii-core directory still exists fd -t d "torii-core$" crates/torii # Verify the existence of new module directories echo "Checking new module directories..." fd -t d "^(engine|sqlite)$" crates/torii # Check for any remaining references to torii-core echo "Checking for remaining torii-core references..." rg "torii-core" crates/toriiLength of output: 334
Script:
#!/bin/bash # Let's verify the dependency structure more thoroughly # Check Cargo.toml files in the new modules echo "=== Engine Cargo.toml ===" cat crates/torii/engine/Cargo.toml echo -e "\n=== SQLite Cargo.toml ===" cat crates/torii/sqlite/Cargo.toml # Check if any crate still depends on torii-core echo -e "\n=== Checking all Cargo.toml files for dependencies ===" fd Cargo.toml | xargs rg "torii-core"Length of output: 3036
crates/torii/engine/src/constants.rs
Outdated
| pub const IPFS_URL: &str = "https://ipfs.io/ipfs/"; | ||
| pub const IPFS_CLIENT_MAX_RETRY: u8 = 3; | ||
|
|
||
| pub const IPFS_CLIENT_URL: &str = "https://ipfs.infura.io:5001"; | ||
| pub const IPFS_CLIENT_USERNAME: &str = "2EBrzr7ZASQZKH32sl2xWauXPSA"; | ||
| pub const IPFS_CLIENT_PASSWORD: &str = "12290b883db9138a8ae3363b6739d220"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Be mindful of embedding credentials in source code.
Storing credentials like IPFS username and password in code poses a security risk if the repository is public. It's typically safer to load them from environment variables or config files at runtime.
Here's an example snippet for environment-based loading:
- pub const IPFS_CLIENT_USERNAME: &str = "2EBrzr7ZASQZKH32sl2xWauXPSA";
- pub const IPFS_CLIENT_PASSWORD: &str = "12290b883db9138a8ae3363b6739d220";
+ pub const IPFS_CLIENT_USERNAME: &str = env!("IPFS_CLIENT_USERNAME");
+ pub const IPFS_CLIENT_PASSWORD: &str = env!("IPFS_CLIENT_PASSWORD");Committable suggestion skipped: line range outside the PR's diff.
| @@ -35,7 +35,7 @@ tracing-subscriber.workspace = true | |||
| libp2p = { git = "https://github.com/libp2p/rust-libp2p", features = [ "dns", "ed25519", "gossipsub", "identify", "macros", "noise", "ping", "quic", "relay", "tcp", "tokio", "websocket", "yamux" ], rev = "cdc9638" } | |||
| libp2p-webrtc = { git = "https://github.com/libp2p/rust-libp2p", features = [ "pem", "tokio" ], rev = "cdc9638" } | |||
| sqlx.workspace = true | |||
| torii-core.workspace = true | |||
| torii-sqlite.workspace = true | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! There's a potential conflict with sqlite dependency in wasm32 target, sensei!
I noticed that torii-sqlite appears both in the regular dependencies and within the wasm32 target section. This could lead to compilation issues since SQLite might not be compatible with wasm32.
- Remove
torii-sqlite.workspace = truefrom the general dependencies section and ensure it's only included in the non-wasm32 target section.
Note: The current setup shows torii-sqlite being potentially available for wasm32 builds, which could cause problems since SQLite typically requires native system libraries that aren't available in the WebAssembly environment.
🔗 Analysis chain
The target-specific dependency looks good, sensei!
The addition of torii-sqlite as a workspace dependency for non-wasm32 targets is correctly configured.
Let's verify there are no conflicting dependencies for wasm32:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any sqlite-related dependencies in wasm32 target sections
rg "target.*wasm32.*dependencies" -A 10 --type toml | rg -A 10 "sqlite"
Length of output: 938
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
crates/torii/engine/src/processors/store_update_record.rs (1)
91-93: Ohayo sensei! Evaluate partial deserialization
Deserializing the event values might fail if new fields were added. Ensure you handle incremental schema evolution or mismatch gracefully.crates/torii/engine/src/processors/store_update_member.rs (3)
32-43: Ohayo sensei! Consider clarifying validation criteria
You only check forevent.keys.len() > 1to invalidate. Add clarifying comments or more checks if additional constraints exist.
71-84: Ohayo sensei! Member lookup could fail
You abort if a member is not found. Consider providing additional context or a more detailed error message if a member is missing.
102-111: Ohayo sensei! Revisit skipping entity creation
If the entity doesn’t exist, the update is skipped. This might be intended, but check if auto-creation is desirable to maintain consistency.crates/torii/sqlite/src/test.rs (2)
208-209: Ohayo sensei! Consider stabilizing this flaky test
It’s marked as ignored due to flakiness. If it’s essential, investigate timing or state dependency.
303-306: Ohayo sensei! Validate record deletion logic
Your TODO indicates that only zeroing occurs instead of actual deletion. Confirm which behavior is intended and update accordingly.crates/torii/sqlite/src/utils.rs (2)
17-21: Check conversion edge cases.
must_utc_datetime_from_timestamppanics when the timestamp cannot be converted. Consider returning aResultto handle erroneous timestamps more gracefully and avoid panics in production code.
53-77: Implement exponential backoff or varying retry intervals.
fetch_content_from_ipfsruns a fixed 3-second delay between retries. For improved resilience, consider exponential backoff if IPFS is slow or unavailable, sensei.
📜 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 (32)
crates/torii/core/src/processors/store_update_member.rs(1 hunks)crates/torii/core/src/processors/store_update_record.rs(1 hunks)crates/torii/core/src/sql/test.rs(1 hunks)crates/torii/engine/src/constants.rs(1 hunks)crates/torii/engine/src/engine.rs(1 hunks)crates/torii/engine/src/lib.rs(1 hunks)crates/torii/engine/src/processors/erc20_legacy_transfer.rs(1 hunks)crates/torii/engine/src/processors/erc20_transfer.rs(1 hunks)crates/torii/engine/src/processors/erc721_legacy_transfer.rs(1 hunks)crates/torii/engine/src/processors/erc721_transfer.rs(1 hunks)crates/torii/engine/src/processors/event_message.rs(1 hunks)crates/torii/engine/src/processors/metadata_update.rs(1 hunks)crates/torii/engine/src/processors/mod.rs(1 hunks)crates/torii/engine/src/processors/raw_event.rs(1 hunks)crates/torii/engine/src/processors/register_event.rs(1 hunks)crates/torii/engine/src/processors/register_model.rs(1 hunks)crates/torii/engine/src/processors/store_del_record.rs(1 hunks)crates/torii/engine/src/processors/store_set_record.rs(1 hunks)crates/torii/engine/src/processors/store_transaction.rs(1 hunks)crates/torii/engine/src/processors/store_update_member.rs(1 hunks)crates/torii/engine/src/processors/store_update_record.rs(1 hunks)crates/torii/engine/src/processors/upgrade_event.rs(1 hunks)crates/torii/engine/src/processors/upgrade_model.rs(1 hunks)crates/torii/sqlite/Cargo.toml(1 hunks)crates/torii/sqlite/src/cache.rs(1 hunks)crates/torii/sqlite/src/constants.rs(1 hunks)crates/torii/sqlite/src/erc.rs(1 hunks)crates/torii/sqlite/src/executor/erc.rs(1 hunks)crates/torii/sqlite/src/executor/mod.rs(1 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/test.rs(1 hunks)crates/torii/sqlite/src/utils.rs(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/torii/sqlite/src/cache.rs
- crates/torii/sqlite/src/executor/erc.rs
- crates/torii/sqlite/src/executor/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/torii/engine/src/constants.rs
- crates/torii/sqlite/Cargo.toml
- crates/torii/engine/src/lib.rs
- crates/torii/sqlite/src/constants.rs
- crates/torii/engine/src/engine.rs
🔇 Additional comments (30)
crates/torii/engine/src/processors/store_update_record.rs (2)
58-69: Ohayo sensei! Confirm the silent ignore behavior
You skip processing if the model does not exist. Ensure this is intentional and won't mask potential configuration issues.
81-89: Ohayo sensei! Validate struct schema assumption
You assume every model here is a struct. Returning an error early is fine, but consider logging an explicit warning if this scenario arises often or indicates misconfiguration.crates/torii/engine/src/processors/store_update_member.rs (1)
59-70: Ohayo sensei! Confirm ignoring missing models
Like in store_update_record, silently skipping if the model is missing might obscure problems in configuration or indexing.crates/torii/sqlite/src/test.rs (1)
60-62: Ohayo sensei! Great multi-threaded tests
Using the multi-thread flavor is a good choice for concurrency checks. Keep confirming that your tests remain reliable under parallel conditions.crates/torii/engine/src/processors/store_transaction.rs (1)
7-7: Ohayo sensei! Good import refactor
Switching totorii_sqlite::Sqlstreamlines the code. No further issues spotted.crates/torii/engine/src/processors/raw_event.rs (1)
8-8: Ohayo sensei! Consistent import usage
Importingtorii_sqlite::Sqlhere maintains uniformity across processors. Looks good!crates/torii/engine/src/processors/erc20_transfer.rs (1)
10-10: Ohayo sensei!
Import reference updated
Usingtorii_sqlite::Sqlaligns with the new modular structure. Everything looks good, and the transitional import approach is consistent across the codebase.crates/torii/engine/src/processors/erc721_transfer.rs (1)
10-10: Ohayo sensei!
Consistent module usage
The switch fromcrate::sql::Sqltotorii_sqlite::Sqlis logical and ensures standardized handling of SQLite references. Nicely done!crates/torii/engine/src/processors/erc721_legacy_transfer.rs (1)
10-10: Ohayo sensei!
Seamless transition to new module
Great to see everything being consolidated undertorii_sqlite. This maintains coherent code organization.crates/torii/engine/src/processors/erc20_legacy_transfer.rs (1)
10-10: Ohayo sensei!
Harmonizing imports
The import fromtorii_sqlite::Sqlneatly fits into the refactoring. No issues spotted. Good job!crates/torii/engine/src/processors/event_message.rs (1)
11-11: Ohayo sensei! Smooth import transition here.Switching from
crate::sql::Sqltotorii_sqlite::Sqlcentralizes SQL interactions in the new module, helping keep the codebase consistent and efficient.crates/torii/engine/src/processors/mod.rs (1)
9-9: Ohayo sensei!Continuing the migration to
torii_sqlite::Sqlfosters better modularity by extracting database operations into a dedicated crate.crates/torii/engine/src/processors/store_del_record.rs (1)
10-10: Ohayo sensei! Consistent migration observed.Updating the import to
torii_sqlite::Sqlaligns with the revised architecture and keeps the code unified.crates/torii/engine/src/processors/store_set_record.rs (1)
10-11: Ohayo sensei! SQL modernization on point.Both updates to
torii_sqlite::utils::felts_to_sql_stringandtorii_sqlite::Sqlstrengthen the code structure by consolidating database utilities undertorii_sqlite.crates/torii/core/src/processors/store_update_record.rs (1)
11-11: Ohayo sensei, this import aligns well with the new module structure!
Switching totorii_sqlite::Sqlkeeps things consistent with your overall refactor. Nicely done.crates/torii/engine/src/processors/register_model.rs (1)
11-11: Ohayo sensei, the updated import is a smooth transition to the new SQLite-based module.
Good job adapting your database calls to the newtorii_sqlite::Sqlimport.crates/torii/engine/src/processors/register_event.rs (1)
11-11: Ohayo sensei, I see you've switched totorii_sqlite::Sql.
This change is perfectly consistent with your broader refactor efforts.crates/torii/core/src/processors/store_update_member.rs (1)
14-14: Ohayo sensei, swapping totorii_sqlite::Sqlcompletes the shift toward your new database layer!
This uniform import approach helps keep your codebase clean and maintainable.crates/torii/engine/src/processors/upgrade_model.rs (1)
11-11: Ohayo, sensei! Smooth transition totorii_sqlite::Sql.There are no functional changes here, and referencing
torii_sqlite::Sqllooks consistent with the new modular structure.crates/torii/engine/src/processors/upgrade_event.rs (1)
11-11: Ohayo, sensei! Import path realigned.Everything seems in order with the import switched to
torii_sqlite::Sql.crates/torii/engine/src/processors/metadata_update.rs (1)
15-17: Ohayo, sensei! Synchronized imports forconstants,Sql, andutils.All imports align well with the
torii_sqlitemodule. No functional discrepancies detected.crates/torii/sqlite/src/erc.rs (1)
17-17: Ohayo, sensei! Refactored utility import path for SQL string conversions.Confirm that all references to
felt_and_u256_to_sql_string,felt_to_sql_string, andfelts_to_sql_stringstill resolve correctly in the updated location. Otherwise, everything is in good shape!crates/torii/core/src/sql/test.rs (1)
27-27: Ohayo sensei! The updated import forward seems correct.Replacing
crate::sql::Sqlwithtorii_sqlite::Sqlimproves clarity by reflecting the new module organization.crates/torii/sqlite/src/lib.rs (2)
36-40: Ohayo sensei! Splitting out these modules enhances modularity.Exporting
constants,executor,types,simple_broker, anderroras separate modules is a great step toward keeping concerns well-separated.
177-177: Ohayo sensei! Changing visibility topubbroadens the usage scope.Making the
cursorsfunction public allows top-level consumers of this crate to manage cursors directly. Verify that this aligns with your intended architecture and security requirements.crates/torii/sqlite/src/utils.rs (5)
4-5: Ohayo sensei! These imports look good.Importing additional libraries (
Duration,DateTime,TryStreamExt, etc.) is aligned with the new functionality introduced for timing, date handling, and IPFS streaming.Also applies to: 6-7, 8-10, 11-12, 13-14, 15-16
23-25: Concise helper for date-time strings.
utc_dt_string_from_timestampis straightforward and clarifies time handling. Well done, sensei!
160-173: Ohayo sensei! Great test coverage for timestamp conversion.Tests confirm that valid timestamps produce expected results, and incorrect timestamps lead to panic. Nicely done.
175-180: Ensure consistent error handling.This test checks the panic condition for out-of-range timestamps. Consider returning an error in production instead of panicking, if you want to prevent abrupt termination.
182-189: Proper verification of time format.The test reaffirms correctness of RFC 3339 outputs, sensei. No concerns here.
| let event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | ||
| panic!( | ||
| "Expected {} event to be well formed.", | ||
| <StoreUpdateRecordProcessor as EventProcessor<P>>::event_key(self) | ||
| ) | ||
| }) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Avoid using panics for event conversion
Panicking on malformed events can cause runtime failures. Consider returning a meaningful error instead to maintain graceful error handling.
- let event = match WorldEvent::try_from(event).unwrap_or_else(|_| {
- panic!(
- "Expected {} event to be well formed.",
- <StoreUpdateRecordProcessor as EventProcessor<P>>::event_key(self)
- )
- }) {
+ let event = match WorldEvent::try_from(event) {
+ Ok(e) => e,
+ Err(_) => {
+ return Err(anyhow::anyhow!(
+ "Malformed {} event",
+ <StoreUpdateRecordProcessor as EventProcessor<P>>::event_key(self)
+ ));
+ }
+ };📝 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 event = match WorldEvent::try_from(event).unwrap_or_else(|_| { | |
| panic!( | |
| "Expected {} event to be well formed.", | |
| <StoreUpdateRecordProcessor as EventProcessor<P>>::event_key(self) | |
| ) | |
| }) { | |
| let event = match WorldEvent::try_from(event) { | |
| Ok(e) => e, | |
| Err(_) => { | |
| return Err(anyhow::anyhow!( | |
| "Malformed {} event", | |
| <StoreUpdateRecordProcessor as EventProcessor<P>>::event_key(self) | |
| )); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
crates/torii/indexer/Cargo.toml (2)
15-15: Consider using workspace versions for consistencySeveral dependencies specify explicit versions while most others use workspace versions:
bitflags = "2.6.0"futures-channel = "0.3.0"slab = "0.4.2"tokio = { version = "1.32.0", ... }Consider moving these version specifications to the workspace root for better maintainability and version consistency across the project.
Also applies to: 23-23, 31-31, 36-36
37-37: Remove commented dependencyThe commented
tokio-streamdependency should either be:
- Removed if it's no longer needed
- Uncommented if it's required
- Documented with a comment explaining why it's kept but commented out
crates/torii/sqlite/src/test.rs (2)
60-113: Ohayo sensei, consider adding checkpoints in the test.
Thetest_load_from_remotefunction covers multiple transactions and database assertions in a single block. Splitting it into smaller, targeted tests or adding intermediate asserts can improve clarity and maintainability.
311-391: Ohayo sensei, good structure but consider verifying intermediate states.
test_update_with_set_recordsets up multiple transactions. Verifying partial states after each transaction can help diagnose potential synchronization issues.crates/torii/indexer/src/processors/store_update_record.rs (2)
13-14: Consider updating the log target name.
TheLOG_TARGETreferencestorii_core, but this file is withintorii/indexer. For clarity, we might rename it if there's no direct association withtorii_core.
18-29: Event key and validation.
Ohayo sensei! Theevent_keyuses a static string"StoreUpdateRecord", andvalidatealways returnstrue. You might want to refinevalidateto ensure the event data structure is correct before proceeding.crates/torii/indexer/src/processors/store_update_member.rs (2)
16-16: Log target naming.
We see the log target referencestorii_core::processors::store_update_member. For consistency, consider renaming it to reflect its location intorii/indexer.
32-43: Validation logic.
We only check ifevent.keys.len() > 1, logging an info message if invalid. Consider returning more descriptive logs or tightening the constraints if needed.
📜 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 (30)
Cargo.toml(1 hunks)bin/torii/Cargo.toml(1 hunks)crates/torii/cli/Cargo.toml(1 hunks)crates/torii/grpc/src/server/mod.rs(1 hunks)crates/torii/indexer/Cargo.toml(1 hunks)crates/torii/indexer/src/constants.rs(1 hunks)crates/torii/indexer/src/engine.rs(1 hunks)crates/torii/indexer/src/lib.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)crates/torii/indexer/src/processors/event_message.rs(1 hunks)crates/torii/indexer/src/processors/metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/indexer/src/processors/raw_event.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(1 hunks)crates/torii/indexer/src/processors/register_model.rs(1 hunks)crates/torii/indexer/src/processors/store_del_record.rs(1 hunks)crates/torii/indexer/src/processors/store_set_record.rs(1 hunks)crates/torii/indexer/src/processors/store_transaction.rs(1 hunks)crates/torii/indexer/src/processors/store_update_member.rs(1 hunks)crates/torii/indexer/src/processors/store_update_record.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(1 hunks)crates/torii/libp2p/src/server/mod.rs(1 hunks)crates/torii/libp2p/src/tests.rs(1 hunks)crates/torii/sqlite/Cargo.toml(2 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/test.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/torii/indexer/src/constants.rs
- crates/torii/indexer/src/lib.rs
- crates/torii/indexer/src/engine.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- bin/torii/Cargo.toml
- crates/torii/cli/Cargo.toml
- Cargo.toml
- crates/torii/sqlite/Cargo.toml
- crates/torii/sqlite/src/lib.rs
🔇 Additional comments (35)
crates/torii/indexer/Cargo.toml (3)
1-8: Ohayo! Package metadata looks clean, sensei!The package metadata is well-structured and properly inherits workspace configurations.
43-49: Dev dependencies look appropriateThe development dependencies align well with testing needs, including utilities for testing, running Katana, and handling temporary files.
36-36: Verify tokio featuresThe tokio dependency enables specific features while keeping default-features. Let's verify if all required features are explicitly listed and if default-features are actually needed.
✅ Verification successful
Ohayo sensei! After analyzing the tokio usage in the codebase, I can provide a clear response about the features configuration.
syncandmacrosfeatures are sufficient, no need for default-featuresThe indexer crate only uses the following tokio features:
syncfor Semaphore, broadcast/mpsc channelsmacrosfor async codetimefunctionality (which is pulled in bymacros)The current configuration enables
default-featuresunnecessarily. Here are the specific usages found:
- Synchronization primitives:
tokio::sync::{broadcast, mpsc, Semaphore}- Task management:
tokio::spawn,tokio::task::JoinSet- Time utilities:
tokio::time::{sleep, Instant}- Async control flow:
tokio::select!macroYou can optimize the dependency by updating it to:
tokio = { version = "1.32.0", features = ["macros", "sync"], default-features = false }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tokio usage to verify required features rg -t rust 'tokio::|#\[tokio::' # Check workspace for other tokio configurations rg 'tokio.*=.*{.*}' -g 'Cargo.toml'Length of output: 23754
Script:
#!/bin/bash # Let's check tokio configurations across all Cargo.toml files fd Cargo.toml --exec cat {} | rg -A5 'tokio.*=' # Let's also check for tokio usage patterns in the indexer crate rg -t rust 'tokio::|#\[tokio::' crates/torii/indexer/Length of output: 7721
crates/torii/grpc/src/server/mod.rs (1)
45-48: Ohayo sensei! The refactor totorii_sqliteimports looks seamless and aligns with the code changes throughout the file.
Nicely done in ensuring proper consistency with the newly introduced modules.crates/torii/sqlite/src/test.rs (3)
1-29: Ohayo sensei, clean imports!
These imports exhibit a well-organized structure for testing SQLite-based functionality. Nice approach for ensuring we have all the necessary crates at the top level.
208-309: Ohayo sensei, investigate the test flakiness.
The#[ignore]annotation indicates an unreliable test (test_load_from_remote_del). Adding more detailed logs or systematically stepping through each transaction can help identify timing or concurrency issues.
393-406: Ohayo sensei, handy utility for row counts.
count_tableis a neat helper to confirm database states. If you anticipate large tables, ensure the table name is properly sanitized to avoid potential SQL injection.crates/torii/libp2p/src/server/mod.rs (1)
28-30: Ohayo sensei, smooth transition to torii_sqlite.
These import changes reflect the refactoring to the new SQLite-based module. The references look correct and consistent with the rest of the code.crates/torii/libp2p/src/tests.rs (1)
542-545: Ohayo sensei, consistent module referencing.
Switching from the oldtorii_corereferences totorii_sqliteis coherent with the project’s restructuring. This should streamline database operations in tests.crates/torii/indexer/src/processors/store_update_record.rs (6)
1-9: Ohayo sensei! Great set of imports.
Everything here looks consistent and straightforward.
15-17: Empty struct usage.
The struct is empty, derivingDefaultandDebug. This is fine if no fields are needed. If you foresee the need for configuration or state, consider adding them here.
55-71: Handling missing models.
The substring check for"no rows"is a bit fragile. Consider using a dedicated error variant for missing rows to avoid potential false matches or locale issues.
73-89: Struct key handling.
Ohayo sensei! The logic for removing key fields is straightforward. Ensure that future expansions of the entity schema do not inadvertently require key fields in subsequent updates.
91-96: Entity update.
This call todb.set_entityis well-structured. Confirm you handle errors gracefully if the entity insertion for large data fails.
31-53: 🛠️ Refactor suggestionPanic on invalid event.
Currently, the code panics if the event is not well-formed or not aStoreUpdateRecord. Consider returning an error or skipping to avoid crashing.crates/torii/indexer/src/processors/store_update_member.rs (5)
1-14: Ohayo sensei!
The import statements align well with the updatedtorii_sqlite::Sql. No immediate issues.
18-18: Index constant.
DefiningMEMBER_INDEX = 2is straightforward. Ensure that future expansions to the event data structure maintain this offset.
20-21: Empty struct definition.
This struct doesn't store any fields, which is fine if no configuration or state is needed.
28-30: Event key.
We store a static event key"StoreUpdateMember". This matches the naming style of other processors.
45-118: Store update member logic.
Ohayo sensei! The approach to retrieve model data, ignore missing models, check if the entity exists, and then update the specific member looks well-structured. Usage ofcontext("member not found")is helpful for debugging.crates/torii/indexer/src/processors/store_transaction.rs (1)
7-7: Ohayo sensei!
Switched totorii_sqlite::Sql. This consistent approach across the codebase is good for maintainability.crates/torii/indexer/src/processors/raw_event.rs (1)
8-8: Ohayo sensei!
Import path updated totorii_sqlite::Sql. Looks consistent with the rest of the refactor.crates/torii/indexer/src/processors/erc20_transfer.rs (1)
10-10: Ohayo sensei!
Switched import fromcrate::sql::Sqltotorii_sqlite::Sql. The rename is consistent with other changes.crates/torii/indexer/src/processors/erc721_transfer.rs (1)
10-10: Ohayo sensei, the import update totorii_sqlite::Sqllooks great!This refactor consistently aligns with the new
torii_sqlitemodule. No additional issues found here.crates/torii/indexer/src/processors/erc721_legacy_transfer.rs (1)
10-10: Ohayo sensei, the shift totorii_sqlite::Sqlis seamless.Integrating the new SQLite module maintains the existing logic and promotes consistent database handling.
crates/torii/indexer/src/processors/erc20_legacy_transfer.rs (1)
10-10: Ohayo sensei, the import totorii_sqlite::Sqlis correct!This aligns well with the broader transition toward the new SQLite-backed module. Everything else remains unchanged.
crates/torii/indexer/src/processors/event_message.rs (1)
11-11: Ohayo sensei, switching totorii_sqlite::Sqlis well-implemented.The database interactions and logic remain intact while adopting the new module. Nicely done!
crates/torii/indexer/src/processors/mod.rs (1)
9-9: Ohayo sensei! This updated import aligns nicely with the newtorii_sqlitemodule.
Overall, the refactor is consistent and minimal disruptions are expected for event processing. Great job!crates/torii/indexer/src/processors/store_del_record.rs (1)
10-10: Ohayo sensei! This import update ensures database operations now go through thetorii_sqlitemodule.
No immediate issues spotted; the existing logic can seamlessly tap into the updated SQL handling. Great work, sensei!crates/torii/indexer/src/processors/store_set_record.rs (1)
10-11: Ohayo sensei! Switching totorii_sqlitefunctions keeps the code consistent with the new module structure.
Usingfelts_to_sql_stringfrom the same module also ensures everything stays nicely packaged. Keep it up!crates/torii/indexer/src/processors/register_model.rs (1)
11-11: Ohayo sensei! Transitioning to theSqltype fromtorii_sqliteis an essential change.
The remainder of this file’s logic remains stable, so no concerns here. Smooth sailing ahead!crates/torii/indexer/src/processors/register_event.rs (1)
11-11: Ohayo sensei! Smooth transition totorii_sqlite.
No functional changes or apparent issues. This refactor cleanly points to the new module for database interactions.crates/torii/indexer/src/processors/upgrade_model.rs (1)
11-11: Ohayo sensei! Good alignment with the newtorii_sqliteimport.
This change unifies DB logic under thetorii_sqlitemodule, which is consistent with the refactor goals.crates/torii/indexer/src/processors/upgrade_event.rs (1)
11-11: Ohayo sensei! Approved migration totorii_sqlite.
Ensures consistency across the event processors with a centralized SQL interface.crates/torii/indexer/src/processors/metadata_update.rs (1)
15-17: Ohayo sensei! Nice consolidation of constants, SQL, and utility imports.
Switching fromcratetotorii_sqlitereferences streamlines the module structure and fosters a unified database approach.
crates/torii/sqlite/src/test.rs
Outdated
| pub async fn bootstrap_engine<P>( | ||
| world: WorldContractReader<P>, | ||
| db: Sql, | ||
| provider: P, | ||
| ) -> Result<Engine<P>, Box<dyn std::error::Error>> | ||
| where | ||
| P: Provider + Send + Sync + core::fmt::Debug + Clone + 'static, | ||
| { | ||
| let (shutdown_tx, _) = broadcast::channel(1); | ||
| let to = provider.block_hash_and_number().await?.block_number; | ||
| let world_address = world.address; | ||
| let mut engine = Engine::new( | ||
| world, | ||
| db.clone(), | ||
| provider, | ||
| Processors { ..Processors::default() }, | ||
| EngineConfig::default(), | ||
| shutdown_tx, | ||
| None, | ||
| &[Contract { address: world_address, r#type: ContractType::WORLD }], | ||
| ); | ||
|
|
||
| let data = engine.fetch_range(0, to, &HashMap::new()).await.unwrap(); | ||
| engine.process_range(data).await.unwrap(); | ||
|
|
||
| db.execute().await.unwrap(); | ||
|
|
||
| Ok(engine) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, consider handling errors more gracefully.
Multiple .unwrap() calls in bootstrap_engine() might cause undesirable panics if any step fails. It would be beneficial to return or propagate descriptive error messages to help with debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/test.rs (3)
30-58: Ohayo sensei! Consider more graceful error handling inbootstrap_engine.Currently, lines 52-53 unwrap the results of
fetch_rangeandprocess_range. Usingunwrap()will panic on any error and may disrupt the entire test suite. Propagating or handling these errors gracefully improves resilience and debuggability.
300-303: Ohayo sensei! Clarify the deletion logic.The TODO at line 302 states that the record is only zeroed out instead of fully removed. It might be helpful to either update the model’s logic to truly remove rows or add comments clarifying why zeroing fields is the desired behavior. This ensures test expectations align with implementation details.
393-406: Ohayo sensei! Guard against potential injection incount_table.The function uses string interpolation to build a query from
table_name. Iftable_nameoriginates from untrusted sources, this could allow injection. Since it’s presumably internal usage in a test context, it may be safe, but adding validation or parameterization is a recommended precaution for production code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/indexer/Cargo.toml(1 hunks)crates/torii/indexer/src/lib.rs(1 hunks)crates/torii/indexer/src/test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/indexer/Cargo.toml
🔇 Additional comments (6)
crates/torii/indexer/src/lib.rs (3)
1-3: Ohayo, sensei! Great modular structure.
These new modules help keep the code neat and organized, enabling each concern to be tackled individually and making the codebase more maintainable overall.
4-4: Ohayo, sensei! Good call making theenginemodule public.
This change helps modules outside this file extend and reuseenginefunctionality, ensuring a more flexible architecture.
6-6: Ohayo, sensei! Re-exportingEngineis a slick move.
By re-exportingEngine, it’s simpler for others to integrate and build upon your indexer logic, reducing duplication and streamlining imports.crates/torii/indexer/src/test.rs (3)
39-40: Ohayo sensei! Verify theblock_numberusage.This line retrieves the latest
block_numberbut does not guard against the possibility of it beingNone. Although unlikely, consider fallback logic or error handling if the provider cannot return a validblock_number.
60-61: Ohayo sensei! Good use of multi-threaded async tests.The usage of
#[tokio::test(flavor = "multi_thread")]helps accelerate your test suite. This is a nice approach for concurrency in your integration tests.
208-209: Ohayo sensei! Investigate the flaky test further.This test is marked
#[ignore]due to flakiness. Intermittent failures may indicate timing or concurrency issues that are masked by local sequencing. Running these tests in a controlled environment or mocking external dependencies could help isolate and resolve the cause.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
crates/torii/sqlite/src/lib.rs (2)
35-35: Model module fosters tidy data abstractions.
Great approach to data modeling. Keep an eye on potential duplications if future expansions add redundant interfaces.
36-36: Simple broker module: watch for performance constraints.
As this module grows, consider whether message passing or concurrency patterns could be a bottleneck, and plan accordingly.crates/torii/sqlite/src/utils.rs (3)
20-24: Ohayo sensei, consider making the panic scenario more explicit or returning a properResult.Right now, the
must_utc_datetime_from_timestampfunction panics when conversion fails. A more flexible approach might be returning aResult<DateTime<Utc>>so upstream callers can handle errors gracefully without crashing the application.
26-28: Ohayo sensei, confirm if you need fractional seconds in the RFC3339 output.By default,
to_rfc3339()includes sub-second precision if present. If it’s not needed, this is fine. Otherwise, explicitly format or strip it to keep the output consistent throughout your codebase.
56-75: Ohayo sensei, consider logging failures at warning or error level rather than info.The loop currently uses
info!to log errors. Repeated warnings might be more appropriate when data-fetch attempts fail, helping highlight potential runtime problems.crates/torii/indexer/src/processors/store_update_member.rs (1)
55-68: Ohayo sensei, consider logging model-not-found scenario more explicitly.
Silently ignoring the missing model might cause debugging challenges. Logging a warning or an info message could help diagnose indexing constraints.crates/torii/indexer/src/test.rs (2)
30-59: Ohayo sensei, improve error reporting inbootstrap_engine.
Currently, errors fromfetch_rangeorprocess_rangeare unwrapped, which might hide the root cause. Consider handling them gracefully or adding logs for better observability.
208-309: Ohayo sensei, address the TODO about chronological re-sync testing.
There's a comment suggesting there's a flaky issue with partial states. Propose adding intermediate block checks or partial verifications to validate the re-sync logic.Do you want me to create a new test approach or open an issue to explore a robust chronological re-sync test?
📜 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 (57)
bin/torii/src/main.rs(1 hunks)crates/torii/cli/Cargo.toml(1 hunks)crates/torii/cli/src/args.rs(1 hunks)crates/torii/cli/src/options.rs(1 hunks)crates/torii/graphql/Cargo.toml(1 hunks)crates/torii/graphql/src/object/entity.rs(1 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(1 hunks)crates/torii/graphql/src/object/erc/token_transfer.rs(1 hunks)crates/torii/graphql/src/object/event.rs(1 hunks)crates/torii/graphql/src/object/event_message.rs(1 hunks)crates/torii/graphql/src/object/model.rs(1 hunks)crates/torii/graphql/src/query/data.rs(1 hunks)crates/torii/graphql/src/query/mod.rs(1 hunks)crates/torii/graphql/src/schema.rs(1 hunks)crates/torii/graphql/src/tests/metadata_test.rs(1 hunks)crates/torii/graphql/src/tests/mod.rs(1 hunks)crates/torii/graphql/src/tests/subscription_test.rs(1 hunks)crates/torii/grpc/Cargo.toml(3 hunks)crates/torii/grpc/src/server/mod.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/entity.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/error.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/event.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/event_message.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/indexer.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/model_diff.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/token_balance.rs(1 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)crates/torii/indexer/src/constants.rs(1 hunks)crates/torii/indexer/src/engine.rs(1 hunks)crates/torii/indexer/src/lib.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)crates/torii/indexer/src/processors/event_message.rs(1 hunks)crates/torii/indexer/src/processors/metadata_update.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/indexer/src/processors/raw_event.rs(1 hunks)crates/torii/indexer/src/processors/register_event.rs(1 hunks)crates/torii/indexer/src/processors/register_model.rs(1 hunks)crates/torii/indexer/src/processors/store_del_record.rs(1 hunks)crates/torii/indexer/src/processors/store_set_record.rs(1 hunks)crates/torii/indexer/src/processors/store_transaction.rs(1 hunks)crates/torii/indexer/src/processors/store_update_member.rs(1 hunks)crates/torii/indexer/src/processors/store_update_record.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_event.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(1 hunks)crates/torii/indexer/src/test.rs(1 hunks)crates/torii/libp2p/src/tests.rs(1 hunks)crates/torii/server/src/artifacts.rs(1 hunks)crates/torii/sqlite/src/cache.rs(1 hunks)crates/torii/sqlite/src/constants.rs(1 hunks)crates/torii/sqlite/src/erc.rs(1 hunks)crates/torii/sqlite/src/executor/erc.rs(1 hunks)crates/torii/sqlite/src/executor/mod.rs(1 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/utils.rs(3 hunks)
✅ Files skipped from review due to trivial changes (8)
- crates/torii/graphql/src/query/data.rs
- crates/torii/cli/src/args.rs
- crates/torii/grpc/src/server/subscriptions/model_diff.rs
- crates/torii/graphql/src/query/mod.rs
- crates/torii/grpc/src/server/subscriptions/indexer.rs
- crates/torii/graphql/src/object/event.rs
- crates/torii/graphql/src/object/erc/token_balance.rs
- crates/torii/graphql/src/schema.rs
🚧 Files skipped from review as they are similar to previous changes (27)
- crates/torii/indexer/src/constants.rs
- crates/torii/indexer/src/processors/mod.rs
- crates/torii/indexer/src/processors/upgrade_event.rs
- crates/torii/indexer/src/processors/register_model.rs
- crates/torii/indexer/src/processors/store_transaction.rs
- crates/torii/indexer/src/processors/erc721_legacy_transfer.rs
- crates/torii/indexer/src/processors/raw_event.rs
- crates/torii/indexer/src/processors/event_message.rs
- crates/torii/indexer/src/processors/erc20_transfer.rs
- crates/torii/indexer/src/processors/store_del_record.rs
- crates/torii/indexer/src/processors/erc20_legacy_transfer.rs
- crates/torii/indexer/src/processors/metadata_update.rs
- crates/torii/indexer/src/processors/erc721_transfer.rs
- crates/torii/cli/Cargo.toml
- crates/torii/sqlite/src/executor/erc.rs
- crates/torii/indexer/src/processors/register_event.rs
- crates/torii/indexer/src/processors/store_set_record.rs
- crates/torii/indexer/src/lib.rs
- crates/torii/sqlite/src/cache.rs
- crates/torii/sqlite/src/constants.rs
- crates/torii/indexer/src/processors/store_update_record.rs
- crates/torii/sqlite/src/executor/mod.rs
- crates/torii/indexer/src/processors/upgrade_model.rs
- crates/torii/indexer/src/engine.rs
- crates/torii/grpc/src/server/mod.rs
- crates/torii/sqlite/src/erc.rs
- crates/torii/libp2p/src/tests.rs
🔇 Additional comments (43)
crates/torii/grpc/src/server/subscriptions/error.rs (1)
2-2: Ohayo sensei! The updated import path is consistent with the refactoring.Switching from
torii_coretotorii_sqlitealigns with the new architecture direction. This ensures that future expansions or changes intorii_sqliteare cleanly integrated with error parsing here. Solid work!crates/torii/sqlite/src/lib.rs (5)
31-31: Ohayo sensei! Splendid grouping of constant values.
By placing constants in a dedicated module, you ensure better code organization and clarity. Keep it up!
33-33: Centralized error module is a wise choice.
Having a dedicated error module promotes reusability and simplifies debugging by keeping error types in one place.
34-34: Executor module: verify concurrency aspects.
Ensure that any concurrency mechanisms or multi-threaded code in this executor module properly handle shared state.
37-37: Useful segregation of custom types.
Defining your domain-specific types in their own module helps reduce confusion. Nicely done, sensei!
175-175: Publiccursorsmethod might need permission checks.
Changing frompub(crate)topubbroadens use outside this crate. Consider verifying that outside modules won't accidentally misuse it.crates/torii/sqlite/src/utils.rs (2)
56-75: Ohayo sensei, nicely implemented retry logic.You’ve provided a robust mechanism to keep trying until success or until retries are exhausted. This approach is beneficial for resilience in distributed environments.
163-192: Ohayo sensei, good test coverage for invalid timestamps.Verifying the panic scenario helps ensure future contributors understand the function’s safety constraints and that test coverage remains robust.
crates/torii/indexer/src/processors/store_update_member.rs (1)
33-41: Ohayo sensei, verify the single-key assumption.
This logic rejects events with more than one key. Ensure that the system truly needs only one key. Otherwise, valid multi-key events might be erroneously dropped.crates/torii/graphql/src/object/model.rs (1)
7-8: Ohayo sensei, the transition totorii_sqliteis consistent.
No immediate concerns about the updated imports. Implementation remains clear.crates/torii/grpc/src/server/subscriptions/event.rs (1)
16-19: Ohayo sensei, module import updates appear stable.
The shift totorii_sqlitealigns with the overall refactoring. The rest of the event flow remains logically sound.crates/torii/grpc/src/server/tests/entities_test.rs (1)
25-29: Ohayo sensei, the new import paths look good.
Shifting fromtorii_coretotorii_indexerandtorii_sqliteis consistent, and no issues are apparent in how these dependencies are used.crates/torii/graphql/src/tests/metadata_test.rs (1)
11-14: Ohayo sensei, these imports look solid!
By switching fromtorii_coretotorii_sqlite, we align the test suite with the refactored module structure. This approach should simplify maintenance and clarify dependencies.crates/torii/graphql/src/object/entity.rs (1)
10-11: Ohayo sensei, the entity imports have been updated cleanly!
The newSimpleBrokerandEntityimports fromtorii_sqlitemaintain existing logic while aligning to the updated architecture.crates/torii/grpc/src/server/subscriptions/token_balance.rs (1)
15-17: Ohayo sensei, the error and type imports look good!
Thanks for moving these definitions fromtorii_coretotorii_sqlite. This ensures consistency with the rest of the updated codebase.crates/torii/grpc/src/server/subscriptions/event_message.rs (1)
16-19: Ohayo sensei, these new constants and type imports are well-placed!
Shifting fromtorii_coretotorii_sqlitealigns with the overall refactoring strategy. Confirm that all references toSQL_FELT_DELIMITERandOptimisticEventMessageare updated project-wide.crates/torii/graphql/src/object/event_message.rs (1)
10-11: Ohayo sensei, theSimpleBrokerandEventMessageimports are spot-on!
This minimal change completes the transition totorii_sqlitefor event message handling.crates/torii/grpc/src/server/subscriptions/entity.rs (4)
16-16: Ohayo sensei! Transitioning import fromtorii_core::constantstotorii_sqlite::constants
This change aligns with the new modular architecture. Everything looks good.
17-17: Ohayo sensei! Changing error imports totorii_sqlite::error
Seamless integration of error types from the SQLite-specific module.
18-18: Ohayo sensei! Updating the broker import
Switching totorii_sqlite::simple_broker::SimpleBrokeris consistent with the new structure.
19-19: Ohayo sensei! Adoptingtorii_sqlite::types::OptimisticEntity
Good job reflecting the shift to SQLite-based entities. No issues detected.bin/torii/src/main.rs (6)
34-34: Ohayo sensei! Introducingtorii_indexer::engine::*
These engine-related imports correctly match the new indexing structure.
35-35: Ohayo sensei! Using theStoreTransactionProcessorfromtorii_indexer
The store transaction processor is now migrated totorii_indexer, ensuring a consistent approach.
38-38: Ohayo sensei! Switching toModelCachefromtorii_sqlite::cache
Incorporating theModelCachefrom the SQLite module improves clarity of database logic.
39-39: Ohayo sensei! Executor import fromtorii_sqlite
This aligns the execution engine with the new SQLite-based infrastructure.
40-40: Ohayo sensei! Broker import fromtorii_sqlite
Great job ensuring real-time updates integrate with the SQLite broker.
41-41: Ohayo sensei! MovingContract,ContractType, andModeltotorii_sqlite::types
The domain-specific types now reside in the SQLite module, reflecting the new design.crates/torii/graphql/src/object/erc/token_transfer.rs (3)
8-8: Ohayo sensei!get_transaction_hash_from_event_idrelocated totorii_indexer::engine
No issues spotted. The refactor is consistent with the indexing logic.
9-9: Ohayo sensei! UsingTOKEN_TRANSFER_TABLEfromtorii_sqlite::constants
This table constant is now properly aligned undertorii_sqlite.
10-10: Ohayo sensei!felt_to_sql_stringmoved totorii_sqlite::utils
Logical shift of this helper function into the SQLite module is correct.crates/torii/graphql/src/tests/mod.rs (5)
30-30: Ohayo sensei! IntroducingEngine,EngineConfig, andProcessorsfromtorii_indexer::engine
The indexing engine references are now properly consolidated undertorii_indexer.
31-31: Ohayo sensei! Swapping toModelCachefromtorii_sqlite::cache
Centralizing model caching logic intorii_sqliteis a clean refactor.
32-32: Ohayo sensei! Executor import fromtorii_sqlite::executor
Adheres to the consistent architectural shift toward the SQLite-based executor.
33-33: Ohayo sensei! UpdatingContractandContractTypereferences
Spot-on integration of these contracts into the SQLite context.
34-34: Ohayo sensei! Shifting theSqltype totorii_sqlite::Sql
This unifies SQL operations under the new SQLite-driven design.crates/torii/server/src/artifacts.rs (2)
18-18: Ohayo sensei! InliningIPFS_CLIENT_MAX_RETRYandTOKENS_TABLEfromtorii_sqlite::constants
Ensures the IPFS logic references the appropriate constants from the correct module.
19-19: Ohayo sensei! Utilizingfetch_content_from_ipfsfromtorii_sqlite::utils
The IPFS utility now belongs totorii_sqlite, reflecting the shift to the SQLite-based architecture.crates/torii/cli/src/options.rs (1)
8-8: Ohayo sensei! The updated import is consistent with your new architecture
Everything looks consistent with the shift from torii-core to torii-sqlite, which helps unify the codebase under the new approach.crates/torii/graphql/src/tests/subscription_test.rs (1)
20-24: Ohayo sensei! The updated imports ensure correct referencing of the newtorii_sqlitecrate
Make sure that the transition doesn't break any references or tests that rely on the oldtorii_coremodules.crates/torii/graphql/Cargo.toml (1)
31-32: Ohayo sensei! Dependencies ontorii-sqliteandtorii-indexerintroduced
The new dependencies align with the architecture refactor for the indexing and database layer.crates/torii/grpc/Cargo.toml (3)
17-17: Ohayo sensei!torii-sqliteintroduced as an optional dependency
This supports the new SQLite-based approach for the project.
41-41: Ohayo sensei!torii-indexeradded
This addition aligns with the project's new modular architecture for indexing.
65-65: Ohayo sensei! Theserverfeature now depends ontorii-sqlite
Ensure your builds remain consistent across different targets since wasm builds may omit the server feature.
| let mut member = schema | ||
| .as_struct() | ||
| .expect("model schema must be a struct") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, replace .expect(...) with graceful error handling.
Using .expect(...) can panic if the schema is not a Struct. Handle this case gracefully to prevent unexpected panics in production.
- .expect("model schema must be a struct")
+ .ok_or_else(|| Error::msg("Model schema is not a struct"))?📝 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 mut member = schema | |
| .as_struct() | |
| .expect("model schema must be a struct") | |
| let mut member = schema | |
| .as_struct() | |
| .ok_or_else(|| Error::msg("Model schema is not a struct"))? |
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 (1)
crates/torii/indexer/src/processors/store_update_member.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/store_update_member.rs
[error] 6-12: Code formatting error: 'use torii_sqlite::Sql;' statement is incorrectly positioned. It should be at line 6 instead of line 12.
🔇 Additional comments (2)
crates/torii/indexer/src/processors/store_update_member.rs (2)
74-76: Ohayo sensei, gracefully handle the schema mismatch scenario.Revisit your usage of
.expect(...); in production, this can cause unexpected panics. Consider returning an error to ensure that an invalid schema does not lead to a forced application exit.
1-102: Ohayo sensei, overall structure looks good.The rest of the file is well-structured, and your approach for handling
StoreUpdateMemberevents is coherent. Keep up the good work!🧰 Tools
🪛 GitHub Actions: ci
[error] 6-12: Code formatting error: 'use torii_sqlite::Sql;' statement is incorrectly positioned. It should be at line 6 instead of line 12.
| use starknet::core::types::Event; | ||
| use starknet::core::utils::get_selector_from_name; | ||
| use starknet::providers::Provider; | ||
| use tracing::info; | ||
|
|
||
| use super::{EventProcessor, EventProcessorConfig}; | ||
| use torii_sqlite::Sql; |
There was a problem hiding this comment.
Ohayo sensei, please fix the code formatting error.
The pipeline complains that use torii_sqlite::Sql; at line 12 is incorrectly positioned. Move it closer to line 6 or reorganize imports to resolve the formatting issue.
use anyhow::{Context, Error, Result};
use async_trait::async_trait;
+use torii_sqlite::Sql;
use dojo_types::schema::{Struct, Ty};
use dojo_world::contracts::abigen::world::Event as WorldEvent;
use dojo_world::contracts::world::WorldContractReader;
-use torii_sqlite::Sql;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: ci
[error] 6-12: Code formatting error: 'use torii_sqlite::Sql;' statement is incorrectly positioned. It should be at line 6 instead of line 12.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2856 +/- ##
=======================================
Coverage 55.74% 55.75%
=======================================
Files 446 445 -1
Lines 57818 57818
=======================================
+ Hits 32230 32234 +4
+ Misses 25588 25584 -4 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
Refactoring
torii-sqliteandtorii-indexerpackagesNew Features
Improvements
Changes
torii-coretotorii-sqlitePerformance
This release focuses on architectural improvements and modularization of the Torii library.