feat(torii-indexer): transaction calls and outside calls - #3085
Conversation
WalkthroughOhayo, sensei! This pull request introduces a caching mechanism for contract classes in the SQLite module via a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant Cache as ContractClassCache
participant P as Provider
C ->> Cache: get(contract_address, block_id)
alt Entry exists in cache
Cache -->> C: Return cached ContractClass
else Entry missing in cache
Cache ->> P: Fetch contract hash for contract_address
P -->> Cache: Return contract hash
Cache ->> P: Fetch ContractClass using contract hash
P -->> Cache: Return ContractClass
Cache ->> Cache: Store ContractClass in cache
Cache -->> C: Return ContractClass
end
sequenceDiagram
participant T as Transaction
participant S as store_transaction Method
participant CP as Call Parser
participant DB as Database
T ->> S: Invoke store_transaction(transaction_hash, sender_address, ...)
S ->> CP: Parse calldata to extract FunctionCall details
CP -->> S: Return lists of calls and outsideCalls
S ->> DB: Insert transaction record with JSON serialized calls
DB -->> S: Acknowledge insertion
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
🧹 Nitpick comments (2)
crates/torii/sqlite/src/cache.rs (1)
165-179: Error handling needs improvement in thegetmethod.Sensei, while the cache implementation is solid, I noticed you're using
unwrap()on lines 175 and 176 which could cause panics if the provider operations fail. Consider proper error handling instead.- let class_hash = self.provider.get_class_hash_at(block_id, contract_address).await.unwrap(); - let class = self.provider.get_class(block_id, class_hash).await.unwrap(); + let class_hash = self.provider.get_class_hash_at(block_id, contract_address).await?; + let class = self.provider.get_class(block_id, class_hash).await?;crates/torii/sqlite/src/lib.rs (1)
565-596: Consider extracting outside call parsing into a helper function.Ohayo, sensei! The outside call parsing logic is duplicated for both v2 and v3 cases. Consider extracting this into a helper function to avoid code duplication.
+ fn parse_outside_calls(calldata: &[Felt], calldata_offset: usize, has_v3_formatting: bool) -> Vec<FunctionCall> { + let mut outside_calls = vec![]; + let outside_calls_idx = if has_v3_formatting { calldata_offset + 5 } else { calldata_offset + 4 }; + let outside_calls_len: usize = calldata[outside_calls_idx].try_into().unwrap_or(0); + + let offset_start = if has_v3_formatting { calldata_offset + 6 } else { calldata_offset + 5 }; + + for _ in 0..outside_calls_len { + let to_offset = offset_start; + let selector_offset = to_offset + 1; + let calldata_offset = selector_offset + 2; + let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap_or(0); + + let outside_call = FunctionCall { + contract_address: calldata[to_offset], + entry_point_selector: calldata[selector_offset], + calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), + }; + outside_calls.push(outside_call); + } + + outside_calls + } // Then in the code: - if call.entry_point_selector == selector!("execute_from_outside_v3") { - let outside_calls_len: usize = calldata[calldata_offset + 5].try_into().unwrap(); - for _ in 0..outside_calls_len { - let to_offset = calldata_offset + 6; - let selector_offset = to_offset + 1; - let calldata_offset = selector_offset + 2; - let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); - let outside_call = FunctionCall { - contract_address: calldata[to_offset], - entry_point_selector: calldata[selector_offset], - calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), - }; - outside_calls.push(outside_call); - } - } else if call.entry_point_selector == selector!("execute_from_outside_v2") { - // the execute_from_outside_v2 nonce is only a felt, thus we have a 4 offset - let outside_calls_len: usize = calldata[calldata_offset + 4].try_into().unwrap(); - for _ in 0..outside_calls_len { - let to_offset = calldata_offset + 5; - let selector_offset = to_offset + 1; - let calldata_offset = selector_offset + 2; - let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); - let outside_call = FunctionCall { - contract_address: calldata[to_offset], - entry_point_selector: calldata[selector_offset], - calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), - }; - outside_calls.push(outside_call); - } + if call.entry_point_selector == selector!("execute_from_outside_v3") { + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, true)); + } else if call.entry_point_selector == selector!("execute_from_outside_v2") { + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, false));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/migrations/20250307134638_transaction-calls.sql(1 hunks)crates/torii/sqlite/src/cache.rs(2 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ensure-windows
- GitHub Check: test-hurl
- GitHub Check: ensure-wasm
- GitHub Check: dojo-core-test
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: dojo-examples-test
🔇 Additional comments (5)
crates/torii/migrations/20250307134638_transaction-calls.sql (1)
1-7: LGTM! The migration adds necessary columns for transaction analysis.Ohayo, sensei! This migration looks well-structured, adding TEXT columns for storing parsed calls from transaction calldata and outside calls. This aligns perfectly with the PR objective to enhance transaction handling functionality.
crates/torii/sqlite/src/cache.rs (1)
154-163: Well-structured contract class cache implementation.Ohayo, sensei! The
ContractClassCachestruct looks clean and follows good design patterns with proper generics constraints. Nice job using RwLock for thread safety!crates/torii/sqlite/src/lib.rs (3)
14-15: LGTM! Imports updated for the new functionality.Ohayo, sensei! The imports look good with the addition of
FunctionCalland theselectormacro.
524-532: LGTM! Updated transaction field access.Sensei, you've correctly modified this section to directly use the calldata fields instead of converting them to strings right away.
602-614: LGTM! SQL query updated to handle new transaction data.Nice work adding the new columns to the SQL query and properly serializing the calls data as JSON, sensei!
| for _ in 0..calls_len { | ||
| let to_offset = offset + 1; | ||
| let selector_offset = to_offset + 1; | ||
| let calldata_offset = selector_offset + 2; | ||
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | ||
|
|
||
| let call = FunctionCall { | ||
| contract_address: calldata[to_offset], | ||
| entry_point_selector: calldata[selector_offset], | ||
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguards against out-of-bounds access.
Sensei, this section of code calculates various offsets and accesses elements by index without verifying array bounds first. This could lead to panics if called with unexpected data.
Consider adding checks before accessing array elements:
for _ in 0..calls_len {
let to_offset = offset + 1;
let selector_offset = to_offset + 1;
let calldata_offset = selector_offset + 2;
+
+ if selector_offset + 1 >= calldata.len() {
+ break; // or return an error
+ }
let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap();
+
+ if calldata_offset + calldata_len > calldata.len() {
+ break; // or return an error
+ }
let call = FunctionCall {
contract_address: calldata[to_offset],
entry_point_selector: calldata[selector_offset],
calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(),
};📝 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.
| for _ in 0..calls_len { | |
| let to_offset = offset + 1; | |
| let selector_offset = to_offset + 1; | |
| let calldata_offset = selector_offset + 2; | |
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | |
| let call = FunctionCall { | |
| contract_address: calldata[to_offset], | |
| entry_point_selector: calldata[selector_offset], | |
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | |
| }; | |
| for _ in 0..calls_len { | |
| let to_offset = offset + 1; | |
| let selector_offset = to_offset + 1; | |
| let calldata_offset = selector_offset + 2; | |
| if selector_offset + 1 >= calldata.len() { | |
| break; // or return an error | |
| } | |
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | |
| if calldata_offset + calldata_len > calldata.len() { | |
| break; // or return an error | |
| } | |
| let call = FunctionCall { | |
| contract_address: calldata[to_offset], | |
| entry_point_selector: calldata[selector_offset], | |
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | |
| }; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3085 +/- ##
==========================================
- Coverage 57.14% 56.82% -0.33%
==========================================
Files 442 442
Lines 61142 61561 +419
==========================================
+ Hits 34942 34979 +37
- Misses 26200 26582 +382 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/graphql/src/mapping.rs (1)
67-71: Consider using camelCase for field names!Ohayo sensei! The new
FUNCTION_CALL_MAPPINGfields use snake_case (contract_address,entry_point_selector,calldata), but GraphQL conventions and other mappings in this file use camelCase. Consider renaming these fields to maintain consistency:- (Name::new("contract_address"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), - (Name::new("entry_point_selector"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), + (Name::new("contractAddress"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), + (Name::new("entryPointSelector"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/graphql/src/constants.rs(1 hunks)crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/transaction.rs(1 hunks)crates/torii/graphql/src/schema.rs(2 hunks)
🔇 Additional comments (8)
crates/torii/graphql/src/constants.rs (1)
33-33: LGTM! New type name constant follows convention.Ohayo! The constant
FUNCTION_CALL_TYPE_NAMEis properly defined and follows the same naming pattern as other type names in this file. Good consistency, sensei!crates/torii/graphql/src/schema.rs (2)
29-29: Import looks good!Ohayo! The import for
FunctionCallObjectalongsideTransactionObjectis properly structured. Clean change, sensei!
138-138: Excellent addition to the objects list!The
FunctionCallObjectis properly added to the predefined objects list following the same pattern as otherBasicobjects. This ensures it will be registered in the GraphQL schema correctly.crates/torii/graphql/src/object/transaction.rs (3)
5-6: Import pattern looks good!Ohayo! The constant imports are well-organized with the addition of
FUNCTION_CALL_TYPE_NAME. Nicely done, sensei!
7-7: Proper mapping import!Adding the import for
FUNCTION_CALL_MAPPINGfollows the established patterns. Clean addition, sensei!
10-25: New FunctionCallObject implementation looks good!Ohayo! The implementation of
FunctionCallObjectfollows the existing pattern of other objects in this codebase. The naming is consistent with GraphQL conventions using camelCase for "functionCall".crates/torii/graphql/src/mapping.rs (2)
7-7: Import addition is correct!Ohayo! The import for
FUNCTION_CALL_TYPE_NAMEis properly added to the existing imports. Good organization, sensei!
86-93: LGTM! New fields for transaction calls look good!Ohayo! The additions of
callsandoutsideCallsfields to theTRANSACTION_MAPPINGcorrectly use theFUNCTION_CALL_MAPPINGfor their nested types. The naming uses camelCase which follows GraphQL conventions. Excellent work, sensei!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/torii/sqlite/src/lib.rs (2)
582-597:⚠️ Potential issueAdd safeguards against out-of-bounds access.
Sensei, this section of code calculates various offsets and accesses elements by index without verifying array bounds first. This could lead to panics if called with unexpected data.
Consider adding checks before accessing array elements:
for _ in 0..calls_len { let to_offset = offset + 1; let selector_offset = to_offset + 1; let calldata_offset = selector_offset + 2; + + if selector_offset + 1 >= calldata.len() { + break; // or return an error + } let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + + if calldata_offset + calldata_len > calldata.len() { + break; // or return an error + } let call = FunctionCall { contract_address: calldata[to_offset], entry_point_selector: calldata[selector_offset], calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), };
582-634:⚠️ Potential issueTransaction calldata parsing logic needs better error handling.
Ohayo, sensei! The transaction parsing logic is intricate, but I'm concerned about the error handling. You're using
unwrap()for array conversions and accessing elements without bounds checking, which could lead to runtime panics.- let calls_len: usize = calldata[0].try_into().unwrap(); + let calls_len: usize = calldata.get(0) + .ok_or_else(|| anyhow!("Calldata is empty"))? + .try_into() + .map_err(|_| anyhow!("Failed to parse calls length"))?; // Add similar error handling for other unwraps and array accessesAlso, add checks before accessing array indices to ensure they're within bounds:
+ if calldata.len() <= selector_offset + 1 { + return Err(anyhow!("Calldata too short for selector_offset")); + } let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + if calldata.len() < calldata_offset + calldata_len { + return Err(anyhow!("Calldata too short for calldata_len")); + }
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
599-614: Replace magic numbers with named constants.Ohayo! The code uses magic numbers like
calldata_offset + 5for offsets, making it difficult to understand the structure of the calldata. Consider defining named constants for these offsets to improve readability.+ // Offsets in execute_from_outside_v3 calldata + const OUTSIDE_CALLS_LEN_OFFSET_V3: usize = 5; + const OUTSIDE_CALLS_START_OFFSET_V3: usize = 6; if call.entry_point_selector == selector!("execute_from_outside_v3") { - let outside_calls_len: usize = calldata[calldata_offset + 5].try_into().unwrap(); + let outside_calls_len: usize = calldata[calldata_offset + OUTSIDE_CALLS_LEN_OFFSET_V3].try_into().unwrap(); for _ in 0..outside_calls_len { - let to_offset = calldata_offset + 6; + let to_offset = calldata_offset + OUTSIDE_CALLS_START_OFFSET_V3;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/migrations/20250317023025_transaction_calls.sql(1 hunks)crates/torii/sqlite/src/lib.rs(3 hunks)
🔇 Additional comments (8)
crates/torii/migrations/20250317023025_transaction_calls.sql (2)
1-9: Well-structured table design with appropriate constraints.The table structure is well designed with the appropriate foreign key constraint linking transaction calls to transactions. The default value for
call_typeis a nice touch to maintain consistent data.
11-14: Good indexing strategy for query optimization.You've created indexes on all the important columns that will be used in WHERE clauses. This will ensure optimal query performance when filtering by transaction hash, contract address, entry point selector, or call type.
crates/torii/sqlite/src/lib.rs (6)
14-15: LGTM! Appropriate imports added.The additions of
FunctionCallfromstarknet::core::typesand theselectormacro are necessary for the new transaction call parsing functionality.
524-525: Direct calldata handling improves data integrity.Ohayo, sensei! By passing the raw calldata directly rather than converting it to a string representation, you're preserving the original data structure for later parsing. This is a good improvement for data integrity.
Also applies to: 532-533, 540-541
558-559: Good, still using string representation for storage.While you're processing the raw calldata for the call parsing, you correctly maintain the string representation for the main transactions table. This ensures backward compatibility with existing queries.
577-580: Clean control flow for transaction type filtering.The early return for non-INVOKE transactions keeps the code clean and avoids unnecessary processing.
637-650: LGTM! Clean insertion of transaction calls.The insertion of call data into the transaction_calls table is well-implemented with proper SQL queries and argument handling.
652-666: LGTM! Good differentiation between call types.Setting 'EXECUTE_FROM_OUTSIDE' as the call_type for outside calls helps distinguish them from regular calls in queries, which is valuable for filtering and analysis.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
crates/torii/sqlite/src/lib.rs (2)
582-587: 🛠️ Refactor suggestionAdd error handling for calldata parsing
Sensei, the current implementation uses
unwrap()which could panic if the calldata is malformed. Consider adding proper error handling here.-let calls_len: usize = calldata[0].try_into().unwrap(); +let calls_len: usize = calldata.get(0) + .ok_or_else(|| anyhow!("Calldata is empty"))? + .try_into() + .map_err(|_| anyhow!("Failed to parse calls length"))?;
588-598:⚠️ Potential issueAdd safeguards against out-of-bounds access
Ohayo, sensei! This section calculates various offsets and accesses elements without verifying array bounds, which could lead to panics with unexpected data.
for _ in 0..calls_len { let to_offset = offset + 1; let selector_offset = to_offset + 1; let calldata_offset = selector_offset + 2; + + if selector_offset + 1 >= calldata.len() { + return Err(anyhow!("Calldata too short for selector_offset")); + } let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + + if calldata_offset + calldata_len > calldata.len() { + return Err(anyhow!("Calldata too short for calldata_len")); + } let call = FunctionCall { contract_address: calldata[to_offset], entry_point_selector: calldata[selector_offset], calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), };
🧹 Nitpick comments (6)
crates/torii/graphql/src/mapping.rs (1)
89-96: Ohayo sensei! Confirm consistent usage of these new fields.
Adding bothcallsandoutsideCallsis clear, but be sure the rest of the code—like resolvers and database queries—handles them in a way that clearly differentiates the two. If usage might overlap, consider a more descriptive naming convention.crates/torii/migrations/20250317023025_transaction_calls.sql (1)
12-15: Ohayo sensei! Indexes look solid.
If queries frequently combine these columns, consider multi-column indexes in a future iteration to improve performance for more complex queries.crates/torii/graphql/src/query/data.rs (2)
35-41: Ohayo sensei! Comprehensive join configuration struct.
In the future, you might add validations (e.g., verifyingon_conditionsyntax) to catch potential errors early.
72-106: Ohayo sensei! Good unified approach to fetch a single row with joins.
- Watch out for SQL injection: the dynamic string interpolation is fine here if all user inputs are validated.
- The pipeline flagged formatting issues at lines 82 and 95. Please run
rustfmtto clear them up.🧰 Tools
🪛 GitHub Actions: ci
[error] 82-82: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 95-95: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/graphql/src/object/transaction.rs (2)
156-159: Improve error handling for pagination parametersSensei, you're using
unwrap_orandunwrap_or_elsefor pagination parameters, which is good for providing defaults. However, it would be even better to validate these parameters to ensure they're within reasonable ranges.-let limit: u64 = extract(ctx.args.as_index_map(), "limit").unwrap_or(50); -let offset: u64 = extract(ctx.args.as_index_map(), "offset").unwrap_or(0); +let limit: u64 = extract(ctx.args.as_index_map(), "limit").unwrap_or(50).min(100); // Cap at 100 +let offset: u64 = extract(ctx.args.as_index_map(), "offset").unwrap_or(0); -let order_by: String = extract(ctx.args.as_index_map(), "orderBy").unwrap_or_else(|_| "executed_at".to_string()); -let order_direction: String = extract(ctx.args.as_index_map(), "orderDirection").unwrap_or_else(|_| "DESC".to_string()); +// Validate order_by is a valid column name to prevent SQL injection +let order_by: String = { + let column = extract(ctx.args.as_index_map(), "orderBy").unwrap_or_else(|_| "executed_at".to_string()); + match column.as_str() { + "executed_at" | "transaction_hash" | "sender_address" | "block_number" => column, + _ => "executed_at".to_string(), // Default to a safe column if invalid + } +}; + +// Validate order_direction is either ASC or DESC +let order_direction: String = { + let direction = extract(ctx.args.as_index_map(), "orderDirection").unwrap_or_else(|_| "DESC".to_string()); + match direction.as_str() { + "ASC" | "DESC" => direction, + _ => "DESC".to_string(), // Default to DESC if invalid + } +};
1-213: Fix Rust formatting issuesOhayo, sensei! The pipeline has flagged multiple Rust formatting issues. Please run
rustfmton this file to resolve these formatting issues.rustfmt crates/torii/graphql/src/object/transaction.rs🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 8-8: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 63-63: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 72-72: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 94-94: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 103-103: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 113-113: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 122-122: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 151-151: Rust formatting check failed. Please run 'rustfmt' to format the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
crates/torii/graphql/src/constants.rs(2 hunks)crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/mod.rs(2 hunks)crates/torii/graphql/src/object/transaction.rs(2 hunks)crates/torii/graphql/src/query/data.rs(2 hunks)crates/torii/migrations/20250317023025_transaction_calls.sql(1 hunks)crates/torii/sqlite/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/graphql/src/constants.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/graphql/src/object/mod.rs
[error] 28-28: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 35-35: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 271-271: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/sqlite/src/lib.rs
[warning] 580-580: Rust formatting issues detected. Please run 'rustfmt' to format the code.
[warning] 669-669: Rust formatting issues detected. Please run 'rustfmt' to format the code.
[warning] 686-686: Rust formatting issues detected. Please run 'rustfmt' to format the code.
crates/torii/graphql/src/query/data.rs
[error] 82-82: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 95-95: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/graphql/src/object/transaction.rs
[error] 1-1: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 8-8: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 63-63: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 72-72: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 94-94: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 103-103: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 113-113: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 122-122: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 151-151: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (15)
crates/torii/graphql/src/mapping.rs (1)
7-9: Ohayo sensei! Nice addition of the new imports.
They align well with the rest of the file’s constants.crates/torii/graphql/src/object/mod.rs (2)
31-31: Ohayo sensei! Good import alignment.
These additional imports are consistent with the new join-based functionality introduced in the codebase.
35-35: Ohayo sensei! JoinConfig import looks correct.
Everything is properly set up to handle advanced join scenarios.🧰 Tools
🪛 GitHub Actions: ci
[error] 35-35: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/torii/migrations/20250317023025_transaction_calls.sql (1)
1-10: Ohayo sensei! Double-check the foreign key referencing.
transaction_hashreferencestransactions(id)—verify that thetransactionstable indeed usesidas a primary key for these hashes. If the primary key intransactionsis actuallytransaction_hash, point your foreign key to the matching column.crates/torii/graphql/src/query/data.rs (2)
43-49: Ohayo sensei! Creative approach for unsupported SQLite joins.
MappingRIGHT JOINandFULL JOINtoLEFT JOINis practical. Just ensure the rest of the code and queries handle potential data disparities if you rely on right/full semantics.
51-60: Ohayo sensei! Theas_sqlmethod is straightforward.
If you add more join types or need more robust cross-database support, consider an adapter pattern for better maintainability.crates/torii/sqlite/src/lib.rs (6)
14-15: New imports for improved code structureOhayo, sensei! Good job adding the
FunctionCalltype to the imports and including theselectormacro. This properly organizes the dependencies needed for the new transaction call parsing functionality.
524-540: Calldata handling improvementNice optimization to pass calldata references directly instead of converting them to strings prematurely. This approach is more efficient and preserves the original data structure for proper parsing.
577-580: Early return for non-INVOKE transactionsGood pattern to check transaction type and return early if processing isn't needed. This makes the code path clearer and more efficient.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 580-580: Rust formatting issues detected. Please run 'rustfmt' to format the code.
600-631: Refactor duplicated outside call parsing logicThe logic for parsing outside calls is duplicated between v2 and v3 handling with only small differences in offsets. This reduces maintainability.
+fn parse_outside_calls( + calldata: &[Felt], + calldata_offset: usize, + calls_len_offset: usize, + calls_start_offset: usize, +) -> Result<Vec<(Felt, FunctionCall)>> { + let mut outside_calls = Vec::new(); + let outside_calls_len: usize = calldata.get(calldata_offset + calls_len_offset) + .ok_or_else(|| anyhow!("Calldata too short"))? + .try_into() + .map_err(|_| anyhow!("Failed to parse outside calls length"))?; + + let mut offset = 0; + for _ in 0..outside_calls_len { + let to_offset = calldata_offset + calls_start_offset + offset; + let selector_offset = to_offset + 1; + let calldata_offset = selector_offset + 2; + + if selector_offset + 1 >= calldata.len() { + return Err(anyhow!("Calldata too short for outside call selector_offset")); + } + let calldata_len: usize = calldata[selector_offset + 1].try_into() + .map_err(|_| anyhow!("Failed to parse outside call calldata length"))?; + + if calldata_offset + calldata_len > calldata.len() { + return Err(anyhow!("Calldata too short for outside call calldata_len")); + } + + let outside_call = FunctionCall { + contract_address: calldata[to_offset], + entry_point_selector: calldata[selector_offset], + calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), + }; + outside_calls.push((calldata[0], outside_call)); + offset += 3 + calldata_len; + } + + Ok(outside_calls) +} if call.entry_point_selector == selector!("execute_from_outside_v3") { - let outside_calls_len: usize = calldata[calldata_offset + 5].try_into().unwrap(); - for _ in 0..outside_calls_len { - // ... duplicated logic ... - } + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, 5, 6)?); } else if call.entry_point_selector == selector!("execute_from_outside_v2") { - // ... duplicated logic ... + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, 4, 5)?); }
637-652: Good separation of transaction call storageOhayo! I like how you've separated the storage of the main calls into their own loop. This improves code readability and maintainability.
Some minor formatting issues were flagged in the pipeline. Remember to run
rustfmton the final code.
654-669: Good handling of outside calls with appropriate call typeNice work differentiating between regular calls and outside calls by using the "EXECUTE_FROM_OUTSIDE" call type. This will be helpful for filtering and querying later.
Remember to fix the formatting issues flagged by the pipeline by running
rustfmt.🧰 Tools
🪛 GitHub Actions: ci
[warning] 669-669: Rust formatting issues detected. Please run 'rustfmt' to format the code.
crates/torii/graphql/src/object/transaction.rs (3)
1-14: Updated imports for new functionalityOhayo, sensei! Good work organizing all necessary imports for the new GraphQL functionality. The inclusion of HashMap and Row imports will be useful for the new query processing.
🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 8-8: Rust formatting check failed. Please run 'rustfmt' to format the code.
15-30: Well-structured FunctionCallObject implementationNice job implementing the new
FunctionCallObjectstruct with clear method names and references to constants. This follows the same pattern as other GraphQL objects in your system.
49-57: Updated resolvers to include new function call methodsGood refactoring to include the new resolver methods for transactions with calls. This maintains a clean API structure.
| pub static ref FUNCTION_CALL_MAPPING: TypeMapping = IndexMap::from([ | ||
| (Name::new("contractAddress"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| (Name::new("entryPointSelector"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| (Name::new("calldata"), TypeData::Simple(TypeRef::named_list(TypeRef::STRING))), | ||
| (Name::new("callType"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider using felt-like fields for contract addresses.
Currently, contractAddress and entryPointSelector are typed as strings. If these values represent FELT-252 addresses/selectors, you could store them as numeric or typed FELT strings to maintain consistency across the codebase and possibly prevent accidental misuse.
| // Resolves single object queries with joins, returns current object of type type_name with related data | ||
| pub fn resolve_one_with_joins( | ||
| table_name: &str, | ||
| id_column: &str, | ||
| field_name: &str, | ||
| type_name: &str, | ||
| type_mapping: &TypeMapping, | ||
| joins: Vec<JoinConfig>, | ||
| select_columns: Option<Vec<String>>, | ||
| ) -> Field { | ||
| let type_mapping = type_mapping.clone(); | ||
| let table_name = table_name.to_owned(); | ||
| let id_column = id_column.to_owned(); | ||
| let joins = joins.to_owned(); | ||
| let select_columns = select_columns.to_owned(); | ||
| let argument = InputValue::new(id_column.to_case(Case::Camel), TypeRef::named_nn(TypeRef::ID)); | ||
|
|
||
| Field::new(field_name, TypeRef::named_nn(type_name), move |ctx| { | ||
| let type_mapping = type_mapping.clone(); | ||
| let table_name = table_name.to_owned(); | ||
| let id_column = id_column.to_owned(); | ||
| let joins = joins.to_owned(); | ||
| let select_columns = select_columns.to_owned(); | ||
|
|
||
| FieldFuture::new(async move { | ||
| let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?; | ||
| let id: String = | ||
| extract::<String>(ctx.args.as_index_map(), &id_column.to_case(Case::Camel))?; | ||
| let data = fetch_single_row_with_joins( | ||
| &mut conn, | ||
| &table_name, | ||
| &id_column, | ||
| &id, | ||
| joins, | ||
| select_columns, | ||
| ).await?; | ||
| let model = value_mapping_from_row(&data, &type_mapping, false, true)?; | ||
| Ok(Some(Value::Object(model))) | ||
| }) | ||
| }) | ||
| .argument(argument) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider refactoring out common parts with resolve_one.
resolve_one_with_joins largely duplicates logic from resolve_one. Extracting shared parts (like argument handling or row retrieval) into helper functions could improve maintainability and reduce potential inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/torii/sqlite/src/lib.rs (3)
587-598:⚠️ Potential issueImplement bounds checking for calldata access.
Ohayo, sensei! This section is still vulnerable to out-of-bounds accesses, just like in the previous review. The code accesses array elements without verifying they exist first.
for _ in 0..calls_len { let to_offset = offset + 1; let selector_offset = to_offset + 1; let calldata_offset = selector_offset + 2; + + if to_offset >= calldata.len() || selector_offset >= calldata.len() { + break; // or return an error + } let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + + if calldata_offset + calldata_len > calldata.len() { + break; // or return an error + } let call = FunctionCall { contract_address: calldata[to_offset], entry_point_selector: calldata[selector_offset], calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), };
600-631: 🛠️ Refactor suggestionRefactor duplicated outside call parsing logic.
Ohayo, sensei! There's significant duplication between the parsing logic for v2 and v3 outside calls. This is the same issue noted in the previous review.
Extract the common logic into a helper function:
+ fn parse_outside_calls( + calldata: &[Felt], + calldata_offset: usize, + calls_len_offset: usize, + calls_start_offset: usize, + ) -> Vec<(Felt, FunctionCall)> { + let mut outside_calls = Vec::new(); + let outside_calls_len: usize = calldata[calldata_offset + calls_len_offset].try_into().unwrap(); + + let mut offset = 0; + for _ in 0..outside_calls_len { + let to_offset = calldata_offset + calls_start_offset + offset; + let selector_offset = to_offset + 1; + let calldata_offset = selector_offset + 2; + let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + + let outside_call = FunctionCall { + contract_address: calldata[to_offset], + entry_point_selector: calldata[selector_offset], + calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), + }; + outside_calls.push((call.contract_address, outside_call)); + offset += 3 + calldata_len; + } + + outside_calls + } if call.entry_point_selector == selector!("execute_from_outside_v3") { - let outside_calls_len: usize = calldata[calldata_offset + 5].try_into().unwrap(); - for _ in 0..outside_calls_len { - // ... duplicated logic ... - } + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, 5, 6)); } else if call.entry_point_selector == selector!("execute_from_outside_v2") { - // ... duplicated logic ... + outside_calls.extend(parse_outside_calls(calldata, calldata_offset, 4, 5)); }
582-635: 🛠️ Refactor suggestionImprove error handling in transaction calls parsing.
Ohayo, sensei! The transaction parsing logic needs better error handling. You're using
unwrap()for conversions and not handling potential errors.- let calls_len: usize = calldata[0].try_into().unwrap(); + let calls_len: usize = calldata.get(0) + .ok_or_else(|| anyhow!("Calldata is empty"))? + .try_into() + .map_err(|_| anyhow!("Failed to parse calls length"))?; // Similarly for other unwraps: - let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); + let calldata_len: usize = calldata.get(selector_offset + 1) + .ok_or_else(|| anyhow!("Missing calldata length"))? + .try_into() + .map_err(|_| anyhow!("Failed to parse calldata length"))?;This would make the function return meaningful errors instead of panicking.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/lib.rs
[error] 580-580: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 669-669: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 686-686: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (4)
crates/torii/sqlite/src/lib.rs (4)
14-15: Added the new FunctionCall import and selector! macro.Ohayo, sensei! I see you've added the
FunctionCalltype to your imports and included theselector!macro. These additions align perfectly with your new transaction call parsing feature.
524-540: Improved parameter handling for calldata.Nice improvement here! You've changed from converting the calldata to a string to directly passing it as a reference, which is much more efficient for the parsing approach you're implementing.
582-586: Added variables for tracking function calls.You're correctly initializing the needed data structures for parsing and storing both direct calls and outside calls.
638-652: Good implementation of transaction calls storage.The SQLite insertion for regular function calls looks good. You've correctly mapped all necessary fields and used appropriate types.
| for outside_call in outside_calls { | ||
| self.executor.send(QueryMessage::other( | ||
| "INSERT OR IGNORE INTO transaction_calls (transaction_hash, contract_address, entry_point_selector, calldata, call_type, caller_address) \ | ||
| VALUES (?, ?, ?, ?, ?, ?)" | ||
| .to_string(), | ||
| vec![ | ||
| transaction_hash.clone(), | ||
| Argument::FieldElement(outside_call.1.contract_address), | ||
| Argument::FieldElement(outside_call.1.entry_point_selector), | ||
| Argument::String(felts_to_sql_string(&outside_call.1.calldata)), | ||
| Argument::String("EXECUTE_FROM_OUTSIDE".to_string()), | ||
| Argument::FieldElement(outside_call.0), | ||
| ], | ||
| ))?; | ||
| } |
There was a problem hiding this comment.
Need to fix formatting in outside calls storage.
The implementation for storing outside calls is correct, but the pipeline reports a formatting issue.
Run rustfmt on the file to fix the formatting issues on line 669.
🧰 Tools
🪛 GitHub Actions: ci
[error] 669-669: Rust formatting check failed. Please run 'rustfmt' to format the code.
| // Parse and store calls for INVOKE transactions | ||
| if transaction_type != "INVOKE" { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Logic added to process only INVOKE transactions.
Good check to only process INVOKE transactions for call parsing. However, there's a formatting issue reported by the pipeline.
- // Parse and store calls for INVOKE transactions
- if transaction_type != "INVOKE" {
- return Ok(());
- }
+ // Parse and store calls for INVOKE transactions
+ if transaction_type != "INVOKE" {
+ return Ok(());
+ }Run rustfmt to fix the formatting issues.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: ci
[error] 580-580: Rust formatting check failed. Please run 'rustfmt' to format the code.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
crates/torii/indexer/src/engine.rs(5 hunks)crates/torii/indexer/src/processors/mod.rs(3 hunks)crates/torii/indexer/src/processors/store_transaction.rs(2 hunks)crates/torii/migrations/20250317023025_transaction_calls.sql(1 hunks)crates/torii/sqlite/src/cache.rs(2 hunks)crates/torii/sqlite/src/lib.rs(3 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/migrations/20250317023025_transaction_calls.sql
🔇 Additional comments (21)
crates/torii/sqlite/src/types.rs (2)
230-243: Ohayo! Clean implementation of CallType enum.The CallType enum with Display implementation follows best practices and provides a clear way to distinguish between different types of contract calls.
245-252: Well-structured ParsedCall struct, sensei!This struct provides all the necessary fields to represent a contract call in a structured way. Good job including the caller_address which will be helpful for call attribution.
crates/torii/indexer/src/engine.rs (6)
26-26: Ohayo! Adding ContractClassCache import.This import properly brings in the new caching functionality we're implementing.
233-233: Nice field addition for caching, sensei!Adding the contract_class_cache field to the Engine struct will help optimize transaction processing by avoiding redundant contract class retrievals.
260-261: Clean provider initialization.Wrapping the provider in Arc directly without redundant code.
265-266: Using cloned provider consistently.Good practice to use the already Arc-wrapped provider.
278-279: Proper cache initialization, sensei!Correctly initializing the contract_class_cache with the provider.
779-780: Passing cache reference to processor.Correctly passing the contract class cache to the transaction processor.
crates/torii/indexer/src/processors/mod.rs (4)
2-2: Adding Arc import for shared caching.Good addition of the Arc import needed for the contract class cache.
9-9: Importing the cache module.Proper import of the ContractClassCache component.
95-95: Adding Debug trait bound, nice work sensei!Adding std::fmt::Debug trait bound will help with debugging issues related to transaction processing.
106-107: Adding contract_class_cache parameter to the process method.This change enables the TransactionProcessor to access cached contract class data, which will help with parsing and interpreting transaction calls.
crates/torii/sqlite/src/cache.rs (1)
157-166: Ohayo! Well-designed ContractClassCache struct.The cache implementation with RwLock provides thread-safe access to cached contract classes, which is important in an asynchronous environment.
crates/torii/sqlite/src/lib.rs (3)
14-15: Ohayo sensei, imports look good!
No issues spotted.Also applies to: 18-18
546-551: Ohayo sensei, storing associated contracts looks good!
This insertion uses two placeholders for two arguments, properly matching columns.
553-566: Ohayo sensei, storing calls intransaction_callsis well-structured!
The placeholders match the arguments. No issues spotted.crates/torii/indexer/src/processors/store_transaction.rs (5)
5-10: Ohayo sensei, the new imports align perfectly with usage!
They are referenced properly within the file.
29-35: Ohayo sensei, good check on transaction type!
Skipping non-Invokeor non-L1Handlertransactions ensures the rest of the code won’t process irrelevant data.
37-64: Ohayo sensei, the expanded match arms are clear, but watch for missing boundary checks.
Ifcalldatais empty inTransaction::Invoke(V3), callingcalldata[0]triggers a panic.We previously advised adding out-of-bounds checks for
calldatausage. Please ensure the code checkscalldata.len()before indexing.
66-144: Ohayo sensei, calls parsing logic looks good but needs boundary checks.
Parsingcalls_lenand iterating could cause panics ifcalldatalacks sufficient elements. This has already been flagged in a past review.
146-158: Ohayo sensei, storing the transaction with associated calls is properly invoked.
No further concerns here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/store_transaction.rs (2)
66-80: Good implementation of calldata parsing!The implementation correctly extracts each call from the calldata array by calculating offsets and lengths. Using the
contract_class_cacheto fetch contract classes and resolve entrypoint names is a great approach to make the data more readable and useful.Consider adding error handling for the
try_into()unwrap operations to gracefully handle malformed calldata.- let calls_len: usize = calldata[0].try_into().unwrap(); + let calls_len: usize = calldata[0].try_into().unwrap_or_else(|_| { + log::warn!("Failed to parse calls_len from calldata, defaulting to 0"); + 0 + });
89-114: Ohayo! Nice handling of execute_from_outside_v3 calls, sensei!The special handling for
execute_from_outside_v3calls correctly extracts the nested calls, which is crucial for tracking all transaction interactions. The code is well-structured with clear offset calculations.However, there's some duplication between this block and the v2 handler. Consider refactoring to reduce duplication.
You could extract the common outside call parsing logic into a helper function to reduce duplication between v2 and v3 handlers:
fn parse_outside_calls( calls: &mut Vec<ParsedCall>, calldata: &[Felt], base_offset: usize, calls_len_offset: usize, caller_address: Felt, block_number: u64, contract_class_cache: &ContractClassCache<P> ) -> Result<(), Error> { let outside_calls_len: usize = calldata[base_offset + calls_len_offset].try_into().unwrap(); let start_offset = base_offset + calls_len_offset + 1; for _ in 0..outside_calls_len { // Extract and push outside calls (existing logic) // ... } Ok(()) }Then call it with appropriate offsets for v3 and v2.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/processors/store_transaction.rs(3 hunks)
🔇 Additional comments (8)
crates/torii/indexer/src/processors/store_transaction.rs (8)
5-10: Ohayo! Nice job with the imports, sensei!The new imports are well-organized and clearly reflect the new functionality being added. The addition of ContractClassCache and related imports aligns perfectly with the PR objective of implementing transaction calls parsing.
19-30: Method signature update looks good, sensei!Adding the
contract_class_cacheparameter to the process method ensures that contract class information can be retrieved efficiently during transaction processing, which is essential for resolving entrypoint names from selectors.
31-35: Transaction type matching is well-implemented!The matching of transaction types to identify "INVOKE" and "L1_HANDLER" transactions is clean and provides early return for unsupported types, which is a good programming practice.
37-64: Excellent transaction data extraction, sensei!You've handled all the different transaction types (InvokeV3, InvokeV1, L1Handler) thoroughly, extracting the necessary fields from each. The pattern matching is clean and comprehensive.
I particularly appreciate the explicit handling of fields that don't exist in certain transaction types (using Felt::ZERO for missing max_fee and empty vector for missing signature).
81-88: ParsedCall struct creation is well-structured!Creating a
ParsedCallobject with the extracted information is a clean approach. The fields are properly populated with the relevant data from the transaction.
114-140: Good handling of execute_from_outside_v2 calls!The implementation correctly handles the v2 format which has a different offset structure. The comment explaining the difference is helpful for understanding the code.
As mentioned for the v3 handler, consider refactoring to reduce duplication with the v3 logic.
141-144: Logical call addition and offset update, sensei!Adding the main call to the calls vector after processing any outside calls is the correct approach. The offset calculation for moving to the next call in the calldata is accurate.
146-158: Store transaction call looks good!The method signature for
db.store_transactionhas been updated to include the newcallsparameter, ensuring the parsed call data is stored in the database.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/torii/indexer/src/processors/store_transaction.rs (3)
94-151: Be careful with hard-coded offsets in outside calls parsing, sensei.While the implementation for both
execute_from_outside_v3andexecute_from_outside_v2looks thorough, the hard-coded offsets (likecalldata_offset + 5andcalldata_offset + 4) make the code somewhat brittle. Consider extracting these magic numbers into named constants or calculating them based on the calldata structure to improve maintainability.- let outside_calls_len: usize = calldata[calldata_offset + 5].try_into().unwrap(); + // V3 structure: [max_amount, nonce_low, nonce_high, block_timestamp, expiration_timestamp, calls_len, ...] + const V3_CALLS_LEN_OFFSET: usize = 5; + let outside_calls_len: usize = calldata[calldata_offset + V3_CALLS_LEN_OFFSET].try_into().unwrap();Similar refactoring could be applied to other offsets in the code.
152-156: Be mindful of the loop structure and offset calculation, sensei.The offset calculation seems accurate but can be error-prone when the calldata structure changes. It would be good to add a comment explaining the calldata structure or the logic behind the calculation for future reference.
94-151: Consider refactoring the duplicate code in handling outside calls, sensei.The code for handling
execute_from_outside_v3andexecute_from_outside_v2is very similar with only slight differences in offsets. This could be refactored into a helper function to reduce duplication and improve maintainability.fn process_outside_calls( calldata: &[Felt], base_offset: usize, calls_len_offset: usize, start_offset: usize, block_number: u64, caller_address: Felt, contract_class_cache: &ContractClassCache<P>, calls: &mut Vec<ParsedCall>, ) -> Result<(), Error> { let outside_calls_len: usize = calldata[base_offset + calls_len_offset].try_into().unwrap(); for _ in 0..outside_calls_len { let to_offset = base_offset + start_offset; let selector_offset = to_offset + 1; // Rest of the implementation... } Ok(()) } // Then call it like: if call.entrypoint == "execute_from_outside_v3" { process_outside_calls(calldata, calldata_offset, 5, 6, block_number, call.contract_address, contract_class_cache, &mut calls)?; } else if call.entrypoint == "execute_from_outside_v2" { process_outside_calls(calldata, calldata_offset, 4, 5, block_number, call.contract_address, contract_class_cache, &mut calls)?; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/processors/store_transaction.rs(3 hunks)
🔇 Additional comments (4)
crates/torii/indexer/src/processors/store_transaction.rs (4)
5-7: Ohayo! The updated imports and method signature look good.The new parameter
contract_class_cachealong with the necessary imports forContractClassCache,get_entrypoint_name_from_class,ParsedCall, and other types are well organized and match the implementation needs.Also applies to: 9-10, 19-21, 31-32
33-67: Nice match pattern implementation for transaction types, sensei!The extraction of common transaction fields is well-structured, with proper handling of different transaction variants. I like how you've organized the code to handle both
InvokeTransaction(V1 and V3) andL1Handlertransactions, with appropriate default values for missing fields.
68-93: The function call parsing logic is well implemented!The implementation correctly handles the calldata structure for invoke transactions, calculating offsets properly and extracting contract addresses and entrypoints. Using the contract class cache to resolve the human-readable entrypoint name is a nice touch for improved debugging and readability.
158-170: The updates to db.store_transaction look good!The transaction storage function is now called with the parsed calls, which aligns with the purpose of this PR. This allows storing structured function calls instead of just raw calldata.
| let mut calls: Vec<ParsedCall> = vec![]; | ||
|
|
||
| if transaction_type == "INVOKE" { | ||
| let calls_len: usize = calldata[0].try_into().unwrap(); | ||
| let mut offset = 0; | ||
| for _ in 0..calls_len { | ||
| let to_offset = offset + 1; | ||
| let selector_offset = to_offset + 1; | ||
| let calldata_offset = selector_offset + 2; | ||
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | ||
| let contract_address = calldata[to_offset]; | ||
| let contract_class = contract_class_cache | ||
| .get(contract_address, BlockId::Number(block_number)) | ||
| .await?; | ||
| let entrypoint = | ||
| get_entrypoint_name_from_class(&contract_class, calldata[selector_offset]) | ||
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | ||
|
|
||
| let call = ParsedCall { | ||
| contract_address, | ||
| entrypoint, | ||
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | ||
| call_type: CallType::Execute, | ||
| caller_address: sender_address, | ||
| }; | ||
|
|
||
| if call.entrypoint == "execute_from_outside_v3" { | ||
| let outside_calls_len: usize = | ||
| calldata[calldata_offset + 5].try_into().unwrap(); | ||
| for _ in 0..outside_calls_len { | ||
| let to_offset = calldata_offset + 6; | ||
| let selector_offset = to_offset + 1; | ||
| let calldata_offset = selector_offset + 2; | ||
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | ||
| let contract_address = calldata[to_offset]; | ||
| let contract_class = contract_class_cache | ||
| .get(contract_address, BlockId::Number(block_number)) | ||
| .await?; | ||
| let entrypoint = get_entrypoint_name_from_class( | ||
| &contract_class, | ||
| calldata[selector_offset], | ||
| ) | ||
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | ||
|
|
||
| let outside_call = ParsedCall { | ||
| contract_address, | ||
| entrypoint, | ||
| calldata: calldata[calldata_offset..calldata_offset + calldata_len] | ||
| .to_vec(), | ||
| call_type: CallType::ExecuteFromOutside, | ||
| caller_address: call.contract_address, | ||
| }; | ||
| calls.push(outside_call); | ||
| } | ||
| } else if call.entrypoint == "execute_from_outside_v2" { | ||
| // the execute_from_outside_v2 nonce is only a felt, thus we have a 4 offset | ||
| let outside_calls_len: usize = | ||
| calldata[calldata_offset + 4].try_into().unwrap(); | ||
| for _ in 0..outside_calls_len { | ||
| let to_offset = calldata_offset + 5; | ||
| let selector_offset = to_offset + 1; | ||
| let calldata_offset = selector_offset + 2; | ||
| let calldata_len: usize = calldata[selector_offset + 1].try_into().unwrap(); | ||
| let contract_address = calldata[to_offset]; | ||
| let contract_class = contract_class_cache | ||
| .get(contract_address, BlockId::Number(block_number)) | ||
| .await?; | ||
| let entrypoint = get_entrypoint_name_from_class( | ||
| &contract_class, | ||
| calldata[selector_offset], | ||
| ) | ||
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | ||
|
|
||
| let outside_call = ParsedCall { | ||
| contract_address, | ||
| entrypoint, | ||
| calldata: calldata[calldata_offset..calldata_offset + calldata_len] | ||
| .to_vec(), | ||
| call_type: CallType::ExecuteFromOutside, | ||
| caller_address: call.contract_address, | ||
| }; | ||
| calls.push(outside_call); | ||
| } | ||
| } | ||
|
|
||
| calls.push(call); | ||
| offset += 3 + calldata_len; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good implementation of transaction call parsing, but needs bounds checking, sensei.
The current implementation accesses array elements directly without bounds checking, which could lead to panics if the calldata is malformed or shorter than expected. Consider adding bounds checking before accessing array elements or use safe access methods like get().
🏁 Script executed:
#!/bin/bash
# Check if there are any existing error handling for calldata access in the codebase
rg "unwrap\(\)" --type rust crates/torii/indexer/src/processors/ -A 1 -B 1Length of output: 4535
Ohayo sensei, let's add those bounds checks!
The transaction call parsing logic in crates/torii/indexer/src/processors/store_transaction.rs is structured nicely. However, as it stands, it directly indexes into the calldata array without verifying that the indices are within range. This approach (using constructs like calldata[0].try_into().unwrap()) could lead to panics if the input calldata is malformed or shorter than expected. While other modules in the codebase also use similar unwrap calls, enhancing safety here would be beneficial for robustness.
- Please consider:
- Adding explicit bounds checking before each array access (or using safe methods like
.get()). - Revisiting the offset calculations in both the primary and nested loops to ensure they don't exceed
calldata’s length.
- Adding explicit bounds checking before each array access (or using safe methods like
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
114-166:⚠️ Potential issueOhayo sensei, watch out for SQL injection in
LIKEclause!
Embedding{transaction_hash}directly in the string can be risky. Consider parameterizing the pattern (e.g., usinget.event_id LIKE '%' || ? || '%') for safety.
🧹 Nitpick comments (2)
crates/torii/graphql/src/object/erc/token_transfer.rs (1)
263-382: Ohayo sensei, commendable modular design!
token_transfer_mapping_from_rowunifies parsing logic and error handling across all token types. However, consider splitting out sub-logic (e.g., metadata parsing) into helper functions for improved readability.crates/torii/graphql/src/object/erc/token_balance.rs (1)
319-448: Ohayo sensei, robust token balance mapping!
token_balance_mapping_from_rowhandles multiple ERC types elegantly. If it grows further, consider extracting metadata parsing into helper methods for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/graphql/src/constants.rs(2 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(2 hunks)crates/torii/graphql/src/object/erc/token_transfer.rs(4 hunks)crates/torii/graphql/src/object/transaction.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/graphql/src/constants.rs
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/graphql/src/object/erc/token_transfer.rs (4)
crates/torii/graphql/src/object/erc/token_balance.rs (2) (2)
row(335:335)row(391:391)crates/torii/indexer/src/engine.rs (1) (1)
get_transaction_hash_from_event_id(929:931)crates/torii/sqlite/src/executor/erc.rs (2) (2)
token_id(72:72)token_id(109:109)crates/torii/sqlite/src/types.rs (1) (1)
from_str(195:204)
🔇 Additional comments (8)
crates/torii/graphql/src/object/erc/token_transfer.rs (3)
21-21: Ohayo sensei, nice import alignment!
Bringing inErc1155TokenandErc721Tokenis helpful for handling multiple ERC token types with clarity.
245-253: Ohayo sensei, great error-handling approach!
Continuing after a failed row transformation ensures partial data is still processed while logging issues for debugging.
394-394: Ohayo sensei, good call on makingTransferQueryResultRawpublic!
This change improves cross-module accessibility and reuse without duplicating structs.crates/torii/graphql/src/object/erc/token_balance.rs (2)
301-310: Ohayo sensei, nice integration of the mapping function!
Invokingtoken_balance_mapping_from_rowhere streamlines the transformation and centralizes error handling.
312-317: Ohayo sensei, succinct connection construction!
Returning early with the connection object is straightforward and reduces clutter in the calling code.crates/torii/graphql/src/object/transaction.rs (3)
19-20: Ohayo sensei, neat introduction ofCallObject!
Defining a dedicated struct keeps calls organized and flexible for future expansions.
22-38: Ohayo sensei, well-structuredBasicObjectimplementation!
Providingrelated_fieldsasNonekeepsCallObjectlean and consistent until additional relationships are needed.
55-55: Ohayo sensei, good combination of fields!
ExposingcallsandtokenTransfersat the transaction level enhances discoverability of contextual data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/torii/sqlite/src/types.rs (1)
254-268: Consider a numeric type forblock_numberinstead ofString.Currently, storing
block_numberas aStringmight introduce overhead in parsing and data validation. Converting it to an integer-based type (e.g.,i64) could be helpful, if feasible.crates/torii/sqlite/src/executor/mod.rs (1)
451-476: Ohayo sensei, storing transaction data and calls in a single transaction is sound.One optimization to consider is a bulk insert if there are many calls, which can improve performance. Otherwise, logic is straightforward and correct.
crates/torii/sqlite/src/cache.rs (2)
157-162: Ohayo sensei, consider adding bounds to your cache.
Currently, the cache can grow unbounded, potentially leading to large memory usage in production. An eviction or size-limiting strategy could help.
164-196: Check handling of multiple block IDs, sensei.
Because the cache stores classes keyed only by address, subsequent calls with differentblock_idvalues for the same address might reuse outdated data. Ensure that this fallback toBlockTag::Pendingand ignoring block ID variations is intentional for your scenario.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/sqlite/src/cache.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(8 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/lib.rs (1)
crates/torii/sqlite/src/utils.rs (2) (2)
felts_to_sql_string(32-35)utc_dt_string_from_timestamp(28-30)
🔇 Additional comments (19)
crates/torii/sqlite/src/types.rs (2)
229-244: Ohayo sensei, this enum addition looks great!Nice and concise implementation with matching
Displayvariants. No issues spotted here.
245-252: Ohayo sensei, this struct is clear and straightforward!The fields cover all necessary data for a parsed call, and the design is readable.
crates/torii/sqlite/src/executor/mod.rs (9)
1-1: Ohayo sensei, new import looks fine!
11-11: Ohayo sensei, no concerns about this addition.
27-27: Ohayo sensei, importingParsedCallandTransactionis appropriate.
29-29: No issues with these utility imports.
54-54: BrokerMessage variant inclusion looks good!
108-113: Ohayo sensei, this struct effectively captures new transaction data!
116-116: Enum variant addition is consistent with the new struct.
141-141: Renaming to avoid conflicts is a good call.
874-874: Publishing the transaction as a broker message is clearly integrated.crates/torii/sqlite/src/lib.rs (5)
13-13: Ohayo sensei, this import aligns perfectly with the new transaction logic.
15-15: No issues with this import.
18-18: ImportingParsedCallis consistent with usage instore_transaction.
507-520: Function signature is cohesive and well-defined.The chosen parameters cover all crucial transaction fields.
522-544: Ohayo sensei, the query logic is correctly aligned with the arguments.The column-argument mapping matches, preventing accidental shifts. Good job!
crates/torii/sqlite/src/cache.rs (3)
2-2: Ohayo sensei, nice usage of Arc!
This import facilitates shared ownership of your provider instance across async tasks.
7-12: Ohayo sensei, these imports look solid for contract interactions.
They neatly cover the essentials for fetching contract classes and handling provider errors.
198-257: Ohayo sensei,unwrap()usage here can still cause panics.
This duplicates a prior comment regarding unwrapping at lines 225 and 249. Consider graceful error handling or returningNoneif parsing fails.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/torii/graphql/src/object/erc/token_balance.rs (1)
326-452: Ohayo sensei! Well-structured token mapping function with robust error handling.The extracted
token_balance_mapping_from_rowfunction provides a centralized place for token type conversion logic. The error handling is thorough, especially for metadata parsing.One small improvement could be to use more descriptive variable names for clarity in the token_id splitting operations, particularly for ERC721 and ERC1155 sections.
- let token_id = row.token_id.split(':').collect::<Vec<&str>>(); - if token_id.len() != 2 { - return Err(format!("Invalid token_id format: {}", row.token_id)); - } + let token_id_parts = row.token_id.split(':').collect::<Vec<&str>>(); + if token_id_parts.len() != 2 { + return Err(format!("Invalid token_id format (expected 'contract:id'): {}", row.token_id)); + } + let contract_part = token_id_parts[0]; + let id_part = token_id_parts[1];crates/torii/graphql/src/object/transaction.rs (1)
143-145: Consider using a prepared statement for consistencyOhayo, sensei! While this query isn't vulnerable to SQL injection since it only uses constants, consider using a style consistent with other queries for better maintainability.
- let query = &format!( - "SELECT * FROM {TRANSACTION_CALLS_TABLE} WHERE transaction_hash = ?" - ); + let query = &format!( + "SELECT * FROM [{}] WHERE transaction_hash = ?", TRANSACTION_CALLS_TABLE + );This makes the query format consistent with other queries in the codebase using table name in square brackets.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
crates/torii/graphql/src/mapping.rs(1 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(2 hunks)crates/torii/graphql/src/object/erc/token_transfer.rs(4 hunks)crates/torii/graphql/src/object/mod.rs(2 hunks)crates/torii/graphql/src/object/transaction.rs(3 hunks)crates/torii/graphql/src/query/data.rs(2 hunks)crates/torii/sqlite/src/cache.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(8 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/object/erc/token_transfer.rs
- crates/torii/sqlite/src/types.rs
🧰 Additional context used
🧬 Code Definitions (4)
crates/torii/graphql/src/object/mod.rs (1)
crates/torii/graphql/src/query/data.rs (11) (11)
data(197-197)data(198-198)data(223-223)data(224-224)data(227-227)data(228-228)count_rows(11-26)fetch_multiple_rows(109-244)fetch_single_row(62-70)fetch_single_row_with_joins(72-106)joins(87-96)
crates/torii/sqlite/src/lib.rs (2)
crates/torii/sqlite/src/executor/mod.rs (2) (2)
new(166-168)new(241-267)crates/torii/sqlite/src/utils.rs (2) (2)
felts_to_sql_string(32-35)utc_dt_string_from_timestamp(28-30)
crates/torii/graphql/src/object/transaction.rs (2)
crates/torii/graphql/src/object/erc/token_transfer.rs (8) (8)
token_transfer_mapping_from_row(264-383)name(31-33)type_name(35-37)type_mapping(39-41)ctx(57-57)extract(59-59)row(289-289)row(331-331)crates/torii/graphql/src/object/mod.rs (16) (16)
resolve_many(321-374)resolve_one(246-273)name(46-46)type_name(49-49)type_mapping(52-52)ctx(84-84)ctx(109-109)ctx(123-123)ctx(148-148)ctx(161-161)ctx(190-190)ctx(264-264)ctx(301-301)ctx(339-339)extract(266-266)extract(303-303)
crates/torii/graphql/src/object/erc/token_balance.rs (8)
crates/torii/graphql/src/object/erc/token_transfer.rs (2) (2)
row(289-289)row(331-331)crates/torii/graphql/src/object/erc/erc_token.rs (2) (2)
row(597-597)row(639-639)crates/torii/graphql/src/object/connection/mod.rs (2) (2)
row(138-138)row(139-139)crates/torii/graphql/src/query/mod.rs (7) (7)
row(123-123)row(125-125)row(310-310)row(312-312)row(319-320)row(326-326)e(67-81)crates/torii/grpc/src/server/mod.rs (3) (3)
row(1130-1130)row(1131-1135)value(121-121)crates/torii/sqlite/src/executor/mod.rs (2) (2)
None(300-300)None(848-848)crates/torii/sqlite/src/executor/erc.rs (2) (2)
token_id(72-72)token_id(109-109)crates/torii/sqlite/src/types.rs (1) (1)
from_str(195-204)
🔇 Additional comments (29)
crates/torii/graphql/src/mapping.rs (1)
67-74: Ohayo sensei! Consider using felt-like fields for contract addresses.Currently,
contractAddressandentrypointare typed as strings. If these values represent FELT-252 addresses/selectors, you could store them as numeric or typed FELT strings to maintain consistency across the codebase (like you did in other mappings such as TRANSACTION_MAPPING).- (Name::new("contractAddress"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), - (Name::new("entrypoint"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), + (Name::new("contractAddress"), TypeData::Simple(TypeRef::named(Primitive::Felt252(None).to_string()))), + (Name::new("entrypoint"), TypeData::Simple(TypeRef::named(Primitive::Felt252(None).to_string()))),crates/torii/graphql/src/object/mod.rs (1)
275-318: Ohayo sensei! The implementation looks good but consider refactoring out common parts withresolve_one.The new function
resolve_one_with_joinslargely duplicates logic fromresolve_one. Consider extracting shared parts (like argument handling or field creation) into helper functions to improve maintainability and reduce potential inconsistencies.crates/torii/graphql/src/query/data.rs (1)
35-60: Ohayo sensei! Clean implementation of join types and configuration.The
JoinTypeenum andJoinConfigstruct are well-designed and provide a flexible way to configure SQL joins. I appreciate the handling of SQLite's limitations for RIGHT and FULL joins in theas_sqlmethod. This makes the API more forgiving for users who might not be aware of SQLite's limitations.crates/torii/graphql/src/object/erc/token_balance.rs (2)
147-159: Ohayo sensei! Good refactoring of token balance logic for subscriptions.The refactoring to use the centralized
token_balance_mapping_from_rowfunction improves code maintainability and consistency. The error handling with warning logs is also beneficial for debugging.
307-316: Ohayo sensei! Good refactoring of token balance output logic.Extracting the token mapping logic into a separate function makes the code more maintainable and easier to understand. The improved error handling with warning logs helps debugging potential issues.
crates/torii/graphql/src/object/transaction.rs (5)
22-41: Ohayo! New CallObject implementation looks good!The new
CallObjectstruct and its implementation of theBasicObjecttrait follow the established pattern in the codebase. The type mappings are properly referenced from constants.
58-60: Related fields implementation looks clean, sensei!Good addition of both call and token transfer fields to the transaction object, making the GraphQL API more feature-rich.
84-112: Nice subscription implementation!The subscription field for transactions with optional hash filtering is well implemented. The filter_map pattern allows for efficient filtering of transaction events.
115-131: Complete value mapping for transaction fields!All the necessary transaction fields are correctly mapped to GraphQL field names with appropriate types.
133-163: Good calls field implementation, sensei!The implementation fetches and maps transaction calls correctly. The query uses proper parameter binding with the
?placeholder, which is secure against SQL injection.crates/torii/sqlite/src/cache.rs (6)
2-2: Ohayo sensei, this import looks correct.
No concerns here.
7-12: Ohayo sensei, these new imports are well-organized.
Everything seems in order.
157-163: Ohayo sensei, nice concurrency setup.
Storing(Felt, ContractClass)in anRwLock<HashMap<...>>is straightforward. Consider memory usage if the cache grows large.
164-168: Ohayo sensei, neat constructor forContractClassCache.
It cleanly injects the provider and read/write lock.
198-254: Ohayo sensei, consider handling.unwrap()safely.
Calls to.unwrap()can panic on invalid JSON or selector mismatch. ReturningNoneor an error is safer.- let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi).unwrap(); + let abi: Vec<AbiEntry> = match serde_json::from_str(&sierra.abi) { + Ok(abi) => abi, + Err(_) => return None, + }; - if get_selector_from_name(&function.name).unwrap() == selector + if get_selector_from_name(&function.name).ok()? == selector
169-195:Details
❓ Verification inconclusive
Ohayo sensei, verify cache key usage with different blocks.
Thegetmethod stores contract classes keyed only by contract address, ignoringblock_id. This may lead to returning stale data if a contract address differs across blocks.Here’s a script to see if references exist regarding multi-block caching strategy:
🏁 Script executed:
#!/bin/bash rg --context 5 "multi-block"Length of output: 28
Ohayo, sensei! Quick heads-up on the caching strategy:
Thegetmethod currently caches contract classes using only the contract address and ignores theblock_id. This design may lead to serving stale data in cases where a contract’s class could differ between blocks. The grep search for "multi-block" didn't yield any supporting evidence for a multi-block caching strategy elsewhere in the codebase, which suggests this might be an oversight.
- Please double-check whether the intention was to cache solely based on contract address.
- If contract variants across blocks are valid, consider incorporating
block_idinto the cache key to avoid stale data.crates/torii/sqlite/src/executor/mod.rs (8)
1-1: Ohayo sensei, this addition looks good!
No concerns with importingHashSet. It's neatly used for storing distinct contract addresses.
11-11: Ohayo sensei, no issues found here!
Aliasing theTransactiontype toSqlxTransactionhelps avoid naming collisions. Continue.
27-27: No immediate concerns with the new imports.
IntroducingParsedCallis a welcome addition for structured call data handling.
29-29: Neat usage of helpers.
felt_to_sql_stringandfelts_to_sql_stringare consistent with the existing code. Good to see them used here.
108-112: Ohayo sensei, nice addition ofStoreTransactionQuery.
Encapsulating the set of contract addresses and parsed calls in one struct clarifies the code’s intent. Consider including other relevant transaction metadata if needed in future expansions.
116-116: Enum variant addition looks good.
AddingStoreTransaction(StoreTransactionQuery)is consistent with the rest of theQueryTypeenum usage.
141-141: Smooth transition toSqlxTransaction.
Renaming prevents confusion with your internalTransactiontype while preserving clarity.
451-489: Ohayo sensei, solid handling of the newStoreTransactionvariant!
- The logic for inserting into
transaction_contractandtransaction_callsis cohesive and straightforward.- Using
INSERT OR IGNOREprevents duplicates, but confirm that ignoring collisions is desired behavior. If updates are ever needed, consider usingINSERT ... ON CONFLICT ... DO UPDATE.- Fetching and using the newly inserted transaction row is well-managed. The error handling with
.with_context(...)is helpful for debugging.- Pushing
BrokerMessage::Transactionto the queue is a good approach for decoupling.Looking great overall!
Do you want to confirm that collisions for the tables
transaction_contractandtransaction_callsare never expected to require subsequent updates? If so, verifying no manual updates are needed might be wise.crates/torii/sqlite/src/lib.rs (5)
13-13: Imports look neat.
No issues with referencingStoreTransactionQuery,Felt, and the newParsedCall.Also applies to: 15-15, 18-18
509-519: Ohayo sensei, good signature for your new function parameters!
Explicit fields liketransaction_hash,sender_address, andnoncemake everything more transparent. This design is more flexible than passing a singleTransactionobject.
521-522: Nice comment clarifying the insertion.
The purposeful approach of ignoring existing records might be fine if the transaction data won't change.
528-538: Double usage oftransaction_hashfor bothidandtransaction_hashcolumns is now aligned.
- The argument count matches the number of columns.
- Reusing the same
transaction_hashfor both columns can be intentional if they are effectively the same ID. Confirm this design meets your business requirements.Do you want me to check for references to a separate schema where
idcould differ fromtransaction_hash?
539-542: LinkingStoreTransactionQueryin the final step is consistent with your design.
You're properly encapsulating the contract addresses and calls in the query. Looks good!
| pub async fn fetch_single_row_with_joins( | ||
| conn: &mut SqliteConnection, | ||
| table_name: &str, | ||
| id_column: &str, | ||
| id: &str, | ||
| joins: Vec<JoinConfig>, | ||
| select_columns: Option<Vec<String>>, | ||
| ) -> sqlx::Result<SqliteRow> { | ||
| // Build the SELECT clause | ||
| let select = match select_columns { | ||
| Some(columns) => columns.join(", "), | ||
| None => format!("[{}].*", table_name), | ||
| }; | ||
|
|
||
| // Build the JOIN clauses | ||
| let join_clauses = joins | ||
| .iter() | ||
| .map(|join| { | ||
| let table_ref = match &join.alias { | ||
| Some(alias) => format!("[{}] AS {}", join.table, alias), | ||
| None => format!("[{}]", join.table), | ||
| }; | ||
| format!("{} {} ON {}", join.join_type.as_sql(), table_ref, join.on_condition) | ||
| }) | ||
| .collect::<Vec<String>>() | ||
| .join(" "); | ||
|
|
||
| // Build the complete query | ||
| let query = format!( | ||
| "SELECT {} FROM [{}] {} WHERE [{}].{} = '{}'", | ||
| select, table_name, join_clauses, table_name, id_column, id | ||
| ); | ||
|
|
||
| sqlx::query(&query).fetch_one(conn).await | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Be cautious about potential SQL injection in the join query builder.
While the function builds SQL queries dynamically, it directly interpolates values from the JoinConfig struct into the query string. Consider parameterizing these values where possible or implementing additional sanitization to prevent potential SQL injection attacks.
- let query = format!(
- "SELECT {} FROM [{}] {} WHERE [{}].{} = '{}'",
- select, table_name, join_clauses, table_name, id_column, id
- );
+ let query = format!(
+ "SELECT {} FROM [{}] {} WHERE [{}].{} = ?",
+ select, table_name, join_clauses, table_name, id_column
+ );
+ sqlx::query(&query).bind(id).fetch_one(conn).await📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn fetch_single_row_with_joins( | |
| conn: &mut SqliteConnection, | |
| table_name: &str, | |
| id_column: &str, | |
| id: &str, | |
| joins: Vec<JoinConfig>, | |
| select_columns: Option<Vec<String>>, | |
| ) -> sqlx::Result<SqliteRow> { | |
| // Build the SELECT clause | |
| let select = match select_columns { | |
| Some(columns) => columns.join(", "), | |
| None => format!("[{}].*", table_name), | |
| }; | |
| // Build the JOIN clauses | |
| let join_clauses = joins | |
| .iter() | |
| .map(|join| { | |
| let table_ref = match &join.alias { | |
| Some(alias) => format!("[{}] AS {}", join.table, alias), | |
| None => format!("[{}]", join.table), | |
| }; | |
| format!("{} {} ON {}", join.join_type.as_sql(), table_ref, join.on_condition) | |
| }) | |
| .collect::<Vec<String>>() | |
| .join(" "); | |
| // Build the complete query | |
| let query = format!( | |
| "SELECT {} FROM [{}] {} WHERE [{}].{} = '{}'", | |
| select, table_name, join_clauses, table_name, id_column, id | |
| ); | |
| sqlx::query(&query).fetch_one(conn).await | |
| } | |
| pub async fn fetch_single_row_with_joins( | |
| conn: &mut SqliteConnection, | |
| table_name: &str, | |
| id_column: &str, | |
| id: &str, | |
| joins: Vec<JoinConfig>, | |
| select_columns: Option<Vec<String>>, | |
| ) -> sqlx::Result<SqliteRow> { | |
| // Build the SELECT clause | |
| let select = match select_columns { | |
| Some(columns) => columns.join(", "), | |
| None => format!("[{}].*", table_name), | |
| }; | |
| // Build the JOIN clauses | |
| let join_clauses = joins | |
| .iter() | |
| .map(|join| { | |
| let table_ref = match &join.alias { | |
| Some(alias) => format!("[{}] AS {}", join.table, alias), | |
| None => format!("[{}]", join.table), | |
| }; | |
| format!("{} {} ON {}", join.join_type.as_sql(), table_ref, join.on_condition) | |
| }) | |
| .collect::<Vec<String>>() | |
| .join(" "); | |
| // Build the complete query | |
| let query = format!( | |
| "SELECT {} FROM [{}] {} WHERE [{}].{} = ?", | |
| select, table_name, join_clauses, table_name, id_column | |
| ); | |
| sqlx::query(&query).bind(id).fetch_one(conn).await | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
175-199:⚠️ Potential issuePotential SQL injection in token transfers query!
Ohayo, sensei! The SQL query is vulnerable to injection because you're directly interpolating the transaction_hash into the LIKE pattern instead of using a parameterized query.
Fix this by using parameters properly:
- WHERE - et.event_id LIKE '%:{transaction_hash}:%' + WHERE + et.event_id LIKE '%:' || ? || ':%'This ensures the transaction hash is properly escaped and handles any special characters safely.
🧹 Nitpick comments (3)
crates/torii/sqlite/src/types.rs (2)
230-234: Consider deriving more traits for CallType enumOhayo, sensei! The
CallTypeenum looks good but could benefit from deriving additional traits for better functionality:-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub enum CallType { Execute, ExecuteFromOutside, }Adding these traits would enable easy comparison, use in HashMap/HashSet collections, and serialization/deserialization - making the enum more versatile throughout the codebase.
245-252: ParsedCall struct could benefit from serialization traitsOhayo sensei! The
ParsedCallstruct looks well-structured for representing calls. For consistency with other types in this file, consider adding serialization traits:-#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize, Deserialize)] pub struct ParsedCall { pub contract_address: Felt, pub entrypoint: String, pub calldata: Vec<Felt>, pub call_type: CallType, pub caller_address: Felt, }This would make it easier to serialize/deserialize this struct when needed, especially for API responses or persistence.
crates/torii/graphql/src/object/transaction.rs (1)
204-211: Consider optimizing token transfer data processingThe current approach processes each row individually. For better performance with large result sets, consider using batch processing or parallelization.
- let mut results = Vec::new(); - for row in &rows { - let row = TransferQueryResultRaw::from_row(row)?; - let result = token_transfer_mapping_from_row(&row)?; - results.push(FieldValue::owned_any(result)); - } + let results = rows.iter() + .map(|row| -> Result<FieldValue, async_graphql::Error> { + let row = TransferQueryResultRaw::from_row(row)?; + let result = token_transfer_mapping_from_row(&row)?; + Ok(FieldValue::owned_any(result)) + }) + .collect::<Result<Vec<_>, _>>()?;This functional approach is more concise and makes the error handling more explicit.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/graphql/src/mapping.rs(2 hunks)crates/torii/graphql/src/object/transaction.rs(3 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/graphql/src/object/transaction.rs (2)
crates/torii/graphql/src/object/erc/token_transfer.rs (8) (8)
token_transfer_mapping_from_row(264-383)name(31-33)type_name(35-37)type_mapping(39-41)ctx(57-57)extract(59-59)row(289-289)row(331-331)crates/torii/graphql/src/object/mod.rs (16) (16)
resolve_many(321-374)resolve_one(246-273)name(46-46)type_name(49-49)type_mapping(52-52)ctx(84-84)ctx(109-109)ctx(123-123)ctx(148-148)ctx(161-161)ctx(190-190)ctx(264-264)ctx(301-301)ctx(339-339)extract(266-266)extract(303-303)
🔇 Additional comments (10)
crates/torii/sqlite/src/types.rs (2)
236-243: Display implementation looks goodThe
Displaytrait implementation is clear and follows the naming convention consistently. Nice work!
254-268: Transaction struct implementation looks goodThe
Transactionstruct is well-defined with appropriate fields and trait derivations. The camelCase serialization annotation is consistent with other structs in this file, and the inclusion ofFromRowmakes it compatible with SQLx for database operations.crates/torii/graphql/src/mapping.rs (2)
67-74: Ohayo sensei! Consider using felt-like fields for contract addresses.Currently,
contractAddress,entrypoint, andcallerAddressare typed as strings in your newCALL_MAPPING. If these values represent FELT-252 addresses/selectors, it would be more consistent to useTypeRef::named(Primitive::Felt252(None).to_string())like you've done in other mappings, rather than simple strings.
109-112: Looks good! Adding blockNumber to transaction mapping.The blockNumber field is properly added to the transaction mapping. Using STRING is appropriate here since block numbers are often represented in both decimal and hex formats in client applications.
crates/torii/graphql/src/object/transaction.rs (6)
22-41: Ohayo, the CallObject implementation looks good!The BasicObject implementation for CallObject is clean and follows the established pattern in the codebase. It properly references the constants and mappings defined elsewhere.
58-60: Good addition of related fields to TransactionObject!Adding calls and token transfers as related fields to transactions provides a nice GraphQL structure that allows clients to fetch this data in a single query.
84-112: Nice implementation of transaction subscriptions!The subscription implementation allows clients to receive real-time updates about transactions, with the option to filter by transaction hash. The filter logic is clean and efficient.
115-130: Clean value mapping implementation for transactions.This method properly maps all transaction fields to their GraphQL representation. The calldata splitting on "/" is an interesting approach - make sure this matches how calldata is stored in the database.
133-163: Ohayo sensei! The calls_field implementation looks good.The implementation correctly fetches call data associated with a transaction. You're properly using parameterized queries to prevent SQL injection.
202-202: Make sure SQL bindings match query parametersThe query uses string interpolation for transaction_hash (which is unsafe) but then also tries to bind it as a parameter. When fixing the LIKE clause, ensure you're using the bound parameter correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
178-205:⚠️ Potential issueSQL injection vulnerability in token_transfers query!
Ohayo, sensei! The query is vulnerable to SQL injection because you're directly interpolating the transaction_hash into the LIKE pattern instead of using a parameterized query. Additionally, you're binding a parameter on line 205, but there's no corresponding placeholder in the query.
Fix this by using parameters properly:
let query = format!( r#" SELECT et.id, et.contract_address, et.from_address, et.to_address, et.amount, et.token_id, et.executed_at, t.name, t.symbol, t.decimals, c.contract_type, t.metadata FROM {TOKEN_TRANSFER_TABLE} et JOIN tokens t ON et.token_id = t.id JOIN contracts c ON t.contract_address = c.contract_address WHERE - et.event_id LIKE '%:{transaction_hash}:%' + et.event_id LIKE '%:' || ? || ':%' "# ); let rows = sqlx::query(&query).bind(&transaction_hash).fetch_all(&mut *conn).await?;
🧹 Nitpick comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
115-134: Clean implementation of value_mappingThis helper function efficiently transforms the Transaction struct into a GraphQL-compatible value mapping. The use of
split("/")on line 123 for calldata works, but consider documenting the expected format to avoid future confusion.( Name::new("calldata"), - Value::from(transaction.calldata.split("/").collect::<Vec<_>>()), + // Split calldata on "/" as it's stored as slash-separated values + Value::from(transaction.calldata.split("/").collect::<Vec<_>>()), ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/graphql/src/object/transaction.rs(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/graphql/src/object/transaction.rs (5)
crates/torii/sqlite/src/lib.rs (3) (3)
sqlx(87-87)sqlx(175-175)sqlx(182-182)crates/torii/graphql/src/object/erc/token_transfer.rs (5) (5)
name(31-33)type_name(35-37)type_mapping(39-41)row(289-289)row(331-331)crates/torii/graphql/src/object/mod.rs (5) (5)
name(46-46)type_name(49-49)type_mapping(52-52)related_fields(55-57)subscriptions(216-218)crates/torii/graphql/src/object/erc/token_balance.rs (6) (6)
name(37-39)type_name(41-43)type_mapping(45-47)subscriptions(94-168)row(341-341)row(396-396)crates/torii/graphql/src/query/data.rs (1) (1)
list(291-298)
🔇 Additional comments (5)
crates/torii/graphql/src/object/transaction.rs (5)
22-41: Good implementation of CallObject struct, ohayo sensei!The new
CallObjectstructure is well-implemented with all required trait methods. It follows the same pattern as other objects in the codebase, making the code consistent and maintainable.
58-60: LGTM! Nice enhancement to related_fieldsAdding related fields for calls and token transfers improves the GraphQL schema by enabling clients to fetch transaction-related data in a single query.
84-113: Good implementation of GraphQL subscriptions, sensei!The subscription implementation allows clients to subscribe to transaction updates by hash or to all transactions. The filtering logic is clean and effective.
136-166: Nice implementation of calls_field, ohayo!The function correctly fetches calls for a transaction from the database. Good use of parameterized queries to prevent SQL injection!
207-219: LGTM! Good result handling logicThe code correctly processes the query results by mapping database rows to the expected GraphQL output format.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
194-218:⚠️ Potential issuePotential SQL injection vulnerability in token transfers query!
Ohayo, sensei! The SQL query is vulnerable to injection because you're directly interpolating the transaction_hash into the LIKE pattern instead of using a parameterized query.
Fix this by using parameters properly:
- let query = format!( - r#" - SELECT - et.id, - et.contract_address, - et.from_address, - et.to_address, - et.amount, - et.token_id, - et.executed_at, - t.name, - t.symbol, - t.decimals, - c.contract_type, - t.metadata - FROM - {TOKEN_TRANSFER_TABLE} et - JOIN - tokens t ON et.token_id = t.id - JOIN - contracts c ON t.contract_address = c.contract_address - WHERE - et.event_id LIKE '%:{transaction_hash}:%' - "# - ); + let query = format!( + r#" + SELECT + et.id, + et.contract_address, + et.from_address, + et.to_address, + et.amount, + et.token_id, + et.executed_at, + t.name, + t.symbol, + t.decimals, + c.contract_type, + t.metadata + FROM + {TOKEN_TRANSFER_TABLE} et + JOIN + tokens t ON et.token_id = t.id + JOIN + contracts c ON t.contract_address = c.contract_address + WHERE + et.event_id LIKE '%:' || ? || ':%' + "# + );This ensures the transaction hash is properly escaped and prevents SQL injection.
🧹 Nitpick comments (1)
crates/torii/graphql/src/object/transaction.rs (1)
139-140: Consider using collect with type annotation for clarity.Sensei, the code works but could be slightly clearer with an explicit type annotation:
- Value::from(transaction.calldata.split("/").collect::<Vec<_>>()), + Value::from(transaction.calldata.split("/").collect::<Vec<&str>>()),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/graphql/src/object/transaction.rs(3 hunks)crates/torii/sqlite/src/executor/mod.rs(8 hunks)crates/torii/sqlite/src/types.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/types.rs
🔇 Additional comments (16)
crates/torii/sqlite/src/executor/mod.rs (8)
1-1: Ohayo! Good addition of HashSet import.The HashSet is appropriately imported for efficient storage of unique contract addresses in the new transaction storage mechanism.
11-11: Nice type renaming to avoid name conflicts, sensei!Renaming
TransactiontoSqlxTransactionis a smart move to prevent confusion with the domainTransactiontype from your types module.
27-27: Good additions to support structured transaction calls.Adding
ParsedCallto the imports and thefelts_to_sql_stringutility function properly sets up the structure needed for the new transaction call handling mechanism.Also applies to: 29-29
108-112: Well-designed struct for transaction query!The
StoreTransactionQuerystruct elegantly captures both the contract addresses and function calls associated with a transaction. Using a HashSet for addresses ensures uniqueness.
116-116: Clean enum addition, sensei!The new
StoreTransactionvariant in theQueryTypeenum follows the existing pattern and integrates well with the codebase.
141-141: Consistent field type update.The
transactionfield type is correctly updated to match the renamedSqlxTransactiontype, maintaining consistency throughout the codebase.
451-492: Excellent implementation of transaction storage with structured calls!The implementation nicely handles:
- Retrieving the transaction from the database
- Storing contract addresses in the transaction_contract table
- Storing structured calls in the transaction_calls table
- Publishing the transaction message
The code properly uses parameterized queries to prevent SQL injection and includes proper error handling.
890-890: Updated broker message handling appropriately.The
send_broker_messagefunction is correctly updated to handle the new transaction message type.crates/torii/graphql/src/object/transaction.rs (8)
3-5: Ohayo, sensei! Good addition of SubscriptionField types.I see you've added imports for supporting the new subscription functionality. Clear and thoughtful inclusion of the necessary dynamic types.
15-17: Constants added appropriately for the new functionality.The constants for CALL_TYPE_NAME and TRANSACTION_CALLS_TABLE are properly imported, maintaining consistency with the project's pattern of centralizing constants.
19-23: Great job importing the CALL_MAPPING!Adding the CALL_MAPPING import aligns well with how you're handling other type mappings in the system.
25-44: Nice implementation of the CallObject struct!The implementation follows the established pattern for GraphQL objects in the system. Clean and consistent with other object implementations.
61-63: Well-structured related_fields implementation.You've properly updated the related_fields method to include both calls and token transfers, which creates a good GraphQL schema structure.
131-149: Clean value_mapping implementation!The method properly converts Transaction fields to GraphQL values. I like how you're using IndexMap for predictable field ordering.
152-182: Well implemented calls_field function!The function correctly fetches and maps function calls data. Good job using parameterized queries to prevent SQL injection.
221-221: Make sure SQL bindings match query parameters.The query uses string interpolation for transaction_hash (which is unsafe) but then also tries to bind it as a parameter. When fixing the LIKE clause, ensure you're using the bound parameter correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/mod.rs (1)
462-503: Ohayo sensei! Add error handling for calls collection processing.The implementation for handling StoreTransaction queries looks solid overall, but I'm concerned about the error handling when processing the calls collection. If there's an issue with the format of any call data, the current code might panic.
Consider adding error checking before accessing call data elements, especially when processing large call collections:
for call in &store_transaction.calls { + // Validate call data before inserting + if call.calldata.is_empty() { + tracing::warn!(target: LOG_TARGET, "Empty calldata for call to {} with entrypoint {}", &call.contract_address, &call.entrypoint); + } + sqlx::query( "INSERT OR IGNORE INTO transaction_calls (transaction_hash, \ contract_address, entrypoint, calldata, call_type, caller_address) \ VALUES (?, ?, ?, ?, ?, ?)", ) // ... }crates/torii/sqlite/src/lib.rs (1)
538-551: Ohayo! Updated function signature looks good but needs documentation.The function signature change from accepting a Transaction object to individual parameters makes the function more flexible but could benefit from documentation to explain the purpose and expected values of each parameter.
Consider adding a docstring to explain the function parameters:
+ /// Stores a transaction and its associated calls in the database + /// + /// # Arguments + /// * `transaction_hash` - The hash of the transaction + /// * `sender_address` - The address of the transaction sender + /// * `calldata` - The transaction calldata + /// * `max_fee` - The maximum fee for the transaction + /// * `signature` - The transaction signature + /// * `nonce` - The transaction nonce + /// * `block_number` - The block number where the transaction was included + /// * `contract_addresses` - Set of contract addresses associated with this transaction + /// * `transaction_type` - The type of transaction (e.g., "INVOKE") + /// * `block_timestamp` - The timestamp of the block + /// * `calls` - The parsed calls contained in this transaction pub fn store_transaction( &mut self, transaction_hash: Felt, sender_address: Felt, calldata: &[Felt], max_fee: Felt, signature: &[Felt], nonce: Felt, block_number: u64, contract_addresses: &HashSet<Felt>, transaction_type: &str, block_timestamp: u64, calls: &[ParsedCall], ) -> Result<()> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/torii/indexer/src/processors/mod.rs(3 hunks)crates/torii/sqlite/src/executor/mod.rs(9 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)crates/torii/sqlite/src/types.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/indexer/src/processors/mod.rs
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/lib.rs (2)
crates/torii/sqlite/src/executor/mod.rs (2) (2)
new(177-179)new(252-278)crates/torii/sqlite/src/utils.rs (2) (2)
felts_to_sql_string(35-38)utc_dt_string_from_timestamp(31-33)
🔇 Additional comments (16)
crates/torii/sqlite/src/types.rs (4)
227-231: Ohayo, nice enum implementation for call types!Clean and straightforward implementation with the two call types. The naming is descriptive and follows Rust conventions.
233-240: Display implementation looks good, sensei.The Display trait implementation correctly formats both variants as uppercase strings, which will be useful for storing in the database.
242-249: Solid ParsedCall struct design.This struct neatly encapsulates all the essential information about a contract call. The fields are aptly named and the field types are appropriate.
251-270: Transaction struct looks good with the new fields!I like how you've added the new fields for
callsandcontract_addresseswith the#[sqlx(skip)]attribute to ensure they're not included in database operations. This separation of concerns makes sense given the complex nature of these fields.crates/torii/sqlite/src/executor/mod.rs (8)
1-1: Nice HashSet addition to the imports.Adding HashSet to the existing HashMap import is clean and makes sense for the new contract_addresses field.
11-11: Good rename to avoid naming conflict, sensei!Renaming
TransactiontoSqlxTransactionavoids confusion with your new Transaction struct.
27-27: Clean import addition for ParsedCall.Adding ParsedCall to the existing imports from types.rs is an efficient way to manage the imports.
29-29: Well-organized utility function import.Adding the felts_to_sql_string utility shows good code organization.
54-54: BrokerMessage transaction variant looks good.The Transaction variant in BrokerMessage enum aligns with your other changes.
109-112: Clean design for StoreTransactionQuery struct.The struct has a simple and clear design with just two fields: contract_addresses and calls, which aligns well with the requirement to store transaction calls.
127-127: QueryType enum addition looks good.Adding the StoreTransaction variant to QueryType enum is a necessary and clean extension.
954-955: Transaction broker message publishing looks good.You've correctly updated the send_broker_message function to handle Transaction messages consistently with other message types.
crates/torii/sqlite/src/lib.rs (4)
13-14: Good update to imports.Adding StoreTransactionQuery to the imports is appropriate for the changes.
15-15: Clean up of import list.Removing Transaction from the starknet imports since you're now using your own Transaction struct.
18-19: Nice additions to imports.Adding ParsedCall and felts_to_sql_string shows good attention to dependencies.
552-574: Ohayo sensei! Transaction storage query looks solid but check the ID column.The implementation for storing transactions is clean and follows a consistent pattern with other parts of the codebase. However, I noticed you're using the transaction_hash as both the ID and transaction_hash columns.
Make sure this is intentional - if the ID column is meant to be a unique identifier separate from the transaction hash, you might want to generate a different value. If using the transaction hash as ID is intentional (which is common and reasonable), then this is perfect.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/sqlite/src/cache.rs (1)
164-196: Consider locking strategy to avoid duplicate fetches, sensei.
Your double-checked locking approach is fine, but if multiple tasks request the same contract concurrently, you could overlap fetches. A small improvement would be to store an “in-progress” marker or use a future-based cache.crates/torii/sqlite/src/executor/mod.rs (1)
463-469: Ohayo sensei! Verifying fetch withINSERT OR IGNORE.
While retrieving the newly inserted row withfetch_onecan work, keep in mind that "INSERT OR IGNORE" might not insert a row if it already exists, potentially makingfetch_onereturn zero rows. Consider handling the scenario where the record already exists to avoid runtime errors.You could, for example, switch to a pattern like:
- INSERT OR IGNORE INTO transactions (...) VALUES (...) RETURNING * + INSERT INTO transactions (...) VALUES (...) ON CONFLICT(id) DO UPDATE SET ... RETURNING *;Then proceed with
fetch_oneto reliably get the current row.crates/torii/sqlite/src/lib.rs (1)
38-38: Ohayo sensei!#[allow(clippy::too_many_arguments)].
Acceptable if needed, though consider grouping arguments into a builder struct in the future to avoid passing so many parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/sqlite/src/cache.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(9 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/lib.rs (3)
crates/torii/sqlite/src/executor/mod.rs (8) (8)
sqlx(321-321)sqlx(520-520)sqlx(611-611)sqlx(651-651)sqlx(720-720)sqlx(803-803)new(177-179)new(252-278)crates/torii/sqlite/src/model.rs (1) (1)
new(36-70)crates/torii/sqlite/src/utils.rs (2) (2)
felts_to_sql_string(35-38)utc_dt_string_from_timestamp(31-33)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (25)
crates/torii/sqlite/src/cache.rs (5)
2-2: Ohayo sensei, nice Arc usage!
This import is appropriate for synchronous concurrency, well done.
7-12: Ohayo sensei, these imports fit the new caching flow well.
They cleanly provide the types you need for contract interactions. Great job!
157-162: Ohayo sensei, good approach for caching contract classes!
The struct fields make sense. Storing both class hash andContractClasstogether can help avoid repeated lookups.
225-225: Ohayo sensei, avoid.unwrap()for JSON deserialization.
This.unwrap()could panic on malformed JSON. Use a safe error-handling pattern (e.g.?) instead.
242-242: Ohayo sensei, watch out for.unwrap()onget_selector_from_name.
If this function fails on an invalid name, the code will panic. Graceful error handling is preferable.crates/torii/sqlite/src/executor/mod.rs (13)
1-1: Ohayo sensei! New import for HashSet.
This import enables usingHashSetfor handling unique collections, which is beneficial for avoiding duplicates in transaction-related logic.
11-11: Ohayo sensei! IntroducingSqlxTransactionalias.
Renaming or aliasing the transaction type helps clarify the domain-specific usage here. Ensure that all references and doc comments are updated to reflect this new naming.
27-27: Ohayo sensei!ParsedCallimport recognized.
IncludingParsedCallin this import list indicates it will be used downstream for transaction calls. The addition is consistent with your expanded transaction processing logic.
29-29: Ohayo sensei! Utility imports look good.
Bringing infelts_to_sql_stringandfelt_to_sql_stringis consistent with the new transaction calls logic.
54-54: Ohayo sensei! New message variant for transactions.
AddingTransaction(Transaction)toBrokerMessageensures transaction data can be published across the system. LGTM!
108-112: Ohayo sensei! IntroducingStoreTransactionQuery.
This struct captures the contract addresses and parsed calls for a transaction. Structuring them in a dedicated query object is clear and maintainable.
127-127: Ohayo sensei! AddedStoreTransactionenum variant.
This is a natural extension to handle storing transactions. The naming is intuitive, matching the new struct.
152-152: Ohayo sensei! Transaction field usesSqlxTransaction.
Replacing the older naming withSqlxTransactioncan help prevent confusion with different transaction abstractions.
462-462: Ohayo sensei! ImplementingStoreTransactionbranch.
The new branch inhandle_query_messagesystematically processes stored transactions. This integration point looks consistent.
471-480: Ohayo sensei!transaction_contractinsertion logic.
You're inserting contract addresses into thetransaction_contracttable for every relevant transaction. Check that ignoring duplicates is desired—if so, this is fine. Otherwise, considerUPDATEorREPLACEif data might change over time.Would you like me to generate a script to check for existing code references that rely on updating existing rows in
transaction_contract?
482-497: Ohayo sensei!transaction_callsinsertion loop.
Storing each parsed call withINSERT OR IGNOREis consistent. If you want to track call updates (e.g., updatedentrypointorcalldataafter a re-run), consider anON CONFLICTupsert strategy.
499-503: Ohayo sensei! Finalizing the transaction object.
Appendingcontract_addressesandcallsto thetransactionstruct before publishing is a clear approach and ensures a consistent, enrichedBrokerMessage::Transaction.
954-954: Ohayo sensei! Transaction variant broker publish.
Publishing theTransactionvariant completes the pipeline for storing and broadcasting transaction data. Good addition!crates/torii/sqlite/src/lib.rs (7)
13-13: Ohayo sensei! AddingStoreTransactionQueryimport.
This import aligns with the new transaction-handling logic. Looks consistent with the rest of the file’s usage.
15-15: Ohayo sensei!Felttype usage.
We seeFeltis central to StarkNet calls; bringing its symbol into scope here is consistent with the store logic.
18-18: Ohayo sensei!ParsedCallnow in scope.
This newly imported type suggests you’ll parse calls individually. Appropriately used for capturing call details.
541-546: Ohayo sensei! Extended store_transaction parameters.
Adding these parameters (transaction_hash,sender_address, etc.) clarifies the transaction shape. This is a good step toward modularizing the logic.
549-549: Ohayo sensei!transaction_typeparameter.
Allowing injection of the transaction type string might help differentiate behaviors if needed.
551-551: Ohayo sensei! Acceptingcallsslice.
This design paves the way for storing multiple function calls within a single transaction.
553-574: Ohayo sensei! Transaction insertion with new fields.
Storing the extra fields intransactionsfollowed by aStoreTransactionQueryensures the subsequent insertion of calls and contract addresses flows seamlessly in the executor.
glihm
left a comment
glihm
left a comment
There was a problem hiding this comment.
A small comment that could help on performance and reducing the memory overhead of the classes that could be big. And a Dojo project may have bunch of classes.
Not mandatory though, can be optimized later by removing the byte code of classes and focusing only on selectors<>entrypoint name pairs.
| }, | ||
| }; | ||
| let class = self.provider.get_class_at(block_id, contract_address).await?; | ||
| self.classes.write().await.insert(contract_address, (class_hash, class.clone())); |
There was a problem hiding this comment.
If it mostly used for ABI lookup, storing the ABI only instead of the whole class with byte code could result in less memory being used.
Also, if the usage is mostly mapping a selector to a name, the mapping could be done the first time the class is fetched instead of storing the raw ABI.
There was a problem hiding this comment.
Oh nice catch yes!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/sqlite/src/cache.rs (1)
244-252:⚠️ Potential issueRemove unwrap in Legacy ABI selector lookup, sensei.
The Legacy ABI lookup uses
unwrap()on line 246 when getting a selector from a function name, which could panic.- LegacyContractAbiEntry::Function(function) - if get_selector_from_name(&function.name).unwrap() == selector => + LegacyContractAbiEntry::Function(function) + if get_selector_from_name(&function.name).ok() == Some(selector) =>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/sqlite/src/cache.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
crates/torii/sqlite/src/cache.rs (7)
158-162: Ohayo, sensei! Clean enum definition for ClassAbi.This enum provides a nice separation between Sierra and Legacy ABI types, making it easier to handle different contract classes consistently.
164-168: Good use of generics and thread-safe caching, sensei!The
ContractClassCachedesign with generics over a Provider type and RwLock for thread-safety is appropriate. The struct stores only the class hash and ABI data instead of the entire contract class, which is memory-efficient.
170-173: Simple and clean constructor implementation.The
newfunction properly initializes the cache with an empty HashMap and stores the provider reference.
175-197: Improved error handling for provider operations, sensei!The implementation correctly handles the block not found error by falling back to the pending block. This is a good improvement from the previous implementation that was using
unwrap().
206-206: Consider implementing cache size limits, sensei.The cache currently grows unbounded as new contract classes are accessed. Consider adding a maximum size limit or an LRU eviction policy to prevent potential memory issues.
211-221: Nice recursive function to flatten ABI functions.This helper function efficiently extracts function entries from nested ABI definitions, which is useful for searching across all function definitions later.
223-243: Good entrypoint lookup for Sierra ABI.The implementation correctly handles the lookup of entrypoint names from Sierra ABIs by first finding the matching selector in the entrypoints list and then looking up the function details in the ABI.
| let class = match self.provider.get_class_at(block_id, contract_address).await? { | ||
| ContractClass::Sierra(sierra) => { | ||
| let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi).unwrap(); | ||
| let functions: Vec<AbiEntry> = flatten_abi_funcs_recursive(&abi); | ||
| ClassAbi::Sierra((sierra.entry_points_by_type, functions)) | ||
| } | ||
| ContractClass::Legacy(legacy) => ClassAbi::Legacy(legacy.abi.unwrap_or_default()), | ||
| }; | ||
| self.classes.write().await.insert(contract_address, (class_hash, class.clone())); | ||
| Ok(class) | ||
| } |
There was a problem hiding this comment.
Potential panic with unwrap on JSON parsing, sensei.
There's still an unwrap() on line 200 when parsing the Sierra ABI JSON. This could panic if the ABI is malformed.
- let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi).unwrap();
+ let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi)
+ .map_err(|e| Error::ParseError(ParseError::FromJsonStr(e)))?;📝 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 class = match self.provider.get_class_at(block_id, contract_address).await? { | |
| ContractClass::Sierra(sierra) => { | |
| let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi).unwrap(); | |
| let functions: Vec<AbiEntry> = flatten_abi_funcs_recursive(&abi); | |
| ClassAbi::Sierra((sierra.entry_points_by_type, functions)) | |
| } | |
| ContractClass::Legacy(legacy) => ClassAbi::Legacy(legacy.abi.unwrap_or_default()), | |
| }; | |
| self.classes.write().await.insert(contract_address, (class_hash, class.clone())); | |
| Ok(class) | |
| } | |
| let class = match self.provider.get_class_at(block_id, contract_address).await? { | |
| ContractClass::Sierra(sierra) => { | |
| - let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi).unwrap(); | |
| + let abi: Vec<AbiEntry> = serde_json::from_str(&sierra.abi) | |
| + .map_err(|e| Error::ParseError(ParseError::FromJsonStr(e)))?; | |
| let functions: Vec<AbiEntry> = flatten_abi_funcs_recursive(&abi); | |
| ClassAbi::Sierra((sierra.entry_points_by_type, functions)) | |
| } | |
| ContractClass::Legacy(legacy) => ClassAbi::Legacy(legacy.abi.unwrap_or_default()), | |
| }; | |
| self.classes.write().await.insert(contract_address, (class_hash, class.clone())); | |
| Ok(class) | |
| } |
2 participants
Summary by CodeRabbit