refactor(torii-indexer): index only pending txns with specific contract - #3135
Conversation
WalkthroughOhayo, sensei! This PR enhances the event processing logic within the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/engine.rs
[error] 731-731: Rust formatting check failed. Missing opening brace on the line following the conditional statement.
🔇 Additional comments (2)
crates/torii/indexer/src/engine.rs (2)
710-711: Ohayo, sensei! Kudos for improving code clarity!Nice addition of this explanatory comment that makes the purpose of the subsequent check more explicit. This helps other developers understand why certain events are being skipped.
719-720: Type annotation enhances code readability, sensei!Explicitly specifying the
Stringtype forevent_idimproves clarity, though the compiler would have inferred it correctly from theformat!macro. This is a good practice for important variables like identifiers.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3135 +/- ##
==========================================
- Coverage 55.74% 55.68% -0.07%
==========================================
Files 443 443
Lines 62750 62825 +75
==========================================
- Hits 34983 34981 -2
- Misses 27767 27844 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/torii/indexer/src/processors/store_transaction.rs (2)
47-55: Consider using an enum for transaction_type.Storing the
transaction_typeas a&'static strmight limit extensibility if more transaction types are added later. Anenumcould help ensure type safety and maintainability.
1-300: Formatting warnings detected.Pipeline logs show multiple formatting issues (e.g., lines 100, 123, 147, 179, 191, 216, 228, 237, 263). Please run
cargo fmtorrustfmtto resolve them.🧰 Tools
🪛 GitHub Actions: ci
[warning] 100-100: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 123-123: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 147-147: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 179-179: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 191-191: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 216-216: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 228-228: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 237-237: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 263-263: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
📜 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 context used
🧬 Code Definitions (1)
crates/torii/indexer/src/processors/store_transaction.rs (1)
crates/torii/sqlite/src/cache.rs (1)
get_entrypoint_name_from_class(235-265)
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/store_transaction.rs
[warning] 100-100: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 123-123: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 147-147: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 179-179: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 191-191: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 216-216: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 228-228: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 237-237: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 263-263: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
🔇 Additional comments (6)
crates/torii/indexer/src/processors/store_transaction.rs (6)
3-3: Ohayo sensei! This new import is fine.Using
anyhow::Resultis consistent with the rest of the codebase for error handling.
60-60: Great addition of the impl block.Defining methods under
StoreTransactionProcessorimproves code organization.
61-92: Verify omitted fields for V3 & confirm ignoring other transaction types is intentional.
- For V3, you currently set
max_feetoFelt::ZERO. IfInvokeTransaction::V3supports non-zeromax_fee, consider capturing it to maintain data integrity.- You return
Nonefor other transaction variants likeDeclare,Deploy, orDeployAccount. Double-check if you intend to skip them.Would you like to handle these scenarios, sensei, or keep the current approach?
94-115: Looks good to me.This parsing logic cleanly retrieves the contract class, retrieves the entrypoint name, and populates a
ParsedCall. Straightforward, ohayo sensei!🧰 Tools
🪛 GitHub Actions: ci
[warning] 100-100: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
204-246: Clean and modular approach to parsing execute calls.This method nicely aggregates calls and any outside calls for processing. Good job maintaining clarity, sensei!
🧰 Tools
🪛 GitHub Actions: ci
[warning] 216-216: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 228-228: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 237-237: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
249-295: Request confirmation on skipping other transactions & empty call lists.
- Currently, only
INVOKEtransactions parse execute calls. Others (e.g., L1 handlers) skip the parse logic. Please confirm if that’s intentional.- When
ExecuteTransactionorLegacyExecuteTransactionparsing fails, we store an empty calls vector. Verify if partial transaction info should still be stored or if an error is more appropriate.🧰 Tools
🪛 GitHub Actions: ci
[warning] 263-263: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
| async fn process_outside_calls<P: Provider + Send + Sync + std::fmt::Debug>( | ||
| contract_class_cache: &ContractClassCache<P>, | ||
| call: &ParsedCall, | ||
| ) -> Result<Vec<ParsedCall>> { | ||
| let mut outside_calls = Vec::new(); | ||
|
|
||
| match call.entrypoint.as_str() { | ||
| "execute_from_outside_v3" => { | ||
| let outside_calls_len: usize = call.calldata[5].try_into().unwrap(); | ||
| for _ in 0..outside_calls_len { | ||
| let outside_call = Self::parse_outside_call( | ||
| contract_class_cache, | ||
| &call.calldata, | ||
| 6, | ||
| call.contract_address, | ||
| ).await?; | ||
| outside_calls.push(outside_call); | ||
| } | ||
| } | ||
| "execute_from_outside_v2" => { | ||
| let outside_calls_len: usize = call.calldata[4].try_into().unwrap(); | ||
| for _ in 0..outside_calls_len { | ||
| let outside_call = Self::parse_outside_call( | ||
| contract_class_cache, | ||
| &call.calldata, | ||
| 5, | ||
| call.contract_address, | ||
| ).await?; | ||
| outside_calls.push(outside_call); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| Ok(outside_calls) | ||
| } |
There was a problem hiding this comment.
Confirm multiple calls are not parsed from the same offset.
For each iteration, base_offset remains constant. This leads to identical slices unless the calls are truly all at the same position. Verify if this is correct or if the offset needs incrementing on each pass.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 179-179: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
[warning] 191-191: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
| async fn parse_legacy_execute_call<P: Provider + Send + Sync + std::fmt::Debug>( | ||
| contract_class_cache: &ContractClassCache<P>, | ||
| call: &LegacyExecuteCall, | ||
| full_calldata: &[Felt], | ||
| caller_address: Felt, | ||
| call_type: CallType, | ||
| ) -> Result<ParsedCall> { | ||
| let contract_class = contract_class_cache | ||
| .get(call.contract_address, BlockId::Tag(BlockTag::Pending)) | ||
| .await?; | ||
|
|
||
| let entrypoint = get_entrypoint_name_from_class(&contract_class, call.selector) | ||
| .unwrap_or(format!("{:#x}", call.selector)); | ||
|
|
||
| Ok(ParsedCall { | ||
| contract_address: call.contract_address, | ||
| entrypoint, | ||
| calldata: full_calldata[call.data_offset..call.data_offset + call.data_length].to_vec(), | ||
| call_type, | ||
| caller_address, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard against potential out-of-range slicing.
Accessing full_calldata[call.data_offset..call.data_offset + call.data_length] can panic if the slice boundaries are invalid. Consider validating the bounds before slicing to avoid runtime errors.
fn parse_legacy_execute_call(...) -> Result<ParsedCall> {
...
- calldata: full_calldata[call.data_offset..call.data_offset + call.data_length].to_vec(),
+ let end = call.data_offset.saturating_add(call.data_length);
+ if end > full_calldata.len() {
+ return Err(anyhow!("Out-of-range calldata slicing"));
+ }
+ calldata: full_calldata[call.data_offset..end].to_vec(),
...
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 123-123: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
| async fn parse_outside_call<P: Provider + Send + Sync + std::fmt::Debug>( | ||
| contract_class_cache: &ContractClassCache<P>, | ||
| calldata: &[Felt], | ||
| base_offset: usize, | ||
| caller_address: Felt, | ||
| ) -> Result<ParsedCall> { | ||
| let to_offset = base_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 contract_address = calldata[to_offset]; | ||
|
|
||
| let contract_class = contract_class_cache | ||
| .get(contract_address, BlockId::Tag(BlockTag::Pending)) | ||
| .await?; | ||
|
|
||
| let entrypoint = get_entrypoint_name_from_class(&contract_class, calldata[selector_offset]) | ||
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | ||
|
|
||
| Ok(ParsedCall { | ||
| contract_address, | ||
| entrypoint, | ||
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | ||
| call_type: CallType::ExecuteFromOutside, | ||
| caller_address, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Prevent panic from unchecked indexes and try_into().unwrap().
Indices like calldata[selector_offset + 1] risk out-of-bounds. Also, try_into().unwrap() can panic if the Felt value doesn't fit a usize. Both cases should be handled gracefully to avoid crashes.
let calldata_len: usize = match calldata.get(selector_offset + 1) {
- Some(val) => val.try_into().unwrap(),
- None => return Err(anyhow!("Missing length value in calldata")),
+ Some(val) => val.try_into().map_err(|_| anyhow!("Felt does not fit in usize"))?,
+ None => return Err(anyhow!("Missing length value in calldata")),
};
-Ok(ParsedCall {
- calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(),
- ...
-})
+if calldata_offset + calldata_len > calldata.len() {
+ return Err(anyhow!("Out-of-bounds for outside call calldata"));
+}📝 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.
| async fn parse_outside_call<P: Provider + Send + Sync + std::fmt::Debug>( | |
| contract_class_cache: &ContractClassCache<P>, | |
| calldata: &[Felt], | |
| base_offset: usize, | |
| caller_address: Felt, | |
| ) -> Result<ParsedCall> { | |
| let to_offset = base_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 contract_address = calldata[to_offset]; | |
| let contract_class = contract_class_cache | |
| .get(contract_address, BlockId::Tag(BlockTag::Pending)) | |
| .await?; | |
| let entrypoint = get_entrypoint_name_from_class(&contract_class, calldata[selector_offset]) | |
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | |
| Ok(ParsedCall { | |
| contract_address, | |
| entrypoint, | |
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | |
| call_type: CallType::ExecuteFromOutside, | |
| caller_address, | |
| }) | |
| } | |
| async fn parse_outside_call<P: Provider + Send + Sync + std::fmt::Debug>( | |
| contract_class_cache: &ContractClassCache<P>, | |
| calldata: &[Felt], | |
| base_offset: usize, | |
| caller_address: Felt, | |
| ) -> Result<ParsedCall> { | |
| let to_offset = base_offset; | |
| let selector_offset = to_offset + 1; | |
| let calldata_offset = selector_offset + 2; | |
| let calldata_len: usize = match calldata.get(selector_offset + 1) { | |
| Some(val) => val.try_into().map_err(|_| anyhow!("Felt does not fit in usize"))?, | |
| None => return Err(anyhow!("Missing length value in calldata")), | |
| }; | |
| let contract_address = calldata[to_offset]; | |
| let contract_class = contract_class_cache | |
| .get(contract_address, BlockId::Tag(BlockTag::Pending)) | |
| .await?; | |
| let entrypoint = get_entrypoint_name_from_class(&contract_class, calldata[selector_offset]) | |
| .unwrap_or(format!("{:#x}", calldata[selector_offset])); | |
| if calldata_offset + calldata_len > calldata.len() { | |
| return Err(anyhow!("Out-of-bounds for outside call calldata")); | |
| } | |
| Ok(ParsedCall { | |
| contract_address, | |
| entrypoint, | |
| calldata: calldata[calldata_offset..calldata_offset + calldata_len].to_vec(), | |
| call_type: CallType::ExecuteFromOutside, | |
| caller_address, | |
| }) | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[warning] 147-147: Formatting issues detected in the file. Please run 'rustfmt' to format the code.
glihm
changed the title
2 participants
Summary by CodeRabbit