fix(torii): erc graphql image path & http ratelimiting #3032
Conversation
|
Ohayo, sensei! Below is the breakdown of the changes: WalkthroughThis PR introduces modifications across several modules. In the GraphQL component, the image path for ERC721 tokens is now generated using the token ID (with a colon replaced by a slash) instead of the contract address. In the server module, the image-fetching mechanism has been updated to use a new helper function, streamlining the HTTP fetching workflow and image format detection. Additionally, the SQLite modules now include a unified retry mechanism with exponential backoff, new static clients, and updated constant definitions for retry limits. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Artifacts as fetch_and_process_image
participant HTTPFetch as fetch_content_from_http
participant Server as HTTP Server
App->>Artifacts: Request image processing
Artifacts->>HTTPFetch: Fetch image using URL
HTTPFetch->>Server: Send HTTP request
alt On Failure
Server-->>HTTPFetch: Return error
HTTPFetch->>HTTPFetch: Apply exponential backoff and retry
else On Success
Server-->>HTTPFetch: Return image bytes
end
HTTPFetch-->>Artifacts: Return bytes / error after max retries
Artifacts->>Artifacts: Load image with load_from_memory_with_format
Artifacts-->>App: Return processed image or error
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/torii/graphql/src/object/erc/erc_token.rs (1)
492-494: 🛠️ Refactor suggestionConsider adding error handling for JSON parsing sensei.
The code assumes that metadata is always valid JSON with
.expect("metadata is always json"). While this might be true for your current data, it's better to handle potential parsing errors gracefully.Consider this safer approach:
- let metadata: serde_json::Value = - serde_json::from_str(&metadata_str).expect("metadata is always json"); + let metadata: serde_json::Value = match serde_json::from_str(&metadata_str) { + Ok(value) => value, + Err(_) => return (metadata_str, None, None, None, String::new()), + };
🧹 Nitpick comments (3)
crates/torii/sqlite/src/utils.rs (2)
119-123: Credentials stored in code
Storing IPFS credentials in versioned source is risky. Consider moving them to environment variables or a secrets manager to strengthen security.
127-154: Ohayo sensei, exponential backoff logic
This retry mechanism is straightforward and robust. Adding a maximum cap to backoff can prevent excessively long waits during persistent failures.crates/torii/graphql/src/object/erc/erc_token.rs (1)
479-480: Add bounds checking for token ID extraction sensei.The code assumes the split operation will always yield at least two parts. While the format is enforced by the database schema, it's good practice to add bounds checking.
Consider this safer approach:
- let token_id = id.split(':').collect::<Vec<&str>>()[1].to_string(); + let token_id = id.split(':') + .nth(1) + .ok_or_else(|| sqlx::Error::RowNotFound)? + .to_string();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/graphql/src/object/erc/erc_token.rs(2 hunks)crates/torii/server/src/artifacts.rs(6 hunks)crates/torii/sqlite/src/constants.rs(1 hunks)crates/torii/sqlite/src/utils.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: docs
🔇 Additional comments (13)
crates/torii/sqlite/src/utils.rs (5)
10-11: Ohayo sensei! Dependency import looks good
IntroducingLazyfromonce_cellandreqwest::Clientis a sound approach to reduce overhead and avoid re-initialization in every request.
18-18: Unified reference toREQ_MAX_RETRIES
The consolidation of constants ensures consistent retry behavior across HTTP and IPFS.
110-117: Ohayo sensei, global static HTTP client
Reusing a singlereqwest::Clientacross the application is typically more efficient than recreating it per request.
125-125: Initial backoff of 100ms
Starting with 100ms may or may not be sufficient, depending on rate-limiting constraints. Please verify this value against your service usage patterns.
156-175: Consistent IPFS retry logic
Synchronizing the IPFS fetching logic with the HTTP retry strategy promotes uniform error handling across different data sources.crates/torii/sqlite/src/constants.rs (1)
7-7: Ohayo sensei, nicely introducedREQ_MAX_RETRIES
Centralizing retry attempts under a single constant helps maintain consistency and clarity in retry-related configurations.crates/torii/server/src/artifacts.rs (6)
18-18: Importing fetch utilities
The explicit import offetch_content_from_httpandfetch_content_from_ipfsclarifies responsibilities and avoids code duplication.
61-61: Conditional fetch logic
Falling back tofetch_and_process_imagewhen no local directory exists is a neat approach to on-demand artifact generation.
176-177: HTTP fetch refactor
Forwarding all external requests throughfetch_content_from_httpcentralizes both error handling and retry logic.
187-188: Ohayo sensei, image format detection
Utilizingload_from_memory_with_formatis a solid upgrade to accurately determine the image type.
210-211: Improved IPFS image load
Harmonizes the IPFS image-loading flow with the same format detection improvements made for HTTP.
232-232: Consistent data URI handling
Applyingimage::load_from_memory_with_formatensures data URIs receive the same robust detection and loading approach.crates/torii/graphql/src/object/erc/erc_token.rs (1)
425-426:❓ Verification inconclusive
Ohayo sensei! The image path construction looks good but let's verify the token ID format.
The change to use token ID for image path construction is consistent across both locations. However, we should verify that the token ID format assumption (containing a colon) is always valid.
Let's verify the token ID format in the database:
Also applies to: 502-502
🏁 Script executed:
#!/bin/bash # Description: Check if all token IDs in the database follow the expected format (containing a colon) # Expected: All token IDs should contain a colon echo "Checking token ID format in the database..." sqlite3 torii.db "SELECT id FROM tokens WHERE contract_type='erc721' AND id NOT LIKE '%:%';"Length of output: 301
Ohayo sensei! The image path changes look consistent across the board, but please manually verify the underlying assumption regarding token ID formats.
- The update at lines 425-426 (and similarly at line 502) correctly constructs the image path by replacing a colon with a slash.
- However, because our automated check failed to run (due to the missing sqlite3 command), we need you to manually confirm that all ERC721 token IDs indeed include a colon.
- If there’s a possibility that any token ID might not follow the expected format, consider adding additional validation or tests to safeguard against potential edge cases.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3032 +/- ##
==========================================
+ Coverage 56.24% 56.27% +0.03%
==========================================
Files 436 437 +1
Lines 58829 58966 +137
==========================================
+ Hits 33087 33184 +97
- Misses 25742 25782 +40 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Chores