feat(torii-erc): add avif support #3011
Conversation
WalkthroughOhayo, sensei! This pull request updates constant declarations, error handling, and client retry logic in the sqlite module. A new public constant Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
🪧 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 (7)
crates/torii/sqlite/src/utils.rs (5)
133-133: Ohayo sensei, consider breaking this line up for readability.
The pipeline has flagged this line’s length as exceeding recommended limits. Splitting it into multiple shorter lines can make it easier to maintain.🧰 Tools
🪛 GitHub Actions: ci
[warning] 133-133: Line exceeds the recommended length. Consider breaking it into multiple lines for better readability.
144-144: Add the URL to your debug log.
Incorporating the URL helps diagnose failures by clearly indicating which endpoint caused the error.- debug!(error = %e, retry = retries, "Request failed, retrying after backoff"); + debug!(error = %e, retry = retries, url = %url, "Request failed, retrying after backoff");
165-165: Include the IPFS CID in the debug statement.
This additional context makes troubleshooting easier when retries fail on a specific resource.- debug!(error = %e, retry = retries, "Request failed, retrying after backoff"); + debug!(error = %e, retry = retries, cid = %cid, "Request failed, retrying after backoff");
284-284: Sensei, the pipeline suggests splitting this line.
Excessively long lines hinder readability. Consider splitting or restructuring it.
310-310: Sensei, please shorten or split this line for improved clarity.
Following line-length guidelines makes the code more comfortable to maintain.crates/torii/sqlite/src/executor/erc.rs (2)
278-279: Beware of logging full response data.
Large or sensitive payloads in logs can cause security or performance issues. Consider limiting the data size or omitting the raw bytes.- .context(format!("Failed to parse metadata JSON from response: {:?}", bytes))? + .context(format!("Failed to parse metadata JSON (length: {})", bytes.len()))?
288-288: Likewise, consider limiting logged IPFS response data.
Full prints of the downloaded data might be too large and could risk leaking information.- .context(format!("Failed to parse metadata JSON from IPFS: {:?}, data: {:?}", cid, bytes)) + .context(format!("Failed to parse metadata JSON from IPFS: {}, data length: {}", cid, bytes.len()))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/sqlite/src/constants.rs(1 hunks)crates/torii/sqlite/src/executor/erc.rs(8 hunks)crates/torii/sqlite/src/utils.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/executor/erc.rs
[warning] 17-17: Line exceeds the recommended length. Consider breaking it into multiple lines for better readability.
[warning] 284-284: Line exceeds the recommended length. Consider breaking it into multiple lines for better readability.
[warning] 310-310: Line exceeds the recommended length. Consider breaking it into multiple lines for better readability.
crates/torii/sqlite/src/utils.rs
[warning] 133-133: Line exceeds the recommended length. Consider breaking it into multiple lines for better readability.
🔇 Additional comments (6)
crates/torii/sqlite/src/utils.rs (1)
111-117: Ohayo sensei! Great job introducing a global HTTP client.
Centralizing the client configuration helps ensure consistent timeouts while reducing repeated instantiations.crates/torii/sqlite/src/constants.rs (1)
7-7: Ohayo sensei, consolidating retry logic is awesome!
Defining a single retry constant (REQ_MAX_RETRIES) simplifies updates and promotes consistency.crates/torii/sqlite/src/executor/erc.rs (4)
77-77: Ohayo sensei, nice usage of.context(...)for error propagation.
Replacing.with_context()with.context(...)is consistent and concise, making your error handling cleaner.Also applies to: 96-96, 144-144, 343-343
256-256: Includingtoken_uriin the warning log is helpful.
Thanks for adding extra context when metadata fails to fetch.
273-279: Nice re-use offetch_content_from_http.
This centralization reduces code repetition and ensures uniform error handling.
314-317: Ohayo sensei, the data URI parsing looks thorough!
Your error context is clear and should help identify bugs in metadata quickly.
| static IPFS_CLIENT: Lazy<IpfsClient> = Lazy::new(|| { | ||
| IpfsClient::from_str(IPFS_CLIENT_URL) | ||
| .expect("Failed to create IPFS client") | ||
| .with_credentials(IPFS_CLIENT_USERNAME, IPFS_CLIENT_PASSWORD) | ||
| }); |
There was a problem hiding this comment.
Ohayo sensei, storing credentials as constants is risky.
Hardcoding credentials can lead to accidental exposure and security vulnerabilities. Consider moving these credentials to environment variables or another secure store.
-static IPFS_CLIENT: Lazy<IpfsClient> = Lazy::new(|| {
- IpfsClient::from_str(IPFS_CLIENT_URL)
- .expect("Failed to create IPFS client")
- .with_credentials(IPFS_CLIENT_USERNAME, IPFS_CLIENT_PASSWORD)
-});
+// Example approach using environment variables
+static IPFS_CLIENT: Lazy<IpfsClient> = Lazy::new(|| {
+ let url = std::env::var("IPFS_CLIENT_URL").expect("Missing IPFS_CLIENT_URL env var");
+ let username = std::env::var("IPFS_CLIENT_USERNAME").unwrap_or_default();
+ let password = std::env::var("IPFS_CLIENT_PASSWORD").unwrap_or_default();
+
+ IpfsClient::from_str(&url)
+ .expect("Failed to create IPFS client")
+ .with_credentials(&username, &password)
+});📝 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.
| static IPFS_CLIENT: Lazy<IpfsClient> = Lazy::new(|| { | |
| IpfsClient::from_str(IPFS_CLIENT_URL) | |
| .expect("Failed to create IPFS client") | |
| .with_credentials(IPFS_CLIENT_USERNAME, IPFS_CLIENT_PASSWORD) | |
| }); | |
| // Example approach using environment variables | |
| static IPFS_CLIENT: Lazy<IpfsClient> = Lazy::new(|| { | |
| let url = std::env::var("IPFS_CLIENT_URL").expect("Missing IPFS_CLIENT_URL env var"); | |
| let username = std::env::var("IPFS_CLIENT_USERNAME").unwrap_or_default(); | |
| let password = std::env::var("IPFS_CLIENT_PASSWORD").unwrap_or_default(); | |
| IpfsClient::from_str(&url) | |
| .expect("Failed to create IPFS client") | |
| .with_credentials(&username, &password) | |
| }); |
Larkooo
changed the title
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/torii/server/src/artifacts.rs(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/server/src/artifacts.rs
[warning] 58-58: Code formatting issue: Await should be on the same line as the function call.
[warning] 177-177: Code formatting issue: Await should be on the same line as the function call.
🔇 Additional comments (4)
crates/torii/server/src/artifacts.rs (3)
18-18: LGTM! Clean import refactoring.Ohayo! The import changes nicely consolidate the HTTP and IPFS fetching utilities.
180-182: LGTM! Improved HTTP fetching with retries.The HTTP fetching logic is now more robust with built-in retry mechanism.
215-215: LGTM! More explicit image format handling.Using
load_from_memory_with_formatis safer as it ensures the correct format is used for loading the image.Cargo.toml (1)
187-187: LGTM! Added AVIF image support.Ohayo sensei! Adding the
avif-nativefeature is great as AVIF offers superior compression while maintaining high image quality.
crates/torii/server/src/artifacts.rs
Outdated
| @@ -173,19 +171,15 @@ async fn fetch_and_process_image( | |||
| .with_context(|| format!("Image field not a string for token_id: {}", token_id))? | |||
| .to_string(); | |||
|
|
|||
| println!("image_uri: {}", image_uri); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statements.
Ohayo sensei! These debug print statements should be replaced with proper logging using the tracing crate that's already imported.
- println!("image_uri: {}", image_uri);
+ debug!(image_uri = %image_uri, "Processing image URI");
- println!("image_type: {:?}", image_type);
+ debug!(image_type = ?image_type, "Determined image type");Also applies to: 247-247
|
#3028 addresses this also i think |
Larkooo
changed the title
Summary by CodeRabbit
New Features
REQ_MAX_RETRIESto define maximum retry attempts for requests.imagedependency to include the newavif-nativefeature for enhanced configuration options.Refactor
image_pathforErc721Tokenobjects to ensure consistency in its derivation from the token's ID.