Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(torii): add support for erc1155 #2955

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Larkooo
Copy link
Collaborator

@Larkooo Larkooo commented Jan 27, 2025

Summary by CodeRabbit

  • New Features

    • Added support for ERC-1155 token transfers.
    • Introduced new processors for handling ERC1155 transfer batch and single events.
    • Enhanced NFT token metadata handling.
  • Refactor

    • Renamed ERC721-specific methods and queries to more generic NFT terminology.
    • Updated contract type handling to include ERC1155 alongside existing standards.
  • Improvements

    • Expanded token registration and balance tracking capabilities for multiple token standards.
    • Adjusted logging levels for improved debugging during IPFS content fetching.

Copy link

coderabbitai bot commented Jan 27, 2025

Walkthrough

Ohayo, sensei! This pull request introduces comprehensive changes to the token handling system in the Torii SQLite module. The modifications primarily focus on enhancing support for ERC-1155 tokens and transitioning from ERC721-specific terminology to a more generalized NFT approach. Key updates include adding a new method for handling NFT transfers, renaming related structs and methods, and introducing a new contract type to support multiple token standards.

Changes

File Change Summary
crates/torii/sqlite/src/erc.rs - Added handle_nft_transfer method
- Renamed register_erc721_token_metadata to register_nft_token_metadata
- Updated query type from RegisterErc721TokenQuery to RegisterNftTokenQuery
crates/torii/sqlite/src/executor/erc.rs - Renamed RegisterErc721TokenQuery to RegisterNftTokenQuery
- Renamed RegisterErc721TokenMetadata to RegisterNftTokenMetadata
- Updated method names to reflect NFT context
- Added handling for ContractType::ERC1155
crates/torii/sqlite/src/executor/mod.rs - Updated imports and exports
- Modified QueryType enum
- Updated Executor struct fields
- Adjusted token registration logic
crates/torii/sqlite/src/types.rs - Added ERC1155 variant to ContractType enum
- Updated from_str and Display implementations
crates/torii/indexer/src/processors/erc1155_transfer_batch.rs - Added Erc1155TransferBatchProcessor struct with methods for processing ERC1155 transfer batch events
crates/torii/indexer/src/processors/erc1155_transfer_single.rs - Added Erc1155TransferSingleProcessor struct for handling ERC1155 "TransferSingle" events
crates/torii/indexer/src/processors/erc721_legacy_transfer.rs - Updated method call in process function to use handle_nft_transfer
crates/torii/indexer/src/processors/erc721_transfer.rs - Updated method call in process function to use handle_nft_transfer
crates/torii/indexer/src/processors/mod.rs - Added new modules for erc1155_transfer_batch and erc1155_transfer_single
crates/torii/indexer/src/engine.rs - Added Erc1155TransferBatchProcessor and Erc1155TransferSingleProcessor to event processors

Possibly related PRs

Suggested reviewers

  • glihm

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/erc.rs (1)

95-96: Fix the formatting issues flagged by CI, sensei!

  • Multi-line method call should be formatted in a single line (lines 95-96)
  • Incorrect indentation in function declaration (line 143)

Also applies to: 143-143

crates/torii/sqlite/src/executor/mod.rs (1)

635-636: Consider logging metadata fetch failures, sensei!

While using empty strings as fallbacks for failed metadata fetches is resilient, consider adding warning logs to help track these issues.

-                            Err(_) => "".to_string(),
+                            Err(e) => {
+                                warn!(
+                                    contract_address = %felt_to_sql_string(&register_nft_token.contract_address),
+                                    error = %e,
+                                    "Failed to fetch token name, using empty string"
+                                );
+                                "".to_string()
+                            }

Also applies to: 656-656, 659-660, 678-678

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b722fd and 9637ab9.

📒 Files selected for processing (4)
  • crates/torii/sqlite/src/erc.rs (5 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (6 hunks)
  • crates/torii/sqlite/src/executor/mod.rs (8 hunks)
  • crates/torii/sqlite/src/types.rs (3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/erc.rs

[warning] 95-96: Formatting issue: Multi-line method call should be formatted in a single line


[warning] 143-143: Formatting issue: Incorrect indentation in function declaration and implementation

crates/torii/sqlite/src/executor/erc.rs

[warning] 235-237: Formatting issue: Comment should be on the same line as the closing brace


[warning] 245-247: Formatting issue: Incorrect brace placement in await expression

🔇 Additional comments (6)
crates/torii/sqlite/src/types.rs (1)

159-159: Ohayo! The ERC1155 contract type addition looks good, sensei!

The new variant is consistently implemented across the enum and its trait implementations.

Also applies to: 176-176, 188-188

crates/torii/sqlite/src/erc.rs (2)

145-205: Ohayo! The ERC1155 transfer handler implementation is well-structured, sensei!

The implementation correctly handles ERC1155 token transfers, following the same pattern as ERC721 but with proper amount handling.


Line range hint 285-305: The NFT metadata registration refactoring looks clean, sensei!

The renaming from ERC721-specific to generic NFT naming improves code maintainability and better reflects the multi-token support.

crates/torii/sqlite/src/executor/erc.rs (2)

25-29: The NFT registration types look good, sensei!

The renaming of token registration types to be more generic improves code maintainability.

Also applies to: 32-36


98-105: The ERC1155 balance handling looks correct, sensei!

The implementation properly handles ERC1155 token balances following the same pattern as ERC721.

crates/torii/sqlite/src/executor/mod.rs (1)

113-113: The query type updates align well with the NFT refactoring, sensei!

The changes to QueryType and register_tasks are consistent with the broader NFT generalization.

Also applies to: 137-137

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (2)

27-35: Good validation logic, sensei!
This method ensures the correct shape of the event, but consider logging more details if the validation fails, for improved debugging.

 fn validate(&self, event: &Event) -> bool {
     ...
-    false
+    {
+        debug!(target: LOG_TARGET, "Validation failed for event: {:?}", event);
+        false
+    }
 }

45-109: Performing multi-pass deserialization, buddybuddy.
The approach of first collecting IDs and then mapping them to amounts is logically clean and straightforward. A couple of suggestions to consider for better error handling and user transparency:

  1. Log or return an error if the input data is malformed (e.g. ids_len is larger than the actual array size).
  2. If you expect huge arrays, consider chunked insertion or batch processing in the database for performance.
crates/torii/indexer/src/processors/erc1155_transfer_single.rs (2)

29-36: Validation logic is concise, ohayo sensei!
You’ve properly checked the length of keys and data. If there's a possibility of partial or unexpected data, you might log more details when validation fails.


46-80: Process method is well-organized, buddybuddy.
Extracting token_id and amount using cairo_deserialize is appropriate. Great to see error handling integrated with Result. One suggestion is to handle batch insertion if transferring multiple tokens in a single event, though this file handles single transfers.

crates/torii/sqlite/src/erc.rs (1)

123-131: Consider using appropriate contract types for balance cache entries, sensei!

The balance cache is using ContractType::ERC721 for all NFT transfers. For better type safety and future maintainability, consider using the actual contract type (ERC721 or ERC1155) when creating cache entries.

-                    erc_cache.entry((ContractType::ERC721, from_balance_id)).or_default();
+                    erc_cache.entry((contract_type, from_balance_id)).or_default();
-                    erc_cache.entry((ContractType::ERC721, to_balance_id)).or_default();
+                    erc_cache.entry((contract_type, to_balance_id)).or_default();
crates/torii/sqlite/src/executor/erc.rs (1)

239-251: Consider adding documentation for the ERC1155 tokenURI handling, sensei!

The ERC1155 tokenURI case is correctly implemented with empty calldata, but it would be helpful to add a comment explaining why ERC1155 doesn't require a token ID for the URI.

+        // ERC1155 uses a base URI that doesn't require a token ID
         else if let Ok(token_uri) = provider
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9637ab9 and bd1a72d.

📒 Files selected for processing (7)
  • crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (1 hunks)
  • crates/torii/indexer/src/processors/erc1155_transfer_single.rs (1 hunks)
  • crates/torii/indexer/src/processors/erc721_legacy_transfer.rs (1 hunks)
  • crates/torii/indexer/src/processors/erc721_transfer.rs (1 hunks)
  • crates/torii/indexer/src/processors/mod.rs (1 hunks)
  • crates/torii/sqlite/src/erc.rs (6 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (6 hunks)
✅ Files skipped from review due to trivial changes (1)
  • crates/torii/indexer/src/processors/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: fmt
🔇 Additional comments (13)
crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (5)

1-9: Ohayo sensei! Imports look clean buddybuddy.
No issues here with the chosen dependencies and their usage. Everything is aligned with async patterns and error handling.


15-16: Struct definition is succinct and well-documented, sensei!
Great job keeping the struct minimal, just enough fields for an event processor.


23-25: Clear event_key definition, buddybuddy.
Returning "TransferBatch" is consistent with standard naming.


37-39: Task priority is straightforward, ohayo buddybuddy.
No suggestions here.


41-43: task_identifier usage is consistent, sensei!
Leveraging TASK_ID_SEQUENTIAL for batch events is wise to avoid concurrency issues.

crates/torii/indexer/src/processors/erc1155_transfer_single.rs (2)

17-18: Minimal struct, sensei buddybuddy!
A minimal approach for the processor struct is a good practice—less overhead and easy to maintain.


25-27: ’event_key’ returning "TransferSingle" is consistent.
This is a good alignment with the event signature. No issues here.

crates/torii/indexer/src/processors/erc721_transfer.rs (1)

83-88: Switch to handle_nft_transfer is a wise refactor, sensei!
Passing U256::from(1u8) lines up with the typical 1:1 ownership model of ERC721 tokens. This unifies the logic across token standards and is consistent with the new approach in ERC1155.

crates/torii/indexer/src/processors/erc721_legacy_transfer.rs (1)

Line range hint 83-92: Ohayo! LGTM - Clean adaptation to the new NFT interface, sensei!

The change correctly adapts the legacy ERC721 transfer processor to the new generalized NFT interface while maintaining the non-fungible nature of ERC721 tokens by using an amount of 1.

crates/torii/sqlite/src/erc.rs (2)

82-91: Ohayo! Clean implementation of the generalized NFT transfer handler, sensei!

The new handler elegantly supports both ERC721 and ERC1155 tokens through the addition of the amount parameter while maintaining the existing transfer logic.


Line range hint 223-246: LGTM - Clean metadata registration update, sensei!

The metadata registration has been cleanly updated to use the new NFT terminology while maintaining the same functionality.

crates/torii/sqlite/src/executor/erc.rs (2)

98-105: Ohayo! Clean implementation of ERC1155 balance diff handling, sensei!

The ERC1155 case is properly handled using the same pattern as ERC721, with correct token ID parsing and balance updates.


Line range hint 370-389: Consider making decimals configurable for ERC1155 tokens, sensei!

The method uses a fixed decimals value of 0, but ERC1155 tokens can be fungible and might need different decimal places. Consider making this configurable or fetching it from the contract for ERC1155 tokens.

Let's check if any ERC1155 contracts in the codebase define decimals:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/erc.rs (2)

Line range hint 98-115: Ohayo sensei! Consider refactoring duplicated token handling logic.

The ERC1155 case shares identical token ID parsing logic with the ERC721 case. Consider extracting this common logic into a helper function.

+ fn parse_token_id(token_id: &str) -> (&str, &str) {
+     let mid = token_id.split(":").collect::<Vec<&str>>();
+     (mid[0], token_id)
+ }

  ContractType::ERC721 | ContractType::ERC1155 => {
      assert!(id.len() == 2);
      let account_address = id[0];
      let token_id = id[1];
-     let mid = token_id.split(":").collect::<Vec<&str>>();
-     let contract_address = mid[0];
+     let (contract_address, token_id) = parse_token_id(token_id);

254-257: Ohayo! Consider adjusting log levels for better observability.

The URI and metadata fetch failures are currently logged at WARN level. Consider:

  1. Using ERROR level for permanent failures (e.g., invalid URIs)
  2. Keeping WARN level for temporary failures (e.g., network issues)
- warn!(
+ error!(
      contract_address = format!("{:#x}", register_nft_token.contract_address),
      token_id = %register_nft_token.actual_token_id,
      "Error fetching token URI, empty metadata will be used instead.",
  );

Also applies to: 290-293

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd1a72d and 4cec8ce.

📒 Files selected for processing (3)
  • crates/torii/indexer/src/engine.rs (2 hunks)
  • crates/torii/indexer/src/processors/erc1155_transfer_single.rs (1 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (6 hunks)
🔇 Additional comments (5)
crates/torii/indexer/src/processors/erc1155_transfer_single.rs (3)

27-34: Ohayo sensei! Consider more granular validation.

While this check ensures the right number of keys and data elements, you could also validate that the addresses (e.g., from, to) are not zero if required by your business logic. A more descriptive error message could help debug misconfigured events.


40-42: Ohayo sensei! Explore parallelized task identifiers.

Currently, this processor returns TASK_ID_SEQUENTIAL. If multiple ERC1155 transfers can safely be processed concurrently, assigning a distinct task identifier might improve overall performance. Please ensure no shared state or concurrency conflicts before parallelizing, though.


64-74: Ohayo sensei! Confirm error handling for handle_nft_transfer.

The .await? usage propagates database errors. Ensure that such errors are gracefully handled upstream, especially if partial failures could leave your system in an inconsistent state.

crates/torii/indexer/src/engine.rs (1)

110-116: Ohayo sensei! Verify comprehensive testing for new ERC1155 additions.

Your addition of Erc1155TransferBatchProcessor and Erc1155TransferSingleProcessor is essential for ERC1155 support. Confirm that unit/integration tests cover batch vs. single transfers, preventing regressions when handling varied transfer scenarios.

Would you like help drafting or expanding test coverage for these new processors?

crates/torii/sqlite/src/executor/erc.rs (1)

25-29: Ohayo! Clean abstraction with NFT-generic structs.

The renaming from ERC721-specific to NFT-generic structs provides better abstraction for handling multiple token standards.

Also applies to: 32-37

crates/torii/sqlite/src/executor/erc.rs Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
crates/torii/sqlite/src/executor/erc.rs (1)

Ohayo sensei! HTTP client requires timeout configuration for secure operation

The HTTP client should be configured with appropriate timeout settings to prevent potential DoS vectors when fetching metadata from external sources. Consider using ClientBuilder with timeout configuration:

let client = ClientBuilder::new()
    .timeout(Duration::from_secs(20))
    .build()
    .context("Failed to create HTTP client")?;

This aligns with the timeout patterns used in other parts of the codebase, such as the RPC services.

🔗 Analysis chain

Line range hint 314-324: Verify HTTP client security configuration.

The HTTP client is created without explicit timeout or redirect settings. This could potentially lead to security issues or resource exhaustion.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any other instances of reqwest Client creation
# that might have security configurations we can reuse
rg -A 5 "Client::new\(\)" 

# Look for any security-related configurations or middleware
rg -g "*.rs" -e "timeout" -e "redirect" -e "middleware"

Length of output: 5467

🧹 Nitpick comments (2)
crates/torii/sqlite/src/executor/erc.rs (2)

98-105: Update comment to reflect correct format.

The comment format account_address/contract_address:id doesn't match the assertion and parsing logic that follows. Consider updating it to accurately reflect the expected format.

-                    // account_address/contract_address:id => ERC1155
+                    // account_address/token_id => ERC1155, where token_id is contract_address:id

Line range hint 373-392: Update error message to reflect NFT generalization.

The error context still references "721Token" which doesn't align with the generalized NFT approach.

-            .with_context(|| format!("Failed to execute721Token query: {:?}", result))?;
+            .with_context(|| format!("Failed to execute NFT token query: {:?}", result))?;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cec8ce and 9201560.

📒 Files selected for processing (1)
  • crates/torii/sqlite/src/executor/erc.rs (7 hunks)
🔇 Additional comments (2)
crates/torii/sqlite/src/executor/erc.rs (2)

25-36: Ohayo sensei! Clean struct renaming for better abstraction!

The renaming from ERC721-specific to generic NFT naming improves code reusability and maintainability by abstracting the token standard specifics.


Line range hint 240-253: ERC1155 URI string substitution needed.

The ERC1155 standard specifies that the URI may include the string {id} which should be replaced with the token ID in hexadecimal (zero-padded to 64 hex chars). This substitution is currently missing.

Consider adding URI string substitution:

        else if let Ok(token_uri) = provider
            .call(
                FunctionCall {
                    contract_address: register_nft_token.contract_address,
                    entry_point_selector: get_selector_from_name("uri").unwrap(),
                    calldata: vec![
                        register_nft_token.actual_token_id.low().into(),
                        register_nft_token.actual_token_id.high().into(),
                    ],
                },
                BlockId::Tag(BlockTag::Pending),
            )
            .await
        {
-            token_uri
+            let mut uri = ByteArray::cairo_deserialize(&token_uri, 0)?
+                .to_string()
+                .expect("Return value not String");
+            if uri.contains("{id}") {
+                let hex_id = format!("{:064x}", register_nft_token.actual_token_id);
+                uri = uri.replace("{id}", &hex_id);
+            }
+            ByteArray::cairo_serialize(&uri.try_into().unwrap())
        }

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 6.56566% with 185 lines in your changes missing coverage. Please review.

Project coverage is 56.58%. Comparing base (9b722fd) to head (f123ced).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...i/indexer/src/processors/erc1155_transfer_batch.rs 4.83% 59 Missing ⚠️
crates/torii/sqlite/src/executor/erc.rs 0.00% 52 Missing ⚠️
.../indexer/src/processors/erc1155_transfer_single.rs 8.10% 34 Missing ⚠️
crates/torii/sqlite/src/executor/mod.rs 0.00% 25 Missing ⚠️
crates/torii/sqlite/src/erc.rs 0.00% 8 Missing ⚠️
...i/indexer/src/processors/erc721_legacy_transfer.rs 0.00% 2 Missing ⚠️
...es/torii/indexer/src/processors/erc721_transfer.rs 0.00% 2 Missing ⚠️
crates/torii/sqlite/src/types.rs 0.00% 2 Missing ⚠️
crates/torii/sqlite/src/utils.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2955      +/-   ##
==========================================
- Coverage   56.70%   56.58%   -0.12%     
==========================================
  Files         420      422       +2     
  Lines       55554    55701     +147     
==========================================
+ Hits        31502    31520      +18     
- Misses      24052    24181     +129     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we have a quick way to add a test on indexing them?

Also, not sure if the erc1155 has the same issue as erc20/erc721 with changes in the events during the life of starknet?

If yes, we may have to adjust the processor to take those cases in account for old erc1155. Will check some of them to see if there's some tokens we can use to test.

But using at least the latest erc1155 from OZ could be great to ensure compatibility.

@Larkooo
Copy link
Collaborator Author

Larkooo commented Jan 28, 2025

Would we have a quick way to add a test on indexing them?

Also, not sure if the erc1155 has the same issue as erc20/erc721 with changes in the events during the life of starknet?

If yes, we may have to adjust the processor to take those cases in account for old erc1155. Will check some of them to see if there's some tokens we can use to test.

But using at least the latest erc1155 from OZ could be great to ensure compatibility.

I used random erc1155 tokens from starkscan to test the behaviour. Not sure about old ones tho.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (1)

89-99: Consider adding error context for database operations.

When the database operation fails, it might be helpful to include context about which token and transfer was being processed.

Here's a suggested enhancement:

-            db.handle_nft_transfer(
-                token_address,
-                from,
-                to,
-                *token_id,
-                amount,
-                block_timestamp,
-                event_id,
-                block_number,
-            )
-            .await?;
+            db.handle_nft_transfer(
+                token_address,
+                from,
+                to,
+                *token_id,
+                amount,
+                block_timestamp,
+                event_id,
+                block_number,
+            )
+            .await
+            .with_context(|| format!(
+                "Failed to handle NFT transfer: token={:?}, id={:?}, from={:?}, to={:?}",
+                token_address, token_id, from, to
+            ))?;
crates/torii/sqlite/src/executor/erc.rs (2)

86-105: Consider reducing code duplication in token handling, sensei!

The ERC1155 implementation follows the same pattern as ERC721. Consider extracting the common logic into a helper function to improve maintainability.

+ fn extract_token_info(id: &[&str]) -> (String, String, String) {
+     let account_address = id[0].to_string();
+     let token_id = id[1].to_string();
+     let mid = token_id.split(":").collect::<Vec<&str>>();
+     let contract_address = mid[0].to_string();
+     (account_address, contract_address, token_id)
+ }

  match contract_type {
      ContractType::WORLD => unreachable!(),
      ContractType::ERC721 | ContractType::ERC1155 => {
          assert!(id.len() == 2);
-         let account_address = id[0];
-         let token_id = id[1];
-         let mid = token_id.split(":").collect::<Vec<&str>>();
-         let contract_address = mid[0];
+         let (account_address, contract_address, token_id) = extract_token_info(&id);

Line range hint 383-402: Enhance error message clarity, sensei!

The error message "Failed to execute721Token query" still references ERC721 despite the function being renamed for generic NFT handling.

-            .with_context(|| format!("Failed to execute721Token query: {:?}", result))?;
+            .with_context(|| format!("Failed to execute NFT token query: {:?}", result))?;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9201560 and f123ced.

📒 Files selected for processing (4)
  • crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (1 hunks)
  • crates/torii/indexer/src/processors/erc1155_transfer_single.rs (1 hunks)
  • crates/torii/sqlite/src/executor/erc.rs (7 hunks)
  • crates/torii/sqlite/src/utils.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/torii/indexer/src/processors/erc1155_transfer_single.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: build
  • GitHub Check: clippy
🔇 Additional comments (7)
crates/torii/sqlite/src/utils.rs (1)

67-67: Ohayo! LGTM on the log level change, sensei!

The change from warn to debug for retry attempts is reasonable since the final error is still propagated. Just ensure your logging configuration captures debug logs if monitoring IPFS fetch retries is important for your operations.

crates/torii/indexer/src/processors/erc1155_transfer_batch.rs (3)

15-22: Ohayo! The processor implementation looks clean, sensei!

The struct and trait implementation with the Provider constraint is well-structured and follows Rust best practices.


27-32: Consider enhancing the validation comment with a spec reference.

The comment explains the key and data format well, but adding a reference to the ERC-1155 specification would be helpful for future maintainers.

Add the spec reference to the validation comment:

     fn validate(&self, event: &Event) -> bool {
-        // key: [hash(TransferBatch), operator, from, to]
-        // data: [ids_len, ids[0].low, ids[0].high, ..., values_len, values[0].low, values[0].high,
-        // ...]
+        // Per ERC-1155 spec (https://eips.ethereum.org/EIPS/eip-1155#transferbatch)
+        // key: [hash(TransferBatch), operator, from, to]
+        // data: [ids_len, ids[0].low, ids[0].high, ..., values_len, values[0].low, values[0].high,
+        // ...]
         event.keys.len() == 4 && !event.data.is_empty()
     }

81-84: LGTM on the bounds checking in the values loop!

Good job implementing proper bounds checking for the values iteration, sensei! This prevents any potential out-of-bounds access during amount processing.

crates/torii/sqlite/src/executor/erc.rs (3)

Line range hint 25-37: Ohayo! Clean struct renaming for better abstraction.

The renaming from ERC721-specific to generic NFT naming improves code maintainability and better represents the multi-token support.


Line range hint 239-287: Excellent implementation of ERC1155 URI handling!

The code correctly implements:

  1. The uri() function call for ERC1155 tokens
  2. The {id} substitution with zero-padded hex as per EIP-1155

Line range hint 266-278: Verify URI handling security, sensei!

Please ensure proper handling of malicious URIs and content:

  1. Consider adding URI length limits
  2. Implement timeouts for external fetches
  3. Add content size limits for metadata
✅ Verification successful

URI parsing already has built-in safeguards, sensei!

The code is secure because:

  • ByteArray and Felt array types provide implicit length constraints
  • Malformed URIs are properly handled with error returns
  • Resource exhaustion is prevented via parallel task limiting
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing URI validation
rg -A 5 "uri|URL" . | grep -i "limit\|timeout\|size"

# Check for potential security measures in dependencies
grep -i "timeout\|limit" Cargo.toml

Length of output: 65659

Comment on lines +76 to +78
// Move index to values array
let values_len = event.data[current_idx].try_into().unwrap_or(0u64) as usize;
current_idx += 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add bounds check for values_len access.

Similar to the ids_len issue, the values_len access should be protected against out-of-bounds access.

Here's a suggested fix:

         // Move index to values array
-        let values_len = event.data[current_idx].try_into().unwrap_or(0u64) as usize;
+        let values_len = event.data.get(current_idx)
+            .and_then(|v| v.try_into().ok())
+            .unwrap_or(0u64) as usize;
         current_idx += 1;
📝 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.

Suggested change
// Move index to values array
let values_len = event.data[current_idx].try_into().unwrap_or(0u64) as usize;
current_idx += 1;
// Move index to values array
let values_len = event.data.get(current_idx)
.and_then(|v| v.try_into().ok())
.unwrap_or(0u64) as usize;
current_idx += 1;

Comment on lines +62 to +74
let ids_len = event.data[0].try_into().unwrap_or(0u64) as usize;
let mut current_idx = 1;

// First pass: read all token IDs
let mut token_ids = Vec::with_capacity(ids_len);
for _ in 0..ids_len {
if current_idx + 1 >= event.data.len() {
break;
}
let token_id = U256Cainome::cairo_deserialize(&event.data, current_idx)?;
token_ids.push(U256::from_words(token_id.low, token_id.high));
current_idx += 2;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add bounds checking for array access, sensei.

The current implementation might panic if ids_len is larger than the actual data array. Consider adding a bounds check before accessing the array.

Here's a suggested fix:

-        let ids_len = event.data[0].try_into().unwrap_or(0u64) as usize;
+        let ids_len = event.data.get(0)
+            .and_then(|v| v.try_into().ok())
+            .unwrap_or(0u64) as usize;
         let mut current_idx = 1;
 
         // First pass: read all token IDs
         let mut token_ids = Vec::with_capacity(ids_len);
         for _ in 0..ids_len {
             if current_idx + 1 >= event.data.len() {
                 break;
             }
📝 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.

Suggested change
let ids_len = event.data[0].try_into().unwrap_or(0u64) as usize;
let mut current_idx = 1;
// First pass: read all token IDs
let mut token_ids = Vec::with_capacity(ids_len);
for _ in 0..ids_len {
if current_idx + 1 >= event.data.len() {
break;
}
let token_id = U256Cainome::cairo_deserialize(&event.data, current_idx)?;
token_ids.push(U256::from_words(token_id.low, token_id.high));
current_idx += 2;
}
let ids_len = event.data.get(0)
.and_then(|v| v.try_into().ok())
.unwrap_or(0u64) as usize;
let mut current_idx = 1;
// First pass: read all token IDs
let mut token_ids = Vec::with_capacity(ids_len);
for _ in 0..ids_len {
if current_idx + 1 >= event.data.len() {
break;
}
let token_id = U256Cainome::cairo_deserialize(&event.data, current_idx)?;
token_ids.push(U256::from_words(token_id.low, token_id.high));
current_idx += 2;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants