opt(torii-indexer): batching requests for ercs - #3132
Conversation
WalkthroughOhayo, sensei! This change implements a batch request mechanism within the Changes
Sequence Diagram(s)sequenceDiagram
participant F as register_erc20_token_metadata
participant P as Provider
F->>P: Send batch request with [CallRequest(Name), CallRequest(Symbol), CallRequest(Decimals)]
P-->>F: Return results array
F->>F: Parse results by index and validate response types
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/sqlite/src/executor/erc.rs (1)
369-372: Consider adding specific error handling for each entry pointOhayo! While the current implementation works well, you might want to add more specific error handling to identify which entry points failed specifically. This could help with debugging issues with specific token standards.
- let token_uri = results.into_iter().find_map(|result| match result { - ProviderResponseData::Call(data) if !data.is_empty() => Some(data), - _ => None, - }); + let endpoints = ["token_uri", "tokenURI", "uri"]; + let mut errors = Vec::new(); + + let token_uri = results.into_iter().enumerate().find_map(|(i, result)| match result { + ProviderResponseData::Call(data) if !data.is_empty() => { + debug!( + endpoint = %endpoints[i], + "Successfully retrieved token URI from endpoint" + ); + Some(data) + }, + _ => { + errors.push(endpoints[i]); + None + }, + }); + + if !errors.is_empty() { + debug!( + failed_endpoints = ?errors, + "Failed to retrieve token URI from some endpoints" + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/sqlite/src/erc.rs(2 hunks)crates/torii/sqlite/src/executor/erc.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/erc.rs (2)
crates/torii/sqlite/src/utils.rs (1)
u256_to_sql_string(48-50)crates/torii/sqlite/src/model.rs (2)
contract_address(92-94)name(80-82)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/erc.rs
[error] 8-8: Rust formatting check failed. Please run 'rustfmt' to format the code.
🔇 Additional comments (7)
crates/torii/sqlite/src/erc.rs (4)
6-6: Added necessary imports for batch request functionalityOhayo! These imports are essential for implementing the batching mechanism for provider requests. Good choice to use the official Starknet types for this optimization.
Also applies to: 9-9
164-191: Excellent batching mechanism implementationOhayo, sensei! This is a great optimization to batch all three metadata requests (name, symbol, decimals) into a single provider call. The code is well-structured with clear separation between request preparation and execution.
193-193: Efficient use of batch request APIThe batch approach will significantly reduce network overhead compared to individual calls, especially for high-volume operations. Nice implementation, sensei!
195-225: Well-structured response parsing logicOhayo! The response parsing logic is well-implemented with proper error handling. The use of pattern matching for different response formats is elegant, and the
unreachable!macros provide good defensive programming.crates/torii/sqlite/src/executor/erc.rs (3)
9-9: Added necessary imports for batch request functionalityOhayo, sensei! These imports are essential for implementing the batching mechanism for token URI retrieval. Consistent with the approach used in the ERC module.
Also applies to: 12-12
337-364: Excellent batching strategy for token URI retrievalOhayo! This is a smart approach to try all three possible token URI entry points (
token_uri,tokenURI, anduri) in a single batch request. The code is well-organized and follows the same pattern as the ERC20 metadata implementation.
366-383: Efficient response processing with find_mapNice use of functional programming with
find_mapto extract the first successful response, sensei! The error handling with appropriate warning logs is also well-implemented.
| use cainome::cairo_serde::{ByteArray, CairoSerde}; | ||
| use starknet::core::types::requests::CallRequest; | ||
| use starknet::core::types::{BlockId, BlockTag, Felt, FunctionCall, U256}; | ||
| use starknet::core::utils::{get_selector_from_name, parse_cairo_short_string}; |
There was a problem hiding this comment.
Fix rustfmt error
Ohayo sensei! CI pipeline indicates a formatting error on this line. Please run rustfmt to fix the formatting issue.
rustfmt crates/torii/sqlite/src/erc.rs🧰 Tools
🪛 GitHub Actions: ci
[error] 8-8: Rust formatting check failed. Please run 'rustfmt' to format the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/erc.rs (1)
195-205: Consider adding a length check for results arrayOhayo sensei! While the batching implementation looks solid, it would be good to verify that the results array has at least 3 elements before accessing them by index to prevent potential index out of bounds panics.
let results = provider.batch_requests(requests).await?; + // Ensure we have all expected results + if results.len() < 3 { + return Err(anyhow::anyhow!("Expected 3 results from batch request, got {}", results.len())); + } // Parse name let name = match &results[0] {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/erc.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
crates/torii/sqlite/src/erc.rs (5)
6-6: Good job adding the necessary imports for batch requests!Ohayo sensei! The additions of
CallRequest,ProviderRequestData, andProviderResponseDataare essential for implementing the batch request approach. This optimization will help reduce network calls and improve performance.Also applies to: 9-9
164-191: Nice implementation of batch requests!Ohayo sensei! This is a great optimization for fetching token metadata. By preparing all three requests (name, symbol, decimals) in a single batch, you're reducing multiple network calls to just one, which should significantly improve performance.
193-193: Proper batch executionThe batch request is correctly executed and the result is properly awaited with error propagation.
196-217: Well-structured parsing logic for name and symbolThe pattern matching for both name and symbol is correctly implemented. I appreciate how you handle both short strings (single felt) and byte arrays consistently.
219-225: Clean parsing for decimalsOhayo sensei! The parsing for decimals is concise and correctly implemented. The error handling with
expectandunreachable!is appropriate for this case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/torii/sqlite/src/executor/mod.rs (3)
149-150: Consider makingbatch_sizemore configurable.Hardcoding
100might be limiting if usage patterns vary significantly. A configurable value could improve flexibility.
244-246: Unify or reference capacities in constants.Ohayo sensei, repeated capacities (
64and100) could be declared as constants or read from config for consistent clarity.
800-814: Tidy and safe bulk registration approach.
process_pending_nft_tokensis well-structured, but consider chunk processing for extremely large sets or to avoid memory spikes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(3 hunks)crates/torii/sqlite/src/executor/mod.rs(10 hunks)
🔇 Additional comments (9)
crates/torii/sqlite/src/executor/mod.rs (7)
9-9: Ohayo sensei! Imports look neat.Everything here seems in order for pulling in new references to
UpdateNftMetadataQuery,Felt, and the requiredtokiomodules. No issues spotted.Also applies to: 11-11, 14-15, 25-25
142-143: Ohayo sensei! Good addition for NFT token batching.
pending_nft_tokensis a helpful approach to accumulate and process NFT tokens in bulk, improving throughput.
282-286: Bulk flush triggers look good!Ohayo sensei, automatically flushing pending tokens when the batch is full is a straightforward and effective strategy.
653-660: Ensure no unbounded backlog.Ohayo sensei, adding tokens to
pending_nft_tokensuntil hittingbatch_sizeis efficient, but do confirm that under heavy load, we don’t accumulate too many queries ifbatch_sizeisn’t reached quickly.
753-754: Pre-execution token flush is consistent.Processing pending tokens before
executeensures data consistency.
793-798: Rollback clearspending_nft_tokens.Ohayo sensei, wiping pending tokens on rollback might discard tokens that could be retried. Confirm if this is desired behavior or if we need a re-queue.
270-271:Details
❓ Verification inconclusive
Verify error handling when shutting down.
We’re processing any remaining tokens before shutting down, but if an error arises, ensure it won't block or degrade the shutdown sequence.
Would you like to confirm error-handling behavior (e.g., partial commits) during shutdown steps with a quick codebase check?
🏁 Script executed:
#!/bin/bash # Searching for any explicit shutdown handling or error conditions that might block rg "shutdown" -A 10Length of output: 59516
Ohayo, sensei – Confirming Error Handling on Shutdown
In reviewing
crates/torii/sqlite/src/executor/mod.rs(lines 270–271), we see that upon receiving the shutdown signal the executor logs the shutdown intent and then calls:// Process any remaining tokens before shutting down self.process_pending_nft_tokens().await?;Since the code uses the
?operator, any error fromprocess_pending_nft_tokens()will be propagated immediately. Given similar shutdown patterns elsewhere in the codebase, please double-check that such error propagation won’t block or degrade the shutdown sequence (e.g., resulting in partial commits or a hanging shutdown). If non-critical errors during token processing should not interrupt shutdown, consider handling them by logging the error and continuing with shutdown instead of propagating the error.crates/torii/sqlite/src/executor/erc.rs (2)
9-9: Ohayo sensei! Newly introduced imports make sense.These imports from
CallRequest,ProviderRequestData, andProviderResponseDataalign well with your batch request logic. No concerns spotted.Also applies to: 12-12
473-545: Batch-basedfetch_token_uriapproach looks solid.Ohayo sensei, the new method for trying multiple entry points in one shot is neat. This should reduce RPC round trips and speed up retrieval. Nicely done!
| pub async fn process_register_nft_tokens_batch( | ||
| tokens: Vec<RegisterNftTokenQuery>, | ||
| provider: Arc<P>, | ||
| name: String, | ||
| symbol: String, | ||
| ) -> Result<RegisterNftTokenMetadata> { | ||
| let token_uri = Self::fetch_token_uri( | ||
| &provider, | ||
| register_nft_token.contract_address, | ||
| register_nft_token.token_id, | ||
| ) | ||
| .await?; | ||
| ) -> Result<Vec<RegisterNftTokenMetadata>> { | ||
| // First batch: Get name and symbol for all contracts (deduplicated) | ||
| let mut unique_contracts = std::collections::HashSet::new(); | ||
| for token in &tokens { | ||
| unique_contracts.insert(token.contract_address); | ||
| } | ||
|
|
||
| let metadata = Self::fetch_token_metadata( | ||
| register_nft_token.contract_address, | ||
| register_nft_token.token_id, | ||
| &token_uri, | ||
| ) | ||
| .await?; | ||
| let mut name_symbol_requests = Vec::new(); | ||
| let block_id = BlockId::Tag(BlockTag::Pending); | ||
|
|
||
| for &contract_address in &unique_contracts { | ||
| // Add name request | ||
| name_symbol_requests.push(ProviderRequestData::Call(CallRequest { | ||
| request: FunctionCall { | ||
| contract_address, | ||
| entry_point_selector: get_selector_from_name("name").unwrap(), | ||
| calldata: vec![], | ||
| }, | ||
| block_id, | ||
| })); | ||
| // Add symbol request | ||
| name_symbol_requests.push(ProviderRequestData::Call(CallRequest { | ||
| request: FunctionCall { | ||
| contract_address, | ||
| entry_point_selector: get_selector_from_name("symbol").unwrap(), | ||
| calldata: vec![], | ||
| }, | ||
| block_id, | ||
| })); | ||
| } | ||
|
|
||
| let name_symbol_results = provider.batch_requests(name_symbol_requests).await?; | ||
|
|
||
| // Create a map of contract_address -> (name, symbol) | ||
| let mut contract_metadata = std::collections::HashMap::new(); | ||
| for (i, contract_address) in unique_contracts.iter().enumerate() { | ||
| let name_result = &name_symbol_results[i * 2]; | ||
| let symbol_result = &name_symbol_results[i * 2 + 1]; | ||
|
|
||
| let name = match name_result { | ||
| ProviderResponseData::Call(name) if name.len() == 1 => { | ||
| parse_cairo_short_string(&name[0]).unwrap() | ||
| } | ||
| ProviderResponseData::Call(name) => { | ||
| ByteArray::cairo_deserialize(name, 0) | ||
| .expect("Return value not ByteArray") | ||
| .to_string() | ||
| .expect("Return value not String") | ||
| } | ||
| _ => "".to_string(), | ||
| }; | ||
|
|
||
| let symbol = match symbol_result { | ||
| ProviderResponseData::Call(symbol) if symbol.len() == 1 => { | ||
| parse_cairo_short_string(&symbol[0]).unwrap() | ||
| } | ||
| ProviderResponseData::Call(symbol) => { | ||
| ByteArray::cairo_deserialize(symbol, 0) | ||
| .expect("Return value not ByteArray") | ||
| .to_string() | ||
| .expect("Return value not String") | ||
| } | ||
| _ => "".to_string(), | ||
| }; | ||
|
|
||
| Ok(RegisterNftTokenMetadata { query: register_nft_token, metadata, name, symbol }) | ||
| contract_metadata.insert(*contract_address, (name, symbol)); | ||
| } | ||
|
|
||
| // Second batch: Get token URIs for all tokens | ||
| let mut token_uri_requests = Vec::new(); | ||
| for token in &tokens { | ||
| // Try all possible token URI functions for each token | ||
| token_uri_requests.push(ProviderRequestData::Call(CallRequest { | ||
| request: FunctionCall { | ||
| contract_address: token.contract_address, | ||
| entry_point_selector: get_selector_from_name("token_uri").unwrap(), | ||
| calldata: vec![token.token_id.low().into(), token.token_id.high().into()], | ||
| }, | ||
| block_id, | ||
| })); | ||
| token_uri_requests.push(ProviderRequestData::Call(CallRequest { | ||
| request: FunctionCall { | ||
| contract_address: token.contract_address, | ||
| entry_point_selector: get_selector_from_name("tokenURI").unwrap(), | ||
| calldata: vec![token.token_id.low().into(), token.token_id.high().into()], | ||
| }, | ||
| block_id, | ||
| })); | ||
| token_uri_requests.push(ProviderRequestData::Call(CallRequest { | ||
| request: FunctionCall { | ||
| contract_address: token.contract_address, | ||
| entry_point_selector: get_selector_from_name("uri").unwrap(), | ||
| calldata: vec![token.token_id.low().into(), token.token_id.high().into()], | ||
| }, | ||
| block_id, | ||
| })); | ||
| } | ||
|
|
||
| let token_uri_results = provider.batch_requests(token_uri_requests).await?; | ||
|
|
||
| // Process results and fetch metadata | ||
| let mut results = Vec::new(); | ||
| for (i, token) in tokens.into_iter().enumerate() { | ||
| let (name, symbol) = contract_metadata.get(&token.contract_address).unwrap().clone(); | ||
|
|
||
| // Find first successful URI response for this token | ||
| let token_uri_base = i * 3; | ||
| let token_uri = token_uri_results[token_uri_base..token_uri_base + 3] | ||
| .iter() | ||
| .find_map(|result| match result { | ||
| ProviderResponseData::Call(data) if !data.is_empty() => { | ||
| let uri = if let Ok(byte_array) = ByteArray::cairo_deserialize(data, 0) { | ||
| byte_array.to_string().expect("Return value not String") | ||
| } else if let Ok(felt_array) = Vec::<Felt>::cairo_deserialize(data, 0) { | ||
| felt_array | ||
| .iter() | ||
| .map(parse_cairo_short_string) | ||
| .collect::<Result<Vec<String>, _>>() | ||
| .map(|strings| strings.join("")) | ||
| .unwrap_or_default() | ||
| } else { | ||
| "".to_string() | ||
| }; | ||
|
|
||
| if uri.is_empty() { | ||
| None | ||
| } else { | ||
| Some(uri) | ||
| } | ||
| } | ||
| _ => None, | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| // Replace {id} in token URI if needed | ||
| let token_uri = if token_uri.contains("{id}") { | ||
| let token_id_hex = format!("{:064x}", token.token_id); | ||
| token_uri.replace("{id}", &token_id_hex) | ||
| } else { | ||
| token_uri | ||
| }; | ||
|
|
||
| // Fetch metadata | ||
| let metadata = Self::fetch_token_metadata(token.contract_address, token.token_id, &token_uri).await?; | ||
|
|
||
| results.push(RegisterNftTokenMetadata { | ||
| query: token, | ||
| name, | ||
| symbol, | ||
| metadata, | ||
| }); | ||
| } | ||
|
|
||
| Ok(results) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential iteration-order mismatch with HashSet.
Ohayo sensei, enumerating a HashSet while indexing responses by i * 2 or i * 2 + 1 may cause mismatch if iteration order diverges from request order. Usually it’s fine within a single iteration block, but not guaranteed by HashSet docs. Consider using an ordered structure or a linked mapping of contracts to responses. That ensures the correct name/symbol pairs map consistently to each contract.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)
245-272:⚠️ Potential issuePotential iteration-order mismatch with
HashSet.Ohayo sensei, enumerating a HashSet while indexing responses by
i * 2ori * 2 + 1may cause mismatch if iteration order diverges from request order. Usually it's fine within a single iteration block, but not guaranteed byHashSetdocs. Consider using an ordered structure or a linked mapping of contracts to responses. That ensures the correct name/symbol pairs map consistently to each contract.- let mut unique_contracts = std::collections::HashSet::new(); + let mut unique_contracts = std::collections::LinkedHashSet::new();Or use a Vec with deduplication:
- let mut unique_contracts = std::collections::HashSet::new(); + let mut unique_contracts = Vec::new(); + for token in &tokens { + if !unique_contracts.contains(&token.contract_address) { + unique_contracts.push(token.contract_address); + } + }
🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/mod.rs (1)
245-246: Consider making the batch size configurable.The hardcoded batch size of 100 works well for most cases, but for flexibility, consider making this configurable through settings. This would allow tuning based on network conditions or load patterns.
- batch_size: 100, + batch_size: config.nft_batch_size.unwrap_or(100),Also applies to: 259-260
crates/torii/sqlite/src/executor/erc.rs (1)
494-497: Consider adding logging for failed token URI requests.When a token URI request fails, adding debug logging could help with troubleshooting. This would provide visibility into which specific request format failed.
- let token_uri = results.into_iter().find_map(|result| match result { - ProviderResponseData::Call(data) if !data.is_empty() => Some(data), - _ => None, - }); + let token_uri = results.into_iter().enumerate().find_map(|(i, result)| match result { + ProviderResponseData::Call(data) if !data.is_empty() => Some(data), + _ => { + let method = match i { + 0 => "token_uri", + 1 => "tokenURI", + 2 => "uri", + _ => "unknown", + }; + debug!( + contract_address = format!("{:#x}", contract_address), + token_id = %token_id, + method = %method, + "Token URI request failed" + ); + None + }, + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/sqlite/src/executor/erc.rs(3 hunks)crates/torii/sqlite/src/executor/mod.rs(10 hunks)examples/spawn-and-move/pistols.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/pistols.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
crates/torii/sqlite/src/executor/mod.rs (7)
143-150: Solid architectural change from concurrent tasks to batch processing!Ohayo sensei! Replacing the JoinSet with a batch collection approach is a great optimization. This should reduce task spawning overhead and improve throughput for NFT token processing.
271-272: Excellent cleanup handling before shutdown!Ohayo sensei, processing remaining tokens before shutdown ensures no work is lost. This is a valuable reliability improvement.
283-287: Well-implemented batch processing trigger.Nice implementation of the batch size check! This ensures tokens are processed in appropriately sized batches during normal operation for optimal performance.
654-661: Good addition of batching logic for RegisterNftToken.The query handling now adds tokens to the pending batch instead of processing them immediately. This aligns perfectly with the new batching architecture.
753-754: Important pre-execution cleanup.Processing pending tokens before executing a transaction ensures consistent state. Good attention to detail here!
796-797: Proper state cleanup on rollback.Clearing pending tokens on rollback maintains consistency with the transaction state. This prevents orphaned tokens from being processed after a rollback.
800-814: Well-structured batch processing implementation.The new
process_pending_nft_tokensmethod efficiently implements the batch approach with good error handling. This is the core of the optimization and it looks solid.crates/torii/sqlite/src/executor/erc.rs (5)
212-215: Good contract deduplication optimization.Deduplicating contract addresses before making RPC calls is an excellent optimization. This reduces network overhead and improves throughput.
217-239: Well-structured batch request building.The request construction for names and symbols is clear and efficient. Good job utilizing the batch request capability of the provider.
275-302: Comprehensive token URI fetching approach.Trying multiple URI function signatures (token_uri, tokenURI, uri) increases compatibility with different NFT standards. This is a thoughtful approach to handle variations in contract implementations.
308-349: Good result processing with error handling.The processing of batched results is well-structured with good error handling. The token ID replacement in URIs is handled properly, and metadata fetching is comprehensive.
463-489: Efficient batch request implementation for fetch_token_uri.Ohayo sensei! Converting the token URI fetching to use batch requests reduces network round-trips and improves performance. This is an excellent optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/sqlite/src/executor/mod.rs (3)
143-143: Consider making batch_size configurableOhayo, sensei! The hardcoded batch size of 100 for NFT tokens looks good for now, but consider making it configurable through constructor parameters or environment variables for better flexibility in different deployment scenarios.
struct Executor<'c, P: Provider + Sync + Send + 'static> { // ... other fields // Used to limit number of tokens processed in a single batch - batch_size: usize, + batch_size: usize, } impl<'c, P: Provider + Sync + Send + 'static> Executor<'c, P> { pub async fn new( pool: Pool<Sqlite>, shutdown_tx: Sender<()>, provider: Arc<P>, + batch_size: Option<usize>, ) -> Result<(Self, UnboundedSender<QueryMessage>)> { // ... existing code Ok(( Executor { // ... other fields - batch_size: 100, + batch_size: batch_size.unwrap_or(100), }, tx, )) } }Also applies to: 150-150
798-799: Consider adding error handling for individual tokensThe current implementation processes tokens in a batch, but if one token fails, it might affect the entire batch. Consider enhancing error handling to isolate failures.
async fn process_pending_nft_tokens(&mut self) -> Result<()> { // ... existing code let tokens = mem::take(&mut self.pending_nft_tokens); - let results = - Self::process_register_nft_tokens_batch(tokens, self.provider.clone()).await?; + let batch_result = Self::process_register_nft_tokens_batch(tokens.clone(), self.provider.clone()).await; + + let results = match batch_result { + Ok(results) => results, + Err(e) => { + error!(target: LOG_TARGET, error = %e, "Failed to process batch. Falling back to individual processing."); + // Optionally implement fallback to individual processing + // or better error recovery strategy + return Err(e); + } + }; for result in results { self.handle_nft_token_metadata(result).await?; } // ... rest of the method }
143-150: Add performance metrics for batch processingOhayo again! Consider adding performance metrics or logging to track the efficiency gains from batch processing. This would help with monitoring and future optimizations.
async fn process_pending_nft_tokens(&mut self) -> Result<()> { if self.pending_nft_tokens.is_empty() { return Ok(()); } + let batch_size = self.pending_nft_tokens.len(); + let start_time = Instant::now(); let tokens = mem::take(&mut self.pending_nft_tokens); let results = Self::process_register_nft_tokens_batch(tokens, self.provider.clone()).await?; for result in results { self.handle_nft_token_metadata(result).await?; } + debug!( + target: LOG_TARGET, + batch_size = batch_size, + duration = ?start_time.elapsed(), + "Processed NFT tokens batch" + ); Ok(()) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/runner/src/lib.rs(0 hunks)crates/torii/sqlite/src/executor/mod.rs(8 hunks)
💤 Files with no reviewable changes (1)
- crates/torii/runner/src/lib.rs
🔇 Additional comments (4)
crates/torii/sqlite/src/executor/mod.rs (4)
647-653: Batching implementation looks good!The token batching implementation is clean and efficient. Adding tokens to the batch and processing them when reaching the batch size is a good approach for optimizing performance.
791-805: Properly handles remaining tokens that don't reach batch sizeGood job, sensei! The implementation ensures that all pending tokens are processed through calls in the
executemethod even if they don't reach the batch size, preventing tokens from being stuck in the pending state.
743-744: Nice handling of pending tokens before transaction executionEnsuring all pending tokens are processed before executing a transaction is crucial for data consistency. This implementation handles that well.
785-788: Good cleanup in rollbackProperly clearing all pending state, including the pending_nft_tokens, during rollback ensures a clean slate for subsequent operations.
02078a9 to
3149ab5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/erc.rs (2)
164-229: Excellent performance optimization through batching!Ohayo sensei! Your implementation of batch requests for fetching token metadata is a great optimization. By reducing three network calls to a single batch request, you've improved the performance of token metadata retrieval, which is especially valuable for high-throughput applications.
The code is well-structured with clear separation between request creation, execution, and response parsing. The error handling is robust with specific error messages for different types of invalid responses.
Consider adding a comment explaining the expected response formats for each request type to help future developers understand the parsing logic more easily. For example:
// Prepare batch requests for name, symbol, and decimals let block_id = BlockId::Tag(BlockTag::Pending); +// For ERC20 tokens, we expect: +// - name: Either a short string (single felt) or a ByteArray +// - symbol: Either a short string (single felt) or a ByteArray +// - decimals: A u8 value let requests = vec![🧰 Tools
🪛 GitHub Actions: ci
[error] 197-197: Formatting check failed. Code changes detected that do not adhere to the expected style.
[error] 211-211: Formatting check failed. Code changes detected that do not adhere to the expected style.
164-229: Consider adding exponential backoff for batch requestsOhayo sensei! While the batch request implementation is excellent, consider adding error handling with exponential backoff for network-related failures. This can improve reliability when the provider service is experiencing temporary issues.
- let results = provider.batch_requests(requests).await?; + let results = match provider.batch_requests(requests).await { + Ok(results) => results, + Err(err) if err.is_network_related() => { + // Retry with exponential backoff + // This is a simplified example - you'd want to implement proper retry logic + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + provider.batch_requests(requests).await? + } + Err(err) => return Err(err.into()), + };This is optional but could improve resilience in production environments.
🧰 Tools
🪛 GitHub Actions: ci
[error] 197-197: Formatting check failed. Code changes detected that do not adhere to the expected style.
[error] 211-211: Formatting check failed. Code changes detected that do not adhere to the expected style.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/sqlite/src/erc.rs(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/erc.rs (2)
crates/torii/indexer/src/engine.rs (1)
block_id(196-203)crates/torii/sqlite/src/model.rs (2)
contract_address(92-94)name(80-82)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/erc.rs
[error] 197-197: Formatting check failed. Code changes detected that do not adhere to the expected style.
[error] 211-211: Formatting check failed. Code changes detected that do not adhere to the expected style.
🔇 Additional comments (8)
crates/torii/sqlite/src/erc.rs (8)
6-9: Ohayo, sensei! Great job updating the imports for the new batch request functionality.The new imports for
CallRequest,ProviderRequestData, andProviderResponseDataalign perfectly with the changes in the implementation. This is a clean way to support the new batching functionality.
164-191: Nice implementation of batch requests to optimize token metadata fetching!Ohayo, sensei! You've done a great job setting up the batch requests for fetching token metadata. Combining three separate calls into a single batch request will definitely improve performance by reducing network round-trips.
The code creates three separate
CallRequestobjects for name, symbol, and decimals, all using the sameblock_id. This is a well-structured approach that makes the code clearer to understand.
193-193: Efficient use of batch requests API!Using
provider.batch_requests(requests).await?is a clean way to execute all three requests in a single call, reducing network overhead.
196-207: Fix formatting issues flagged by CIOhayo sensei! The CI pipeline has flagged formatting issues on this line. Please run rustfmt to fix the formatting issue.
rustfmt crates/torii/sqlite/src/erc.rs🧰 Tools
🪛 GitHub Actions: ci
[error] 197-197: Formatting check failed. Code changes detected that do not adhere to the expected style.
196-207: Good error handling for name parsingThe name parsing logic handles different response formats effectively. You correctly handle both the short string format (with length 1) and the ByteArray format, with proper error handling for invalid responses.
🧰 Tools
🪛 GitHub Actions: ci
[error] 197-197: Formatting check failed. Code changes detected that do not adhere to the expected style.
210-221: Fix formatting issues flagged by CIOhayo sensei! The CI pipeline has flagged formatting issues on this line. Please run rustfmt to fix the formatting issue.
rustfmt crates/torii/sqlite/src/erc.rs🧰 Tools
🪛 GitHub Actions: ci
[error] 211-211: Formatting check failed. Code changes detected that do not adhere to the expected style.
210-221: Well-structured symbol parsing logicThe symbol parsing logic follows the same pattern as the name parsing, maintaining consistency in the codebase. The error handling is appropriate with a clear error message for invalid responses.
🧰 Tools
🪛 GitHub Actions: ci
[error] 211-211: Formatting check failed. Code changes detected that do not adhere to the expected style.
224-229: Clean implementation of decimals parsingThe decimals parsing is appropriately simpler since it expects a specific type (u8). The error handling is still robust with a clear error message if the response is invalid.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/sqlite/src/executor/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/executor/mod.rs (1)
crates/torii/sqlite/src/utils.rs (2)
felt_to_sql_string(40-42)felts_to_sql_string(35-38)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/executor/mod.rs
[error] 14-14: Code formatting issue: Incorrect order of imports in the file.
[error] 27-27: Code formatting issue: Incorrect order of imports in the file.
🔇 Additional comments (5)
crates/torii/sqlite/src/executor/mod.rs (5)
12-12: Ohayo! New import for CallRequest is properly added.The addition of this import enables the usage of
CallRequestwhich is essential for the batch request implementation.
15-15: Import expanded to include Provider and batch request data types.The import now includes
ProviderRequestDataandProviderResponseDatawhich are necessary for implementing the batch request functionality.
17-18: Updated tokio imports for better organization.The tokio imports have been properly organized to include all necessary components for async communication.
682-701: Well implemented batch request setup for token metadata, sensei!The implementation efficiently creates a batch request for fetching both the name and symbol of an NFT token in a single network call. This is a significant improvement over making separate calls.
Key improvements:
- Using a consistent block ID for both requests
- Properly structuring the call requests with the appropriate function selectors
- Creating a vector of requests for batching
702-730: Excellent implementation of batch request processing and response handling.The code properly:
- Sends the batch request
- Handles the response with appropriate error checking
- Parses the results for both name and symbol
- Provides fallbacks when responses don't match expected formats
This approach reduces network overhead and improves performance by combining multiple calls into one request.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)
322-322: Great addition of informative logging!Ohayo sensei! This new logging statement provides valuable context about successfully registered NFT tokens. The info level is appropriate for this operational event, and including token details (name, symbol, contract address, and token ID) will be helpful for troubleshooting.
Consider breaking this very long line into multiple lines for better readability while maintaining the same information.
- info!(target: LOG_TARGET, name = %result.name, symbol = %result.symbol, contract_address = %token.contract_address, token_id = %result.query.token_id, "NFT token registered."); + info!( + target: LOG_TARGET, + name = %result.name, + symbol = %result.symbol, + contract_address = %token.contract_address, + token_id = %result.query.token_id, + "NFT token registered." + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/erc.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/executor/mod.rs
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/executor/erc.rs (1)
crates/torii/sqlite/src/utils.rs (4)
felt_and_u256_to_sql_string(44-46)felt_to_sql_string(40-42)sql_string_to_u256(52-55)u256_to_sql_string(48-50)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/executor/erc.rs
[error] 3-3: Formatting issue detected: Code does not adhere to formatting standards.
[error] 18-18: Formatting issue detected: Code does not adhere to formatting standards.
🔇 Additional comments (3)
crates/torii/sqlite/src/executor/erc.rs (3)
7-7: Well done adding the explicit Mime type import!Ohayo sensei! This is a good explicit import for the
Mimetype that's used on line 276. Making dependencies explicit improves code clarity.
13-13: Nice addition of info-level logging!Ohayo sensei! Adding the
infoimport to the tracing module is a good preparation for the new logging statement you've added. This will help with observability of token registration events.
21-22: Clean import organization!The reordering of imports follows a logical pattern - keeping the alphabetical ordering while making sure related utilities are grouped together.
| use cainome::cairo_serde::{ByteArray, CairoSerde}; | ||
| use data_url::mime::Mime; | ||
| use data_url::DataUrl; | ||
| use data_url::mime::Mime; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Let's address the pipeline failure
Ohayo sensei! The CI pipeline is reporting formatting issues on lines 3 and 18, but these don't match with your changes. Before merging, please run cargo fmt on the entire file to ensure it meets formatting standards.
Also applies to: 13-13, 21-22, 322-322
🏁 Script executed:
#!/bin/bash
# Check rust formatting errors in the file
cd crates/torii/sqlite/src/executor/
rustfmt --check erc.rs || echo "Formatting issues detected"Length of output: 224
Ohayo sensei! Please note there's an issue with the CI pipeline's formatting check.
It seems that the error ("rustfmt: command not found") indicates that our CI environment isn’t set up with rustfmt. This is causing the reported formatting issues on lines 3, 18, 13, 13, 21-22, and 322—even though your changes themselves don’t introduce formatting errors.
Action items:
- Install rustfmt in the CI environment so that formatting checks can run properly.
- Once rustfmt is available, run
cargo fmton the entire file (i.e.,crates/torii/sqlite/src/executor/erc.rs) to ensure all formatting is up to standard.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/sqlite/src/erc.rs (2)
207-225: Consider extracting the parse logic to reduce duplication.Ohayo sensei! The parsing logic for name, symbol, and decimals follows similar patterns. To make the code more maintainable, consider extracting these into helper functions - especially for the name and symbol which share identical parsing logic.
- // Parse symbol - let symbol = match &results[1] { - ProviderResponseData::Call(symbol) if symbol.len() == 1 => { - parse_cairo_short_string(&symbol[0]).unwrap() - } - ProviderResponseData::Call(symbol) => ByteArray::cairo_deserialize(symbol, 0) - .expect("Return value not ByteArray") - .to_string() - .expect("Return value not String"), - _ => return Err(anyhow::anyhow!("Invalid response for symbol")), - }; + // Helper function to parse short strings or byte arrays + fn parse_string_response(response: &ProviderResponseData, field_name: &str) -> Result<String> { + match response { + ProviderResponseData::Call(data) if data.len() == 1 => { + Ok(parse_cairo_short_string(&data[0]).unwrap()) + } + ProviderResponseData::Call(data) => { + Ok(ByteArray::cairo_deserialize(data, 0) + .expect("Return value not ByteArray") + .to_string() + .expect("Return value not String")) + } + _ => Err(anyhow::anyhow!("Invalid response for {}", field_name)), + } + } + // Parse name and symbol + let name = parse_string_response(&results[0], "name")?; + let symbol = parse_string_response(&results[1], "symbol")?;
220-225: Consider more descriptive error messages.When handling errors for decimals, providing more context in the error message would make debugging easier.
- _ => return Err(anyhow::anyhow!("Invalid response for decimals")), + _ => return Err(anyhow::anyhow!("Invalid response type for decimals from contract {}", contract_address)),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/sqlite/src/erc.rs(2 hunks)crates/torii/sqlite/src/executor/erc.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/sqlite/src/executor/erc.rs
- crates/torii/sqlite/src/executor/mod.rs
🧰 Additional context used
🧬 Code Definitions (1)
crates/torii/sqlite/src/erc.rs (2)
crates/torii/indexer/src/engine.rs (1)
block_id(196-203)crates/torii/sqlite/src/model.rs (1)
contract_address(92-94)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
crates/torii/sqlite/src/erc.rs (3)
6-6: Ohayo sensei! Nice import additions for batch requests.The addition of
CallRequestandProviderResponseData/ProviderRequestDataimports sets the foundation for the batching optimization.Also applies to: 9-9
164-193: Great implementation of batch requests!Ohayo sensei! You've successfully created a batch request mechanism that consolidates three separate API calls into a single request. This is an excellent optimization that will reduce network overhead and improve indexer performance.
The structure with separate
CallRequestobjects for name, symbol, and decimals is clean and maintainable.
195-205: Solid parsing logic for the name response.The pattern matching approach correctly handles both short string format and ByteArray deserialization cases. The error handling for invalid responses is also well implemented.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3132 +/- ##
==========================================
- Coverage 55.74% 55.73% -0.02%
==========================================
Files 443 443
Lines 62750 62760 +10
==========================================
- Hits 34983 34980 -3
- Misses 27767 27780 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit