opt(torii-indexer): batch requests contract events & block timestamps reqs - #3130
Conversation
|
Ohayo sensei! WalkthroughThis pull request enhances the event and block timestamp fetching in the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/engine.rs (3)
435-437: Ohayo sensei! Batch event requests are an excellent performance improvement.
However, if partial failures occur (e.g., only one request fails), the current approach returns an error for the entire batch. You might consider a more granular error handling strategy to proceed with successful partial results.Also applies to: 439-445, 447-452
483-484: Ohayo sensei! Mind large block batches.
Pushing one request per block can be large for big ranges. If this list grows significantly, consider splitting into sub-batches for reliability.
490-505: Ohayo sensei! Handling timestamp fetch results is clear.
The match arms return an error for unexpected types, which is good. For partial success, you might consider storing valid block timestamps and logging problematic ones instead of bailing out early.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/indexer/src/engine.rs(4 hunks)examples/spawn-and-move/manifest_dev.json(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (25)
crates/torii/indexer/src/engine.rs (7)
13-13: Ohayo sensei! Great usage of the new imports for batch requests.
They neatly enable the batched fetching of events and blocks, aligning with your new approach. No immediate concerns here.Also applies to: 15-18, 21-21
389-391: Ohayo sensei! The event and request collections look good.
Initializing these vectors here supports your new batching mechanism effectively, and there's no apparent logic gap.
401-405: Ohayo sensei! Initial event page fetch is straightforward.
Looks fine as a first page retrieval. The error propagation via?handles any issues gracefully.
457-457: Ohayo sensei! Tracking block numbers in a HashSet is a neat approach.
Reserving them to request timestamps later is efficient, and no immediate concerns are apparent.Also applies to: 460-461, 468-468
486-488: Ohayo sensei! Excellent check before batching timestamps.
Skipping an empty batch call avoids unnecessary overhead, and the approach flows nicely.
407-410:Details
✅ Verification successful
Ohayo sensei! Verify skipping condition for last processed transaction.
Currently, ifevent.transaction_hash == last_tx, the code continues to skip the rest of the loop for that particular event. Ensure this logic does not inadvertently skip unprocessed events in the same transaction.You can confirm correctness by searching references to
last_txin the codebase:Also applies to: 412-416, 418-419
🏁 Script executed:
#!/bin/bash rg -A 5 "last_tx"Length of output: 3867
Ohayo sensei!
I've verified the usage of the
last_txcondition in the indexer engine. The search results show that the logic incrates/torii/indexer/src/engine.rs(lines 407–410) matches the intended pattern: for each event, if its transaction hash equals the last processed transaction (stored aslast_txincursor_map), then that event is skipped. This behavior is in line with similar patterns we see in the executor module and test files, suggesting that the code is designed to avoid reprocessing events from already completed transactions.Based on the evidence:
- The comparison
if event.transaction_hash == last_txproperly guards against reprocessing.- There’s consistency across modules (e.g., in
crates/torii/sqlite/src/executor/mod.rs) regarding handling of last processed transactions.- No evidence was found that unprocessed events from the same transaction are being inadvertently skipped.
Everything appears to be in order. However, if there is a possibility of having partially processed transactions in the future, consider adding explicit tests to ensure that the skip condition doesn’t hide any edge cases.
421-433:Details
❓ Verification inconclusive
Ohayo sensei! Consider multiple continuation tokens for large event sets.
This logic only enqueues one additional request per contract if a continuation token is encountered, potentially missing subsequent pages. A loop-based approach or repeated batch requests may be needed to handle multiple pages fully.Would you like to confirm how often multiple pages occur in production? If so, we can script-explore logs for repeated continuation tokens.
Ohayo sensei! Confirm Handling of All Event Pages
The inspected snippet in
crates/torii/indexer/src/engine.rs(lines 421-433) enqueues an extra request only when a continuation token is encountered, meaning it handles just a single follow-up page. This may lead to missed pages if there are multiple continuation tokens. To robustly process large event sets, consider refactoring the logic to iterate (or loop) until no further continuation token is returned.Key Points:
- The current implementation appends only one additional request when a continuation token exists.
- For event sets spanning more than two pages, subsequent tokens will not trigger further requests.
- It might be useful to verify production logs to understand how frequently multiple continuation tokens occur.
Please review and confirm if a loop-based approach is feasible or if production data suggests this issue is rare.
examples/spawn-and-move/manifest_dev.json (18)
3-4: Ohayo, sensei! Verify World Identifiers.
The updated"class_hash"and"address"values in the"world"section are new. Please confirm they match your latest deployment records and align with the batching enhancements in the indexer.
1315-1316: Ohayo, sensei! Confirm First Contract Deployment.
The first contract in the"contracts"array now has updated"address"and"class_hash". Double-check that these values reflect your intended deployment and are consistent with your contract indexer.
1669-1670: Ohayo, sensei! Validate Second Contract Identifiers.
The second contract’s updated"address"and"class_hash"indicate a new deployment. Please verify these identifiers are correct and align with your infrastructure expectations.
1863-1864: Ohayo, sensei! Check Third Contract Updates.
The new"address"and"class_hash"for the third contract have been provided. Ensure these values are accurate and consistent with your backend deployment records.
2039-2040: Ohayo, sensei! Review Fourth Contract Details.
The updated contract instance here reflects new identifiers. Please cross-verify the"address"and"class_hash"against your deployment logs to ensure complete accuracy.
2224-2224: Ohayo, sensei! Verify Library Hash Update.
Within the"libraries"section, the"class_hash"has been updated. Confirm that this new hash corresponds to the correct new version of the library you intend to use.
2363-2363: Ohayo, sensei! Confirm Model 'ns-Flatbow' Hash.
The model with tag"ns-Flatbow"now shows an updated"class_hash". Ensure this identifier is correct and reflects the deployed Flatbow model version.
2369-2369: Ohayo, sensei! Validate Model 'ns-Message' Update.
The updated"class_hash"in the"ns-Message"model should be checked against your deployment details to ensure accuracy.
2375-2375: Ohayo, sensei! Check 'ns-MockToken' Model Update.
Please verify that the new"class_hash"for the model tagged"ns-MockToken"is correct and aligns with your recent deployments.
2381-2381: Ohayo, sensei! Verify 'ns-Moves' Model Identifiers.
The"ns-Moves"model now uses an updated"class_hash". Please confirm this value is properly updated and reflects your deployment configuration.
2387-2387: Ohayo, sensei! Double-Check 'ns-PlayerConfig' Hash.
The"ns-PlayerConfig"model has an updated"class_hash". Ensure this new identifier is accurate and consistent with your deployed version.
2393-2393: Ohayo, sensei! Validate 'ns-Position' Model Update.
The model tagged"ns-Position"features a new"class_hash". Please verify that this change is correct and reflects the proper deployment.
2426-2429: Ohayo, sensei! Confirm External ERC721Token Identifiers.
In the"external_contracts"section, the"ERC721Token"instance"Badge"now has updated"class_hash"and"address". Please verify these values against your deployment configurations for external contracts.
3250-3253: Ohayo, sensei! Check 'Bank' Contract Updates.
The"Bank"contract now includes new"class_hash"and"address"values. Verify that these changes are correct and align with your intended contract deployment for the banking system.
3277-3280: Ohayo, sensei! Validate 'GoldToken' Contract Identifiers.
The updated"class_hash"and"address"for the"GoldToken"contract should be confirmed with your deployment details to ensure that the token configuration is accurate.
3771-3774: Ohayo, sensei! Verify 'Rewards' Contract for ERC1155Token.
The"Rewards"contract now features updated identifiers. Please ensure that the new"class_hash"and"address"are accurate to support the rewards system correctly.
4651-4654: Ohayo, sensei! Confirm 'Saloon' Contract Updates.
The"Saloon"contract now shows new"class_hash"and"address"values. It would be wise to cross-check these with your deployment records to maintain consistency in the spawn-and-move example.
4671-4674: Ohayo, sensei! Validate 'WoodToken' Contract Details.
The"WoodToken"contract now has updated identifiers. Please verify that the new"class_hash"and"address"match your latest token deployment information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
474-539: Well-implemented recursive function for event fetching.Excellent implementation of the recursive event fetching function! The function:
- Processes the current batch of requests
- Handles pagination using continuation tokens
- Recursively fetches more events when needed
- Properly checks against the cursor map to avoid processing already-seen events
The recursive approach with proper error handling is elegant and efficient.
I would suggest one minor consideration for very large datasets:
#[async_recursion] async fn fetch_events_recursive( &self, requests: Vec<(Felt, ProviderRequestData)>, cursor_map: &HashMap<Felt, Felt>, ) -> Result<Vec<EmittedEvent>> { if requests.is_empty() { return Ok(Vec::new()); } let mut events = Vec::new(); + // Pre-allocate with a reasonable capacity to avoid frequent reallocations + // events.reserve(requests.len() * self.config.events_chunk_size as usize); let mut next_requests = Vec::new();This optimization could help reduce memory reallocations when dealing with large numbers of events.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (9)
crates/torii/indexer/src/engine.rs (9)
8-8: Added a new dependency for async recursion support.Ohayo, sensei! The
async-recursioncrate is being imported to enable recursive async functions, which will be used later in the new implementation. This is a good addition since it elegantly handles the complexity of recursion in async code.
13-18: New imports for batch request functionality.The new imports are necessary to support the batch request functionality. These types (
GetBlockWithTxHashesRequest,GetEventsRequest,EventFilterWithPage,ResultPageRequest,ProviderRequestData,ProviderResponseData) will be used in the implementation of batched requests for events and block timestamps.Also applies to: 20-20
386-413: Excellent implementation of batch requests for event fetching.Ohayo, sensei! This change replaces the original implementation with a more efficient approach using batch requests. Instead of processing events one by one, the code now:
- Creates initial batch requests for all contracts
- Uses a vector to collect all requests
- Calls a recursive function to fetch events and handle pagination
This is a significant performance improvement as it reduces the number of provider calls.
419-434: Good processing of events to extract block numbers.The code efficiently collects unique block numbers and organizes events by block number and transaction hash. This approach helps in setting up the subsequent batch requests for block timestamps.
437-466: Efficient batching of block timestamp requests.The implementation for batching timestamp requests is well done:
- Creates requests for each unique block number
- Executes all requests in a single batch call
- Properly handles the responses and error cases
This is a significant improvement over fetching timestamps individually for each block.
498-510: Proper handling of cursor state in event processing.The cursor handling logic is well-implemented:
- It retrieves the last processed transaction for each contract
- It skips events that have already been processed
- It only adds new events to the collection
This ensures we don't process duplicate events, which is crucial for idempotent indexing.
512-523: Good pagination handling with continuation tokens.Ohayo, sensei! The code handles pagination properly by:
- Checking for continuation tokens in the response
- Preparing new requests with the continuation token
- Adding these requests to be processed in the next recursive call
This ensures all pages of events are fetched completely.
524-530: Appropriate error handling for unexpected response types.The error handling is thorough and provides a clear error message if an unexpected response type is received from the batch request.
533-538: Elegant recursive continuation of event fetching.The recursive approach to fetch continuation pages is elegant and efficient. It only makes additional calls when necessary (when there are continuation tokens).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/engine.rs (3)
390-409: Consider expanding event filtering logic for potential optimization.
The current loop efficiently creates event filters for each contract. However, if there's a need to further narrow down events by keys, a configurable filter could reduce response sizes and processing overhead.
451-468: Timestamp results handled well; consider more robust fallback.
Currently, the_match branch logs an error and returns. In scenarios involving partial failures, you might wish to continue processing what’s valid, but this is acceptable if a strict all-or-nothing approach is desired.
477-543: Ohayo sensei! Recursive batch fetching is neat, but watch for infinite pages.
Your recursion effectively handles continuation tokens. If an API or contract can keep returning new tokens indefinitely, consider an iterative or more explicit termination check. Otherwise, this is a clean, well-structured batching approach.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
crates/torii/indexer/src/engine.rs (8)
386-386: Ohayo sensei! Vector initialization is straightforward.
No concerns here. Initializing theeventsvector is necessary for collecting fetched events.
388-389: Ohayo sensei! Great clarity with the comment and initialevent_requestscreation.
The comment clearly states the intent, and creating a new vector forevent_requestsis an excellent setup for batch event retrieval.
412-413: Ohayo sensei! Good call on using a recursive batch request.
The call tofetch_events_recursivecentralizes logic for repeated event fetching. Keep an eye on potential unbounded pagination.
415-415: Clear comment helps maintain the code’s readability.
The line describing the event-processing step is concise and beneficial for next maintainers.
418-418: Ohayo sensei! Good use of a set to accumulate block numbers.
Using aHashSeteliminates duplicates when tracking event block references.
426-426: Inserting block numbers ensures consistent data coverage.
Explicitly adding the current block number prevents missing events or timestamps.
436-437: Ohayo sensei! Including the latest block number is a smart move.
This ensures the final processed block is accounted for, covering edge cases where the last block may have valuable data.
439-449: Good approach batching block timestamp requests.
Batching reduces provider calls, improving performance. This design pattern is beneficial for high-volume scenarios.
10caf5b to
1c6f8bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/indexer/src/engine.rs (3)
440-445: Consider adding capacity pre-allocation for timestamp_requests.When creating a vector that you know will have a specific size, pre-allocating the capacity can improve performance by avoiding reallocation as the vector grows.
- let mut timestamp_requests = Vec::new(); + let mut timestamp_requests = Vec::with_capacity(block_numbers.len());
389-390: Consider pre-allocating event_requests vector.Similar to the timestamp requests, pre-allocating capacity for the event_requests vector based on the number of contracts can improve performance.
- let mut event_requests = Vec::new(); + let mut event_requests = Vec::with_capacity(self.contracts.len());
491-492: Use direct iterator collection for efficiency.Instead of creating an intermediate collection, you can directly map and collect in one step.
- let batch_requests: Vec<ProviderRequestData> = - requests.iter().map(|(_, req)| req.clone()).collect(); + let batch_requests = requests.iter().map(|(_, req)| req.clone()).collect::<Vec<_>>();
📜 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 (3)
crates/torii/indexer/Cargo.toml(1 hunks)crates/torii/indexer/src/engine.rs(5 hunks)examples/spawn-and-move/manifest_dev.json(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/indexer/Cargo.toml
- examples/spawn-and-move/manifest_dev.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
crates/torii/indexer/src/engine.rs (9)
8-8: Ohayo! Added the async_recursion crate for better recursion handling.This new dependency is essential for implementing the recursive event fetching approach that follows.
13-18: Great imports for the new batching mechanism, sensei!These additional imports from the Starknet core types are necessary for the batch processing implementation.
20-20: Nicely imported Provider request/response types!These are required for the batched request handling for both events and block timestamps.
386-413: Excellent batch request initialization strategy, sensei!The approach of creating initial batch requests for all contracts and then recursively fetching events is a significant optimization over making individual requests. This should greatly reduce the number of network calls to the provider.
415-438: Clean event processing logic!The code efficiently extracts unique block numbers and organizes transactions in a BTreeMap. Ensuring the latest block number is included is a good safety measure.
439-469: Ohayo! Excellent batching of block timestamp requests!Previously, block timestamps were likely fetched individually. This batched approach for timestamp retrieval significantly reduces network calls. The error handling for unexpected response types is thorough.
477-542: Beautifully implemented recursive event fetching, sensei!The
fetch_events_recursivemethod elegantly handles both the initial batch requests and any subsequent continuation token-based requests. This approach efficiently manages pagination while keeping the code clean and maintainable.Some highlights:
- Proper handling of continuation tokens
- Effective filtering of events based on cursor map
- Recursive extension for additional pages
- Good error handling for unexpected response types
668-669: Minor update to use block timestamp from map.Updated the code to correctly retrieve the timestamp for the latest block from the blocks map.
487-492: Monitor memory usage with large event sets, sensei.When dealing with potentially large numbers of events across many contracts, be mindful of memory consumption. If this becomes an issue in production, consider implementing a streaming approach or processing events in smaller chunks.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3130 +/- ##
==========================================
- Coverage 55.74% 55.72% -0.03%
==========================================
Files 443 443
Lines 62750 62783 +33
==========================================
+ Hits 34983 34987 +4
- Misses 27767 27796 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ContractType::WORLD => fetch_all_events_tasks.push_front(token_events_pages), | ||
| _ => fetch_all_events_tasks.push_back(token_events_pages), | ||
| } | ||
| event_requests.push(( |
There was a problem hiding this comment.
There's no limit on how many requests can be batched? What about the size of the response? Since batched requests are all executed then only one result sent, wondering at which point this could affect Katana settings to ensure the fetch can be done without too large entities?
There was a problem hiding this comment.
Uh not sure, I guess it's also the point of batching them when having too many requests since if we were did 1 request = get events, the overhead is quite massive between the request and response timeframes. This will also all depend on the blocks_chunk_size, if you have a 10240 blocks chunk size then it will result in as many get events as needed to fetch all event pages for those blocks. I have tried it on pathfinder and quickly on katana but not sure on the behaviour under high load / katanas handling of batched requests.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores