opt(torii-indexer): reduce get_events reqs & batch transactions - #3136
Conversation
|
Ohayo sensei! Here’s the detailed summary of the changes: WalkthroughThe changes involve significant modifications to the event processing logic 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: 1
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
371-378: Ohayo sensei! Fetching all events at once for a large block range might risk high memory consumption. Consider a more streaming-oriented approach or chunking the fetch requests themselves to keep memory usage in check.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (19)
crates/torii/indexer/src/engine.rs (19)
188-195: Ohayo sensei! This newEventChunkstruct is a neat addition. DerivingDebugis helpful for troubleshooting, but consider also deriving traits likeCloneorPartialEqif you need to reuse or compare these chunks in tests and beyond.
198-198: Ohayo sensei! Transitioning from a singleFetchRangeResultto a collection ofEventChunkinstances underRange(Vec<EventChunk>)is a clear, coherent step in your chunk-based architecture.
206-212: Ohayo sensei! ReturningNonewhen no chunks exist prevents downstream errors. Good job ensuring boundary conditions don’t cause panics.
395-396: Ohayo sensei! The newfetch_all_eventsmethod name accurately reflects its behavior, improving readability.
402-402: Ohayo sensei! The updated comment about fetching in parallel clarifies how all contract events are retrieved. Great documentation detail.
422-422: Ohayo sensei! Instantiatingblock_setas aHashSetfor block tracking looks tidy and straightforward, no issues here.
458-458: Ohayo sensei! This comment neatly illustrates the mechanism for retrieving timestamps in parallel.
461-461: Ohayo sensei! Initializingblocksas aBTreeMapis a solid choice to maintain an ordered map for block timestamps.
497-530: Ohayo sensei! Thechunk_eventsfunction effectively segments blocks into chunks. This modularization will be helpful for large data sets.
563-563: Ohayo sensei! CheckingFetchDataResult::Range(chunks)here aligns well with the chunk-based design—smooth integration.
564-567: Ohayo sensei! Iterating through chunks inprocessfosters a clean separation of concerns. Nicely done.
621-621: Ohayo sensei! Introducingprocess_chunkis key to chunk-level operations. This method name is concise and complements the new approach.
625-625: Ohayo sensei! Clear comments about processing transactions within each chunk improve readability. Good job.
641-641: Ohayo sensei! Passing the optionaltransactionparameter is consistent with the chunk-based design. No concerns here.
663-663: Ohayo sensei! Accessing the block timestamp with.get(&chunk.to)is a neat fallback approach for chunk finalization logic.
665-665: Ohayo sensei! Gracefully falling back to pull the timestamp from the provider is a thoughtful safeguard when no timestamp is found in the chunk.
667-667: Ohayo sensei! Updating the cursors with the chunk’s ending block ensures the engine won’t re-process old data. Nice finishing touch.
450-452:Details
❓ Verification inconclusive
Ohayo sensei! Inserting the block number into
block_setonly if it's present is safe, but please verify ifevent.block_numbermight ever beNonein real scenarios. If so, add logic to handle or log these cases.
🏁 Script executed:
#!/bin/bash # Searching across code to see if block_number can be absent or handled differently rg -A 5 'block_number\s*=\s*None'Length of output: 35
Ohayo sensei!
Our search didn't reveal any explicit assignments whereevent.block_numberis set toNone. However, this string search may not fully capture all scenarios, especially if the value is determined dynamically at runtime. Please verify—by reviewing the source or through runtime logs—whether there are any cases whereevent.block_numbermight be absent. If such scenarios exist, consider adding a logging or handling branch within this code block incrates/torii/indexer/src/engine.rs(Lines 450–452).
640-640:Details
❓ Verification inconclusive
Ohayo sensei! Referencing
chunk.blocks[&block_number]directly can cause a panic if the block’s timestamp is missing in rare edges. Verify that all transaction blocks are guaranteed to exist before indexing.
🏁 Script executed:
#!/bin/bash # Examine chunk.blocks usage to ensure no missing timestamps for block references rg -A 3 "chunk\.blocks\["Length of output: 612
Ohayo sensei! Our search confirms that we’re still directly indexing into
chunk.blocks(e.g., in the call toself.process_block(block_number, chunk.blocks[&block_number]).await?), which means there’s a potential risk of a panic if the corresponding transaction block (and its timestamp) isn’t present. Could you please verify that the code guarantees all transaction blocks are inserted intochunk.blocksbefore this access occurs? If there’s any chance a block might be missing, consider adding a safety check (or using a safe lookup method like.get()) to prevent potential panics.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
crates/torii/indexer/src/engine.rs (1)
188-203:⚠️ Potential issueOhayo sensei! The PR summary mentions a change to FetchDataResult but it's not implemented yet.
The AI summary mentions that
FetchDataResult::Rangeshould now store a collection of event chunks (Vec<EventChunk>), but the code still shows the originalFetchRangeResultstructure. Make sure to update this enum to reflect the chunking approach.#[derive(Debug)] pub enum FetchDataResult { - Range(FetchRangeResult), + Range(Vec<EventChunk>), Pending(FetchPendingResult), None, } impl FetchDataResult { pub fn block_id(&self) -> Option<BlockId> { match self { - FetchDataResult::Range(range) => Some(BlockId::Number(range.latest_block_number)), + FetchDataResult::Range(chunks) => chunks.last().map(|chunk| BlockId::Number(chunk.to)), FetchDataResult::Pending(_pending) => Some(BlockId::Tag(BlockTag::Pending)), FetchDataResult::None => None, } } }
🧹 Nitpick comments (2)
crates/torii/indexer/src/engine.rs (2)
435-439: Simplify event block number handling.This code has changed to first check if a block number exists before inserting it into the block set.
You can simplify this with Rust's
if letpattern:- if let Some(block_number) = event.block_number { - block_set.insert(block_number); - } + if let Some(block_number) = event.block_number { + block_set.insert(block_number); + }The change looks good though the if-let pattern was already being used correctly here.
608-615: Ohayo sensei! Handle potential KeyError with Option::and_then.The code retrieves the timestamp for the latest block by directly accessing the blocks map with
range.latest_block_number. Although you have a fallback withunwrap_or, a more idiomatic approach would usegetand handle the Option explicitly.- let last_block_timestamp = range - .blocks - .get(&range.latest_block_number) - .copied() - .unwrap_or(get_block_timestamp(&self.provider, range.latest_block_number).await?); + let last_block_timestamp = match range.blocks.get(&range.latest_block_number) { + Some(×tamp) => timestamp, + None => get_block_timestamp(&self.provider, range.latest_block_number).await? + };🧰 Tools
🪛 GitHub Actions: ci
[error] 611-611: 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 (1)
crates/torii/indexer/src/engine.rs(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/engine.rs
[error] 185-185: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 356-356: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 392-392: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 476-476: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 611-611: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 884-884: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (3)
crates/torii/indexer/src/engine.rs (3)
464-475: Ohayo sensei! Good organization of events by block and transaction.This new block of code improves how events are organized by structuring them hierarchically by block number and transaction hash, which aligns with the PR objective to optimize event fetching and processing.
479-480: Smart improvement in determining latest_block_number.You're now determining
latest_block_numberfrom the maximum block in the fetched data, falling back tolast_block_numberif no blocks were fetched. This is better than returning a hardcoded 0 value.
879-907:Details
✅ Verification successful
Improved early termination logic for get_all_events.
The function now stops fetching events when the last block number in a page exceeds the target block number, which is more efficient than fetching all events regardless of block numbers.
🏁 Script executed:
#!/bin/bash # Verify that the early termination logic works as expected by checking if get_events is called with continuation tokens rg -n --context=5 "continuation_token" crates/torii/indexer/src/engine.rsLength of output: 1021
Ohayo, sensei!
The early termination logic in
get_all_eventshas been verified. The code correctly updates and checks thecontinuation_tokenand stops fetching events either when no token is available or when the last event's block number meets/exceeds the target (to). The debug output and usage ofcontinuation_token—as confirmed by thergcommand output—indicate that everything is functioning as intended.
- File:
crates/torii/indexer/src/engine.rs(Lines ~889-905)- Key Logic: Stops fetching further events when
continuation_token.is_none()orlast_block_number >= to.Approved as is.
🧰 Tools
🪛 GitHub Actions: ci
[error] 884-884: Rust formatting check failed. Please run 'rustfmt' to format the code.
|
|
||
| // Fetch all events from 'from' to our blocks chunk size | ||
| let range = self.fetch_range(from, &cursors.cursor_map, latest_block.block_number).await?; | ||
| debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.latest_block_number, "Fetched data for range."); | ||
|
|
||
| FetchDataResult::Range(range) |
There was a problem hiding this comment.
Missing the new EventChunk struct implementation.
According to the PR summary, you're introducing a new EventChunk struct to encapsulate events, transactions, and blocks. The struct implementation is missing in this code segment.
Define the EventChunk struct before using it in the FetchDataResult enum. This struct should encapsulate a range of events with their associated transactions and blocks.
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
387-401: Be cautious with unbounded event fetchingThe event filter now uses
to_block: None, which could potentially fetch a large number of events if there's significant activity on the contract. While this can be more efficient for small to medium-sized chunks, it might cause memory issues for contracts with a high volume of events.Consider adding a safeguard to limit the maximum number of events that can be fetched in a single call:
let events_filter = EventFilter { from_block: Some(BlockId::Number(from)), - to_block: None, + to_block: Some(BlockId::Number(std::cmp::min(from + self.config.blocks_chunk_size, last_block_number))), address: Some(*contract.0), keys: None, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
crates/torii/indexer/src/engine.rs (7)
355-362: Ohayo sensei! Improved event fetching strategyYou've simplified the
fetch_datamethod to retrieve all events in a single call tofetch_range, which should reduce the number of network requests. This is a good optimization.
379-384: Signature update for fetch_rangeNice update to the signature! Changing from a
toparameter tolast_block_numbermakes the function purpose clearer.
410-413: Ohayo sensei! Nice parallel processing implementationUsing a semaphore to manage concurrent tasks when fetching block timestamps is a great approach to control resource usage while improving performance!
447-470: Improved event processingThe refactored code handles events more efficiently by checking block numbers and organizing events by block and transaction. The parallel fetching of block timestamps further improves performance.
483-485: Proper latest block number determinationYou're now correctly determining the latest block number from the blocks map when available, falling back to the provided
last_block_number. This fixes the issue mentioned in previous reviews.
573-625: Process_range now correctly handles the new data structureThe process_range method has been updated to correctly process the new data structure, working with the transactions organized by block numbers. This looks good!
911-915: Optimized loop breaking conditionThe condition to break the loop now checks if the last block number of fetched events exceeds the target block number, which helps prevent unnecessary fetches.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
410-464: Ohayo sensei! Excellent implementation of parallel block timestamp fetchingThe use of a semaphore to limit concurrent requests while still parallelizing timestamp fetching is a good performance optimization. This avoids overwhelming the provider while still improving throughput.
However, note that the initial value of 0 for block timestamps could be misleading if used before the async fetching completes.
You might want to add a comment explaining that the 0 value is temporary until the async fetch completes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(12 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/indexer/src/engine.rs (4)
crates/torii/sqlite/src/types.rs (1)
from(15-17)crates/torii/sqlite/src/utils.rs (2)
from(244-246)from(250-252)crates/torii/indexer/src/task_manager.rs (1)
new(43-58)crates/torii/indexer/src/processors/mod.rs (1)
event(55-55)
🔇 Additional comments (9)
crates/torii/indexer/src/engine.rs (9)
21-21: Ohayo sensei! Good addition of Semaphore for concurrency control.Adding the Semaphore import is a nice enhancement that will help control the number of concurrent operations when fetching block timestamps.
37-40: Adding support for ERC1155 and ERC4906 processors. Nice work!The inclusion of these processors expands the indexer's capability to handle more token standards.
355-362: Simplified event fetching approach looks cleanerThe refactored code now fetches all events in a single call up to the latest block number, which should reduce the number of provider calls. This is a good optimization.
379-384: Method signature updated to better reflect the new chunking approachChanging from a specific range to a
last_block_numberparameter makes the intention clearer. Theto_block: Noneapproach in the implementation will fetch events up to the latest block.
465-471: Clean data structure organization for the transactions mapThe nested BTreeMap and LinkedHashMap structure provides a good organization for the transaction data, ensuring events are associated with their blocks and transactions properly.
485-487: Good fallback value for latest_block_numberUsing the last processed block number or falling back to the provided
last_block_numberis a good approach to ensure we always have a meaningful value.
615-619: Good use of safer map access for last_block_timestampUsing
getandcopiedto safely access the timestamp with a fallback to fetching it is a better approach than direct indexing. Good job!
914-916: Proper chunking limit in get_all_eventsThe loop condition
last_block_number >= toensures we don't fetch events beyond the requested block range, which is important for efficiency.
391-392:Details
❓ Verification inconclusive
Potential concern with unbounded event fetching
Setting
to_block: Nonemeans the provider might fetch all events up to the most recent block. While you do enforce a limit inget_all_events, consider whether this approach might fetch more data than necessary in certain scenarios.
🏁 Script executed:
#!/bin/bash # Check if there are similar patterns in other parts of the codebase where to_block is limited rg "to_block: (Some|None)" -A 5 -B 5Length of output: 13187
Ohayo sensei,
In reviewing the code in
crates/torii/indexer/src/engine.rs(lines 391-392), I noticed the filter is set as follows:to_block: None, address: Some(*contract.0),This setup allows the provider to potentially fetch all events up to the latest block—even though the
get_all_eventsfunction does enforce a limit on the number of events processed. In contrast, other parts of our codebase (for example, incrates/dojo/world/src/remote/events_to_remote.rs) explicitly specify block numbers to bound the data fetching.Action items:
- Assess Performance Impact: Confirm that using
to_block: Nonehere is an intentional design choice, and that the limit enforced inget_all_eventsis sufficient to avoid unnecessary data loads.- Consistency Check: Consider aligning this behavior with similar modules where a specific block range is provided, especially if that could help in predicting performance and resource usage.
Please review whether the unbounded fetch is acceptable in this context or if it might lead to fetching more data than necessary under certain scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/indexer/src/engine.rs (2)
591-592:⚠️ Potential issuePotential panic if block_number doesn't exist in the blocks map.
Accessing
range.blocks[&block_number]will panic if the block number is not in the map. Consider using a safer access method.- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or(&0),
604-604:⚠️ Potential issueSame potential panic issue with block map access.
Similar to the previous comment, direct indexing can panic if the key doesn't exist.
- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or(&0)).await?;
🧹 Nitpick comments (2)
crates/torii/indexer/src/engine.rs (2)
382-387: Ohayo sensei! Consider renaming fetch_range for clarity.The function signature has changed and the behavior is different (fetching without to_block), but the name remains the same. This could cause confusion for maintainers.
- async fn fetch_range( + async fn fetch_events_chunk( &self, from: u64, to: u64, cursor_map: &HashMap<Felt, Felt>, ) -> Result<FetchRangeResult> {
394-394: Setting to_block to None could fetch excess data.Setting
to_block: Nonemeans the filter will fetch events up to the latest block, potentially more than needed. This is mitigated by your continuation check later, but could be more explicit.- to_block: None, + to_block: Some(BlockId::Number(to)),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(12 hunks)
🔇 Additional comments (12)
crates/torii/indexer/src/engine.rs (12)
21-21: Ohayo sensei! Great addition of Semaphore for concurrent task management.Adding the Semaphore import is a good approach for limiting concurrency when fetching block timestamps in parallel.
37-40: Nice addition of ERC1155 and ERC4906 processor imports!These new imports will help handle additional token standards. While not directly related to the optimization work in this PR, it's a good enhancement of the indexer's capabilities.
198-201: Improved block_id retrieval approach.Using
last_key_value()on the BTreeMap is more robust than the previous approach since BTreeMap guarantees ordering. This ensures you're always getting the latest block processed.
210-215: Clear documentation of data structures.The improved comments for the
FetchRangeResultstruct make it easier to understand the purpose of each field.
355-356: Nicely simplified block range calculation!The calculation of the 'to' block has been simplified for better readability while maintaining the same logic.
361-365: Good simplification of fetch_range call.The code now more clearly expresses the intent to fetch events in chunks and the result handling is more straightforward.
409-413: Excellent use of semaphore for parallel fetch limiting!The semaphore implementation for limiting concurrent timestamp fetches is a great optimization that will prevent overwhelming the provider.
446-471: Ohayo sensei! Well-structured parallel timestamp fetching logic.This is a significant improvement in how block timestamps are fetched. By doing it asynchronously and using a semaphore to limit concurrency, you're optimizing both throughput and resource usage.
476-479: Efficient handling of timestamp results.The code nicely collects all the timestamp results from the parallel fetches and updates the blocks map.
612-617: Safer retrieval of last block timestamp.Good improvement using
get()to safely retrieve the timestamp with a fallback to fetching it if needed. This prevents potential panics.
886-886: Parameter name change improves clarity.Changing
limit_block_numbertotomakes the function more consistent with other parts of the code.
907-907: Smart optimization of continuation check.Stopping the loop when either the token is None or the last block number exceeds the target is an efficient way to avoid fetching unnecessary data.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/torii/indexer/src/engine.rs (2)
592-593:⚠️ Potential issueAvoid potential panic with direct map access
Directly indexing into the blocks map with
range.blocks[&block_number]will panic if the block number is not in the map.- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or_else(|| { + warn!(target: LOG_TARGET, "Block timestamp not found for block {}", block_number); + &0 + }),
605-605:⚠️ Potential issueSame potential panic issue with blocks map access
Similar to the previous comment, direct indexing can panic if the key doesn't exist.
- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or_else(|| { + warn!(target: LOG_TARGET, "Block timestamp not found for block {}", block_number); + &0 + })).await?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/indexer/src/engine.rs(12 hunks)crates/torii/indexer/src/processors/controller.rs(1 hunks)crates/torii/indexer/src/processors/erc20_legacy_transfer.rs(1 hunks)crates/torii/indexer/src/processors/erc20_transfer.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/torii/indexer/src/processors/controller.rs
- crates/torii/indexer/src/processors/erc20_transfer.rs
- crates/torii/indexer/src/processors/erc20_legacy_transfer.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/engine.rs
[warning] 18-18: Rust formatting issues detected. Please run 'rustfmt' to format the code.
[warning] 30-30: Rust formatting issues detected. Please run 'rustfmt' to format the code.
[warning] 361-361: Rust formatting issues detected. Please run 'rustfmt' to format the code.
[warning] 445-445: Rust formatting issues detected. Please run 'rustfmt' to format the code.
🔇 Additional comments (13)
crates/torii/indexer/src/engine.rs (13)
21-21: Ohayo sensei! Good addition of Semaphore for concurrency control.Adding the Semaphore from tokio allows you to properly limit the number of concurrent block timestamp fetches later in the code, which is a good practice for managing system resources.
25-25: Smart import optimization!Good move adding Instant alongside the existing sleep import for measuring duration of operations.
37-40: Adding support for ERC1155 and ERC4906 processors!These new imports expand the indexer's capabilities to handle multi-token standard and metadata update events.
198-201: Elegant way to get the latest block number!Using
last_key_value()on the BTreeMap is a clean approach to retrieve the highest block number.
210-213: Much clearer documentation, sensei!These improved comments help clarify the data structure relationships between blocks, transactions, and events.
356-367: Streamlined fetch_data implementation!The changes simplify the block range calculation and better handle logging of the actual range fetched.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 361-361: Rust formatting issues detected. Please run 'rustfmt' to format the code.
410-484: Ohayo! Excellent parallel block timestamp fetching implementation!Your implementation of parallel timestamp fetching with proper concurrency control is a significant improvement:
- Using a semaphore to limit concurrent operations
- Using JoinSet for managing async tasks
- Optimizing by fetching timestamps only once per block
- Using trace logging to provide visibility into the operation
This change should significantly improve performance when processing large numbers of events across many blocks.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 445-445: Rust formatting issues detected. Please run 'rustfmt' to format the code.
519-519: Process range method visibility updatedThe
process_rangemethod is now private, which aligns with its role as an internal implementation detail.
577-578: Better transaction processing structureThe updated loop structure cleanly iterates through transactions grouped by block number, which aligns with the chunking approach mentioned in the PR objectives.
613-624: Robust cursor update implementation!Good implementation for getting the timestamp with a fallback mechanism if it's not in the blocks map.
910-913: Smart optimization for limiting event fetching!This is the key improvement that fulfills the PR objective. The loop now breaks when:
- There's no continuation token, OR
- The last block number in the fetched events is >= the target block number
This effectively limits event fetching to the requested block range, avoiding unnecessary network calls.
18-18: Fix formatting issues flagged by CIOhayo sensei! There are formatting issues detected by the CI pipeline on these lines.
#!/bin/bash # Run rustfmt to fix formatting issues rustfmt crates/torii/indexer/src/engine.rsAlso applies to: 30-30, 361-361, 445-445
🧰 Tools
🪛 GitHub Actions: ci
[warning] 18-18: Rust formatting issues detected. Please run 'rustfmt' to format the code.
383-383: Rename fetch_range to better match its behaviorThe function still has the name
fetch_rangebut its behavior is closer tofetch_all_eventsas noted in previous reviews.- async fn fetch_range( + async fn fetch_all_events(This would make the code more self-documenting and match the actual implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/torii/indexer/src/engine.rs (1)
415-489: 🛠️ Refactor suggestionWell-implemented parallel block timestamp fetching with Semaphore!
The parallelized fetching of block timestamps with semaphore control is an excellent performance optimization. However, there are two issues to address:
- The default timestamp of 0 used during fetching could be misleading if accessed before the fetch completes
- Direct map access in later code could cause panics
- blocks.entry(block_number).or_insert_with(|| { + if !blocks.contains_key(&block_number) { let semaphore = semaphore.clone(); let provider = self.provider.clone(); set.spawn(async move { let _permit = semaphore.acquire().await.unwrap(); trace!( target: LOG_TARGET, "Fetching block timestamp for block number: {}", block_number ); let block_timestamp = get_block_timestamp(&provider, block_number).await?; Ok((block_number, block_timestamp)) }); - 0 + blocks.insert(block_number, 0); + }Also consider using
.get(&block_number)instead of direct indexing in the rest of the code to avoid panics.
♻️ Duplicate comments (2)
crates/torii/indexer/src/engine.rs (2)
597-598:⚠️ Potential issueOhayo sensei! Potential panic if block_number doesn't exist in the blocks map
Accessing
range.blocks[&block_number]will panic if the block number is not in the map. Consider using a safer access method.- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or(&0),
610-610:⚠️ Potential issueSame potential panic issue with block map access
Similar to the previous comment, direct indexing can panic if the key doesn't exist.
- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or(&0)).await?;
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
524-524: Consider renaming process_range to process_chunk for clarityAccording to previous comments,
process_rangeshould be renamed toprocess_chunkto better reflect the purpose of processing event chunks rather than ranges.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(14 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/indexer/src/engine.rs (7)
crates/torii/sqlite/src/executor/mod.rs (4)
None(301-301)None(851-851)new(167-169)new(242-268)crates/torii/sqlite/src/types.rs (1)
from(15-17)crates/torii/sqlite/src/utils.rs (2)
from(244-246)from(250-252)crates/torii/sqlite/src/lib.rs (3)
cursors(197-227)new(65-72)e(914-918)crates/torii/indexer/src/task_manager.rs (1)
new(43-58)crates/torii/sqlite/src/model.rs (1)
new(36-70)crates/torii/indexer/src/processors/mod.rs (1)
event(55-55)
🪛 GitHub Actions: ci
crates/torii/indexer/src/engine.rs
[error] 18-30: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (7)
crates/torii/indexer/src/engine.rs (7)
21-21: Ohayo sensei! Nice addition of Semaphore for concurrency controlAdding the Semaphore will help manage parallel task execution, which aligns well with the PR's goal of optimizing event fetching in chunks.
37-40: Added support for ERC1155 and ERC4906 processors, nice extension!These additional processors enhance the indexer's capabilities for handling more token standards.
197-201: Clean implementation of FetchDataResult::block_id()Using
last_key_value()is a more idiomatic way to get the last entry in the BTreeMap.
355-358: Improved documentation and clearer block range calculationThe non-inclusive comments clarify the behavior, and the simplified calculation of
tois more straightforward.
387-404: Ohayo sensei! Potential excessive data fetching withto_block: NoneSetting
to_block: Nonein the EventFilter might fetch more events than needed, which contradicts the PR goal of reducing fetches.- to_block: None, + to_block: Some(BlockId::Number(to)),While the downstream
get_all_eventsfunction does handle limiting with thetoparameter, setting a proper boundary in the filter would reduce unnecessary network traffic.
582-584: Process_range now processes transactions by chunk - good improvement!The method has been refactored to process transactions by chunk instead of by block, which aligns with the PR objective.
618-628: Improved cursor update logic with fallback for missing timestampsThe updated cursor logic correctly checks for the latest block timestamp in the blocks map, with a fallback to fetching it if not present.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
crates/torii/indexer/src/engine.rs (4)
597-598:⚠️ Potential issuePotential panic if block_number doesn't exist in the blocks map
Direct indexing into
range.blocks[&block_number]will panic if the block number is not in the map. Consider using a safer access method.- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or(&0),
610-610:⚠️ Potential issueSame potential panic issue with block map access
Similar to the previous comment, direct indexing can panic if the key doesn't exist.
- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or(&0)).await?;
915-920:⚠️ Potential issuePotential bug in last_block_number calculation
Ohayo sensei! If
events_page.eventsis empty,last_block_numberwill be 0, which could cause incorrect loop termination.- let last_block_number = events_page.events.last().map_or(0, |e| e.block_number.unwrap()); + let last_block_number = events_page.events.last() + .map(|e| e.block_number.unwrap_or(0)) + .unwrap_or_else(|| continuation_token.is_some().then_some(to - 1).unwrap_or(to));This ensures we continue fetching if we have a continuation token, even with an empty page.
385-398: 💡 Verification agent🧩 Analysis chain
Verify the absence of to_block doesn't cause excessive data fetching
Setting
to_block: Nonein the EventFilter might fetch more events than needed, potentially causing performance issues. Theget_all_eventsfunction does use thetoparameter to limit results, but setting an explicit boundary in the filter would be better.- to_block: None, + to_block: Some(BlockId::Number(to)),Let's check if the current implementation might lead to excessive fetching:
🏁 Script executed:
#!/bin/bash # Search for to_block usage in get_all_events implementation rg -A 5 $'to_block.*None' --type rustLength of output: 1451
Ohayo, sensei!
I've verified that in
crates/torii/indexer/src/engine.rstheEventFilteris instantiated withto_block: None, relying on thetoparameter passed separately toget_all_eventsto limit the fetched events. While the current behavior appears to work—as confirmed by the search results—this indirect approach can lead to excessive data fetching if the provider doesn’t strictly enforce the external boundary. For clarity and future-proofing, it's advisable to explicitly set the filter's upper bound.
- Location:
crates/torii/indexer/src/engine.rs(lines 385-398)- Proposed change:
- to_block: None, + to_block: Some(BlockId::Number(to)),Please consider applying this change and perform any necessary performance verification to ensure that the modification does not introduce unintended behavior.
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
326-326: Removed error context from query executionsOhayo sensei! The removal of
.with_context()calls from query executions simplifies the code, but might make debugging harder if errors occur. This is a trade-off between code simplicity and detailed error information.Consider whether detailed error contexts are important for troubleshooting in a production environment. If so, you might want to retain some of these contexts, especially for critical operations.
Also applies to: 399-399, 437-437, 508-508, 554-554, 560-560, 611-611, 780-780, 816-816
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/indexer/src/engine.rs(13 hunks)crates/torii/sqlite/src/executor/mod.rs(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
crates/torii/indexer/src/engine.rs (6)
195-205: Good change to block_id() retrieval logic!Ohayo sensei! The update to derive the block ID from the
blocksmap's last key is more accurate than using a separatelatest_block_numberfield, as it directly reflects the actual data structure.
208-215: Nice data structure improvement!The updated comments clearly explain the structure of
FetchRangeResult, making it easier to understand how block numbers map to transactions and timestamps.
352-359: Clear logic improvement for block range!Ohayo sensei! The updated comments clearly explain that the
toblock is non-inclusive, which helps prevent confusion about the fetching boundaries.
364-368: Better logging of fetch results!The improved logging now reports the actual latest block number from the range result rather than the target, giving more accurate information about what was processed.
412-490: Great implementation of parallel block timestamp fetching!Ohayo sensei! This is a nice optimization that fetches block timestamps in parallel while processing events, using a semaphore to control concurrency. The approach of caching timestamps in the
blocksmap prevents redundant fetches for the same block.
578-602: Updated process_range to work with new data structureThe updated method correctly processes transactions from the new
FetchRangeResultformat.crates/torii/sqlite/src/executor/mod.rs (1)
7-7: Simplified import for anyhow::ResultThe import has been simplified to just import
Resultfrom anyhow.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/torii/indexer/src/engine.rs (2)
682-682:⚠️ Potential issueSame potential panic issue with block map access
Similar to the previous comment, direct indexing can panic if the key doesn't exist.
Use
getmethod for safer access:- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or(&0)).await?;
392-394: 🛠️ Refactor suggestionPotential excessive data fetching with
to_block: NoneSetting
to_block: Nonemight fetch more events than needed, which contradicts the PR goal of reducing fetches.- to_block: None, + to_block: Some(BlockId::Number(to)),While your downstream code does handle the limit with the
toparameter, setting a boundary in the filter would be more efficient.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/indexer/src/engine.rs(13 hunks)crates/torii/sqlite/src/executor/mod.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/executor/mod.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/engine.rs
[error] 21-21: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 29-29: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 194-194: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 688-688: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (10)
crates/torii/indexer/src/engine.rs (10)
24-24: Ohayo sensei! Good import consolidation.Combining both
Instantandsleepfromtokio::timein a single import line improves code organization.
36-39: Nice addition of ERC1155 and ERC4906 processors!These new imports suggest added support for more token standards, which expands the capabilities of the indexer.
197-197: Better block_id implementation!Using the last key from the blocks map is more accurate than relying on a separate latest_block_number field.
206-210: Excellent restructuring of FetchRangeResult!The BTreeMap approach is more elegant and provides both block numbers and timestamps in a single data structure.
350-353: Ohayo sensei! Clear comment on non-inclusive block range.The detailed comment helps clarify the behavior of the block range, which is essential for understanding how the indexer works.
359-363: Improved fetch_range usage with clearer logging.The explicit comment about fetching all events and improved logging makes the code more maintainable.
381-382: Good choice making fetch_range immutable!Changing from
&mut selfto&selfis a nice improvement as it indicates the method doesn't modify the Engine instance.
500-517: Clearer skip logic for processed transactions.The improved handling of
last_contract_txwith a temporary variable makes the transaction skipping logic more robust.
530-534: Excellent optimization to stop page fetching!Adding this early termination when reaching blocks beyond our target range significantly reduces unnecessary network calls.
690-696: Simpler and cleaner cursor update logic.The code now extracts the last block data directly from the blocks map, which is more straightforward than the previous approach.
| for (block_number, transactions) in range.transactions { | ||
| for (transaction_hash, events) in transactions { |
There was a problem hiding this comment.
Potential panic if block_number doesn't exist in the blocks map
Accessing range.blocks[&block_number] will panic if the block number is not in the map.
Use get instead of direct indexing to avoid potential panics:
- range.blocks[&block_number],
+ *range.blocks.get(&block_number).unwrap_or(&0),Also applies to: 669-670
| @@ -21,22 +21,22 @@ use starknet::providers::{Provider, ProviderRequestData, ProviderResponseData}; | |||
| use starknet_crypto::Felt; | |||
There was a problem hiding this comment.
Fix formatting issues flagged by CI.
The pipeline indicates formatting issues on several lines.
Run rustfmt on the file to fix formatting issues on lines 21, 29, 194, and 688.
#!/bin/bash
# Run rustfmt to fix formatting issues
rustfmt crates/torii/indexer/src/engine.rsAlso applies to: 29-29, 194-194, 688-688
🧰 Tools
🪛 GitHub Actions: ci
[error] 21-21: Rust formatting check failed. Please run 'rustfmt' to format the code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/torii/indexer/src/engine.rs (1)
395-395: 💡 Verification agent🧩 Analysis chain
Setting to_block: None might fetch excessive data
The PR aims to optimize event fetching, but using
to_block: Nonemight fetch more events than needed. I see this was previously flagged and addressed in commits 4a654af to 52d797c, but the current code still sets it toNone.- to_block: None, + to_block: Some(BlockId::Number(to)),This would limit the events fetched to just the specified range, reducing unnecessary network traffic.
🏁 Script executed:
#!/bin/bash # Check if a subsequent PR fixed this issue or if there's a reason for keeping to_block: None echo "Checking if this code was modified in subsequent commits..." git log --oneline --grep="4a654af\|52d797c" -- crates/torii/indexer/src/engine.rs echo "Searching for explanations about to_block usage..." rg -A 3 -B 3 "to_block.*None" crates/torii/indexer/src/engine.rsLength of output: 595
Ohayo, sensei! Please update the event fetching limit.
It looks like the current code in
crates/torii/indexer/src/engine.rs(around line 395) still setsto_block: None, which can result in fetching more events than necessary. The previous commits (4a654af to 52d797c) suggested limiting the event range. To optimize network traffic and avoid processing an excessive amount of data, please change the code as follows:- to_block: None, + to_block: Some(BlockId::Number(to)),Action Items:
- Update
to_blockto use a specific block number in the target file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
crates/torii/indexer/src/engine.rs (6)
507-508: Potential issue with last_block_number calculationIf
events_page.eventsis empty,last_block_numberwill be 0, which could cause incorrect termination logic. I see this was previously flagged and addressed in commits 4a654af to 52d797c, but the current code still has the issue.- let last_block_number = - events_page.events.last().map_or(0, |e| e.block_number.unwrap()); + let last_block_number = events_page.events.last() + .map(|e| e.block_number.unwrap()) + .unwrap_or_else(|| events_page.continuation_token.is_some().then_some(to - 1).unwrap_or(to));This ensures proper continuation even with empty pages.
532-536: Good optimization to limit page fetchesThe early termination when
last_block_number >= tois an excellent optimization to prevent unnecessary fetching of additional pages when we've already reached our target block.
671-671: Ohayo sensei! Potential panic with direct map accessUsing
range.blocks[&block_number]will panic if the key doesn't exist in the map. Use a safer method:- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or(&0),This prevents crashes if a block number is missing from the map.
684-684: Same potential panic issue with direct map accessSimilar to the previous comment, direct map indexing can panic:
- self.process_block(block_number, range.blocks[&block_number]).await?; + self.process_block(block_number, *range.blocks.get(&block_number).unwrap_or(&0)).await?;
360-362: Clear commenting in fetch_dataGood job adding clear comments explaining that this fetches events from 'from' to the blocks chunk size. This improves code readability and maintainability.
502-504: Improved cursor handlingThe changes to how the last contract transaction is handled are good. By cloning the value and using a temporary variable, you prevent unintended modifications to the original cursor.
| FetchDataResult::Range(range) => { | ||
| Some(BlockId::Number(*range.blocks.keys().last().unwrap())) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Potential panic if blocks map is empty
This code assumes that range.blocks is never empty, which could lead to a panic if the map has no keys. Consider adding a safety check:
- Some(BlockId::Number(*range.blocks.keys().last().unwrap()))
+ range.blocks.keys().last().map(|&last_block| BlockId::Number(last_block))📝 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.
| FetchDataResult::Range(range) => { | |
| Some(BlockId::Number(*range.blocks.keys().last().unwrap())) | |
| } | |
| FetchDataResult::Range(range) => { | |
| - Some(BlockId::Number(*range.blocks.keys().last().unwrap())) | |
| + range.blocks.keys().last().map(|&last_block| BlockId::Number(last_block)) | |
| } |
| let (last_block_number, last_block_timestamp) = range.blocks.iter().last().unwrap(); | ||
| self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?; |
There was a problem hiding this comment.
Potential panic when unwrapping last block
This assumes range.blocks is never empty. Add a safety check:
- let (last_block_number, last_block_timestamp) = range.blocks.iter().last().unwrap();
- self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?;
+ if let Some((last_block_number, last_block_timestamp)) = range.blocks.iter().last() {
+ self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?;
+ } else {
+ debug!(target: LOG_TARGET, "No blocks to update cursors with");
+ }📝 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 (last_block_number, last_block_timestamp) = range.blocks.iter().last().unwrap(); | |
| self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?; | |
| if let Some((last_block_number, last_block_timestamp)) = range.blocks.iter().last() { | |
| self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?; | |
| } else { | |
| debug!(target: LOG_TARGET, "No blocks to update cursors with"); | |
| } |
Larkooo
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3136 +/- ##
==========================================
+ Coverage 55.66% 55.67% +0.01%
==========================================
Files 443 443
Lines 62869 62871 +2
==========================================
+ Hits 34993 35005 +12
+ Misses 27876 27866 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/torii/indexer/src/engine.rs (1)
698-712:⚠️ Potential issueSame potential panic issue with block map access
Direct indexing with
range.blocks[&block_number]can panic if the key doesn't exist.- range.blocks[&block_number], + *range.blocks.get(&block_number).unwrap_or(&0),
♻️ Duplicate comments (2)
crates/torii/indexer/src/engine.rs (2)
728-729:⚠️ Potential issuePotential panic when unwrapping last block
This assumes
range.blocksis never empty. Add a safety check:- let (last_block_number, last_block_timestamp) = range.blocks.iter().last().unwrap(); - self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?; + if let Some((last_block_number, last_block_timestamp)) = range.blocks.iter().last() { + self.db.update_cursors(*last_block_number, *last_block_timestamp, None, cursor_map)?; + } else { + debug!(target: LOG_TARGET, "No blocks to update cursors with"); + }
199-201:⚠️ Potential issueOhayo sensei! Potential panic if blocks map is empty
This code assumes that
range.blocksis never empty, which could lead to a panic if the map has no keys. Consider adding a safety check:- Some(BlockId::Number(*range.blocks.keys().last().unwrap())) + range.blocks.keys().last().map(|&last_block| BlockId::Number(last_block))
🧹 Nitpick comments (1)
crates/torii/indexer/src/engine.rs (1)
407-407: Potential excessive data fetching withto_block: NoneSetting
to_block: Nonein the EventFilter might fetch more events than needed, contradicting the PR goal of reducing fetches.- to_block: None, + to_block: Some(BlockId::Number(to)),While the downstream
get_all_eventsfunction does handle limiting with thetoparameter, setting a proper boundary in the filter would reduce unnecessary network traffic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/graphql/src/tests/mod.rs(1 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)crates/torii/indexer/src/engine.rs(14 hunks)crates/torii/indexer/src/test.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
crates/torii/graphql/src/tests/mod.rs (7)
crates/torii/graphql/src/object/erc/token_balance.rs (6)
data(248-248)data(249-249)data(274-274)data(275-275)data(278-278)data(279-279)crates/torii/graphql/src/object/erc/token_transfer.rs (6)
data(185-185)data(186-186)data(211-211)data(212-212)data(215-215)data(216-216)crates/torii/graphql/src/query/data.rs (3)
data(197-197)data(198-198)data(223-223)crates/torii/indexer/src/engine.rs (1)
new(252-289)crates/torii/sqlite/src/executor/mod.rs (2)
new(168-170)new(243-269)crates/torii/sqlite/src/cache.rs (3)
new(46-48)new(135-149)new(183-185)crates/torii/sqlite/src/lib.rs (1)
new(65-72)
crates/torii/grpc/src/server/tests/entities_test.rs (2)
crates/torii/grpc/src/server/mod.rs (3)
data(1133-1137)new(160-210)new(1720-1775)crates/torii/indexer/src/engine.rs (1)
new(252-289)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
crates/torii/indexer/src/engine.rs (9)
208-214: Well-designed encapsulation for transaction data!The new struct nicely organizes transaction data and its emitted events, with clear documentation on when the transaction field is populated.
218-221: LGTM! Improved data organizationThe new structure provides better organization by mapping block numbers to transaction hashes to transaction data, making the relationships clearer and more maintainable.
362-366: Crystal clear comment about block range exclusivityThe comment clarifies that the
toparameter is non-inclusive, which helps prevent confusion about event fetching boundaries.
371-374: Good optimization: passing latest_block_number to avoid redundant queriesPassing the latest block number to
fetch_rangeallows the method to make smarter decisions about which blocks to fetch.
393-399: Improved method signature: mutability and new parameterChanged from
&mut selfto&selfwhich allows concurrent calls, and addedlatest_block_numberparameter for optimizations. However, this is a breaking change that must be updated in all call sites.
451-477: Excellent optimization: batch transaction requestsBatching transaction requests when transactions indexing is enabled will significantly reduce network calls compared to individual fetches.
486-492: Smart optimization for latest block fetchingUsing
BlockTag::Latestfor the latest block number helps leverage caching in the provider implementation.
515-517: Good trace logging additionsAdded trace logging provides visibility into the number of transactions and blocks being processed, which is helpful for debugging.
574-578: Great optimization: early termination on reaching target blockThis early exit prevents fetching unnecessary pages when we've already reached our target block number, saving bandwidth and processing time.
crates/torii/graphql/src/tests/mod.rs (1)
375-375: Ohayo sensei! Correct update to match the new method signatureThe test has been properly updated to include the new
latest_block_numberparameter in thefetch_rangecall, ensuring compatibility with the engine changes.crates/torii/indexer/src/test.rs (1)
54-54: Ohayo sensei! Proper adaptation to API changesThe test has been correctly updated to pass the additional
toparameter tofetch_range, maintaining compatibility with the engine changes.crates/torii/grpc/src/server/tests/entities_test.rs (1)
122-122: Ohayo! Update to match new fetch_range method signature looks good!The change correctly adds the new block number parameter to match the updated
Engine::fetch_rangemethod signature. This aligns with the optimizations made in the engine to reduce get_events requests and batch transactions, sensei.
| debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.blocks.keys().last().unwrap(), "Fetched data for range."); | ||
| FetchDataResult::Range(range) |
There was a problem hiding this comment.
Potential panic in debug logging
The debug log uses unwrap() on range.blocks.keys().last() which could panic if the blocks map is empty.
-debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.blocks.keys().last().unwrap(), "Fetched data for range.");
+debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.blocks.keys().last().map_or(to, |&b| b), "Fetched data for range.");📝 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.
| debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.blocks.keys().last().unwrap(), "Fetched data for range."); | |
| FetchDataResult::Range(range) | |
| debug!(target: LOG_TARGET, duration = ?instant.elapsed(), from = %from, to = %range.blocks.keys().last().map_or(to, |&b| b), "Fetched data for range."); | |
| FetchDataResult::Range(range) |
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
fetch_rangeto accommodate additional parameters, potentially altering data retrieval behavior.