fix(torii): avoid panic on fetch token URI fail #2750
Conversation
In this occasions, the token URI may not be fetched correctly. Since it's mostly use for metadata, a default metadata is used in this scenario.
WalkthroughOhayo, sensei! The recent changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ERC
participant MetadataService
User->>ERC: Register ERC721 Token
ERC->>ERC: Process Register Token
ERC->>ERC: Fetch Token URI
alt Token URI Fetch Success
ERC->>MetadataService: Fetch Metadata
else Token URI Fetch Failure
ERC->>ERC: Log Warning
ERC->>ERC: Assign Empty String to Metadata
end
ERC->>User: Return Metadata
🪧 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 and nitpick comments (2)
crates/torii/core/src/executor/erc.rs (2)
179-186: LGTM! Robust error handling with informative logging.The warning log provides excellent context with both contract address and token ID. The approach to continue with empty metadata aligns perfectly with the PR's objective to avoid panics.
A tiny suggestion to make the warning message even more helpful:
- "Error fetching token URI, empty metadata will be used instead.", + "Failed to fetch token URI (tried both token_uri and tokenURI), falling back to empty metadata.",
202-213: Consider adding debug logging for metadata fetch outcomes.Ohayo sensei! The metadata handling logic is robust, but we could improve observability:
let metadata = if token_uri.is_empty() { + debug!("Using empty metadata for empty token URI"); "".to_string() } else { let metadata = Self::fetch_metadata(&token_uri).await.with_context(|| { format!( "Failed to fetch metadata for token_id: {}", register_erc721_token.actual_token_id ) })?; + debug!( + token_id = %register_erc721_token.actual_token_id, + "Successfully fetched and parsed metadata" + ); serde_json::to_string(&metadata).context("Failed to serialize metadata")? };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/core/src/executor/erc.rs(3 hunks)
🔇 Additional comments (1)
crates/torii/core/src/executor/erc.rs (1)
13-13: LGTM! Import addition aligns with new logging.
Ohayo! The addition of warn to the tracing imports is well-placed and necessary for the new error handling approach.
|
will wait feedback from @ponderingdemocritus on |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2750 +/- ##
==========================================
- Coverage 56.10% 56.10% -0.01%
==========================================
Files 427 427
Lines 54516 54522 +6
==========================================
+ Hits 30586 30587 +1
- Misses 23930 23935 +5 ☔ View full report in Codecov by Sentry. |
fix: ensure torii doesn't stop on fetch URI fail In this occasions, the token URI may not be fetched correctly. Since it's mostly use for metadata, a default metadata is used in this scenario.
In some occasions, the token URI may not be fetched correctly. Since it's mostly use for metadata, a default metadata is used in this scenario.
Summary by CodeRabbit
Bug Fixes
Improvements