fix(torii/graphql): handle empty metadata and schema change #2731
Conversation
WalkthroughOhayo, sensei! This pull request introduces several modifications to type mappings and struct definitions in the GraphQL implementation. Key changes include updating the nullability of fields in the Changes
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: 3
🧹 Outside diff range and nitpick comments (6)
crates/torii/graphql/src/object/erc/token_balance.rs (2)
237-242: Consider adding debug logs for skipped tokens, senseiWhile safely handling null metadata by skipping tokens is good, it might be helpful to add debug logging when tokens are skipped. This would help with troubleshooting and monitoring the frequency of missing metadata.
if row.metadata.is_none() { + tracing::debug!( + "Skipping token with id {} due to missing metadata", + row.token_id + ); continue; }
244-244: Enhance error message for JSON parsing, senseiThe expect message could be more specific to help with debugging.
- serde_json::from_str(metadata_str).expect("metadata is always json"); + serde_json::from_str(metadata_str).expect("Failed to parse token metadata JSON");crates/torii/graphql/src/object/erc/token_transfer.rs (2)
266-270: Ohayo! Consider adding observability for skipped tokensWhile silently skipping tokens with null metadata is a valid approach, it would be helpful to add logging or metrics to track how often this occurs. This information could be valuable for monitoring and debugging.
if row.metadata.is_none() { + warn!("Skipping token transfer for token_id {} due to null metadata", row.token_id); continue; }
286-286: Optimize metadata string handlingThe
to_owned()call creates an unnecessary clone sincemetadata_stris already a reference. We can usemetadata_str.to_string()or simply dereference it.-metadata: metadata_str.to_owned(), +metadata: (*metadata_str).to_string(),crates/torii/core/src/executor/erc.rs (2)
179-183: Enhance error logging for better debugging, sensei!While the error handling is good, we could make debugging easier by including the contract address in the error message.
error!( - "Failed to fetch token_uri for token_id: {}", - register_erc721_token.actual_token_id + "Failed to fetch token_uri for token_id: {} from contract: {}", + register_erc721_token.actual_token_id, + register_erc721_token.contract_address );
199-204: Consider extracting common error logging pattern, sensei!There's a repeated pattern of error logging with token_id. Consider extracting this into a helper method for better maintainability.
+impl<'c, P: Provider + Sync + Send + 'static> Executor<'c, P> { + fn log_token_error(token_id: &U256, message: &str, error: Option<&anyhow::Error>) { + match error { + Some(e) => error!("{} for token_id: {}, error: {}", message, token_id, e), + None => error!("{} for token_id: {}", message, token_id), + } + } +} // Usage: -error!( - "token_uri is neither ByteArray nor Array<Felt> for token_id: {}", - register_erc721_token.actual_token_id -); +Self::log_token_error( + ®ister_erc721_token.actual_token_id, + "token_uri is neither ByteArray nor Array<Felt>", + None +); -error!( - "Failed to fetch metadata for token_id: {}, error: {}", - register_erc721_token.actual_token_id, e -); +Self::log_token_error( + ®ister_erc721_token.actual_token_id, + "Failed to fetch metadata", + Some(&e) +);Also applies to: 214-219
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/torii/core/src/executor/erc.rs(4 hunks)crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(3 hunks)crates/torii/graphql/src/object/erc/token_transfer.rs(3 hunks)
🔇 Additional comments (7)
crates/torii/graphql/src/mapping.rs (3)
103-104: Ohayo! Nice improvement to pagination type safety, sensei!
Making hasPreviousPage and hasNextPage non-nullable is a good practice as these pagination flags should always have a definite value.
174-176: Ohayo! Good handling of optional metadata fields, sensei!
Making metadata fields nullable is a good change that aligns with real-world scenarios where NFT metadata might not always be available.
163-163: Ohayo! Please verify the impact of the decimals type change, sensei!
Changing decimals from STRING to INT is more semantically correct for ERC20 tokens. However, this is a breaking change that might affect clients.
Let's check for any direct usage of the decimals field:
✅ Verification successful
Let me search for more specific patterns to understand the impact better:
Ohayo! The decimals type change is safe to proceed, sensei!
The codebase consistently uses u8 for decimals across all internal representations:
- Core types use
u8incore/src/types.rs - GraphQL objects use
u8ingraphql/src/object/erc/erc_token.rs - GRPC types use
u8ingrpc/src/types/mod.rs
The change from STRING to INT in the GraphQL schema actually aligns better with the internal representation, making it more type-safe. No client code was found using the string representation, suggesting this change won't cause breaking issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of the decimals field that might be affected
rg -A 3 "decimals.*String" --type rust
Length of output: 40
Script:
#!/bin/bash
# Let's search for any usage of decimals field in a broader context
rg -i "decimals" --type rust -A 3
# Also check for any GraphQL schema definitions that might be affected
fd ".graphql$" --exec cat {}
Length of output: 13711
Script:
#!/bin/bash
# Search for type definitions and resolvers related to decimals
ast-grep --pattern 'fn decimals'
# Look for any test cases that might need updates
rg "test.*decimals" --type rust -A 3
Length of output: 73
crates/torii/graphql/src/object/erc/token_balance.rs (1)
304-304: Ohayo! Nice type safety improvement, sensei!
Making metadata an Option<String> is a great change that explicitly handles cases where token metadata might not exist.
crates/torii/graphql/src/object/erc/token_transfer.rs (1)
342-342: LGTM! Good handling of optional metadata
The change to make metadata optional (Option<String>) aligns well with the PR objective to handle cases where token_id no longer exists.
crates/torii/core/src/executor/erc.rs (2)
33-33: Ohayo sensei! Nice improvement to the metadata field type!
Making metadata optional is the right approach as NFT metadata might not always be available, especially when the token_id no longer exists.
186-238: Verify error handling strategy for critical failures
While gracefully handling failures is good, let's ensure we're not silently swallowing critical errors that might indicate larger issues (e.g., contract implementation problems).
✅ Verification successful
Let me gather more context about the error handling patterns to make a complete assessment.
Based on the analysis of the error handling patterns, I can now provide a final response:
Error handling strategy is appropriate for metadata operations
The current error handling approach is well-designed because:
- Non-critical metadata failures (token_uri parsing, metadata fetching) are properly logged and gracefully degraded to
None - Critical contract operations use proper error propagation with
?operator - All error cases are logged with appropriate context for debugging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for patterns that might indicate critical contract issues
# Look for similar error patterns in other files
rg -A 3 "Failed to fetch (token_uri|metadata)"
# Look for contract verification or validation patterns
ast-grep --pattern 'fn $_(contract_address: $_, $$$) {
$$$
.call($$$)
$$$
}'
Length of output: 1818
Script:
#!/bin/bash
# Look for error handling patterns in the ERC module
rg -A 5 "error!|warn!" crates/torii/core/src/executor/erc.rs
# Check if there's any retry logic or fallback mechanisms
ast-grep --pattern 'match $_ {
Ok($_) => $_,
Err($_) => {
$$$
}
}'
# Look for other metadata-related error handling
rg -A 3 "metadata.*Error|Error.*metadata"
Length of output: 121818
| (Name::new("metadataAttributes"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))), | ||
| (Name::new("metadataName"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| (Name::new("metadataDescription"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| (Name::new("metadataAttributes"), TypeData::Simple(TypeRef::named(TypeRef::STRING))), | ||
| (Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))), |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! The imagePath should indeed be nullable, sensei!
Looking at the codebase, image_path is constructed using a simple format string: format!("{}/{}", token_id.join("/"), "image"). This is a static path construction that doesn't verify the actual existence of the image, while other metadata fields (name, description, attributes) are properly wrapped in Option. For consistency and correctness, it should be nullable.
- In
crates/torii/graphql/src/object/erc/erc_token.rs,image_pathis defined as a non-optionalString - In
crates/torii/graphql/src/mapping.rs, it's mapped as non-nullableTypeRef::named_nn - The path construction in both
token_transfer.rsandtoken_balance.rsdoesn't validate image existence
-(Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
+(Name::new("imagePath"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),🔗 Analysis chain
Consider making imagePath nullable for consistency
While the metadata fields are now properly nullable, imagePath remains non-nullable (TypeRef::named_nn). Should this also be nullable since it's part of the metadata that might not be available?
Let's check if imagePath is always guaranteed to be present:
Consider making imagePath nullable for consistency with other metadata fields:
-(Name::new("imagePath"), TypeData::Simple(TypeRef::named_nn(TypeRef::STRING))),
+(Name::new("imagePath"), TypeData::Simple(TypeRef::named(TypeRef::STRING))),🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imagePath handling to understand if it's always present
rg -A 5 "imagePath.*Some" --type rust
rg -A 5 "imagePath.*None" --type rust
Length of output: 79
Script:
#!/bin/bash
# Let's search for imagePath usage in the codebase more broadly
rg "imagePath" --type rust -B 2 -A 2
# Also check for any struct definitions or types that might contain imagePath
ast-grep --pattern 'struct $_ {
$$$
image_path: $_,
$$$
}'
Length of output: 1080
Script:
#!/bin/bash
# Let's check the token struct definition to see how image_path is defined
rg "image_path" --type rust -B 5 -A 5
# Also check for any Option wrapping of image_path in struct definitions
ast-grep --pattern 'struct $_ {
$$$
image_path: Option<$_>,
$$$
}'
Length of output: 6109
| if row.metadata.is_none() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Strengthen token_id parsing robustness, sensei
The current token_id parsing using split and assert could panic if the format is unexpected. Consider using a more robust approach.
-let token_id = row.token_id.split(':').collect::<Vec<&str>>();
-assert!(token_id.len() == 2);
+let token_id: Vec<&str> = row.token_id.split(':').collect();
+if token_id.len() != 2 {
+ warn!(
+ "Skipping token due to invalid token_id format: {}",
+ row.token_id
+ );
+ continue;
+}Committable suggestion skipped: line range outside the PR's diff.
| let metadata_str = row.metadata.as_ref().unwrap(); | ||
| let metadata: serde_json::Value = | ||
| serde_json::from_str(&row.metadata).expect("metadata is always json"); | ||
| serde_json::from_str(metadata_str).expect("metadata is always json"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for metadata JSON parsing, sensei!
The current implementation uses expect() which could panic if the metadata JSON is malformed. Consider using serde_json::from_str().map_err() to handle parsing errors gracefully.
-let metadata_str = row.metadata.as_ref().unwrap();
-let metadata: serde_json::Value =
- serde_json::from_str(metadata_str).expect("metadata is always json");
+let metadata_str = row.metadata.as_ref().unwrap();
+let metadata: serde_json::Value = match serde_json::from_str(metadata_str) {
+ Ok(value) => value,
+ Err(e) => {
+ warn!("Failed to parse metadata JSON for token_id {}: {}", row.token_id, e);
+ continue;
+ }
+};📝 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.
| let metadata_str = row.metadata.as_ref().unwrap(); | |
| let metadata: serde_json::Value = | |
| serde_json::from_str(&row.metadata).expect("metadata is always json"); | |
| serde_json::from_str(metadata_str).expect("metadata is always json"); | |
| let metadata_str = row.metadata.as_ref().unwrap(); | |
| let metadata: serde_json::Value = match serde_json::from_str(metadata_str) { | |
| Ok(value) => value, | |
| Err(e) => { | |
| warn!("Failed to parse metadata JSON for token_id {}: {}", row.token_id, e); | |
| continue; | |
| } | |
| }; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 56.10% 56.09% -0.02%
==========================================
Files 427 427
Lines 54522 54532 +10
==========================================
Hits 30587 30587
- Misses 23935 23945 +10 ☔ View full report in Codecov by Sentry. |
| @@ -30,7 +30,7 @@ pub struct RegisterErc721TokenMetadata { | |||
| pub query: RegisterErc721TokenQuery, | |||
| pub name: String, | |||
| pub symbol: String, | |||
| pub metadata: String, | |||
| pub metadata: Option<String>, | |||
There was a problem hiding this comment.
What is the case when this actually happens?
There was a problem hiding this comment.
there are two cases:
- nft got burnt but we still need to process the transfers
- the metadata json stored in the contract is invalid
lambda-0x
force-pushed
the
spr/main/e92e39b0
branch
from
d1c7629 to
752a88e
Compare
…ql changes commit-id:e92e39b0
lambda-0x
force-pushed
the
spr/main/e92e39b0
branch
from
752a88e to
acf546b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/graphql/src/object/erc/token_balance.rs (1)
237-242: Add logging for skipped tokens, senseiWhile the null check is good, silently skipping tokens might hide issues from users. Consider adding a warning log when skipping tokens due to null metadata.
if row.metadata.is_none() { + warn!( + "Skipping token due to null metadata. Token ID: {}", + row.token_id + ); continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(3 hunks)crates/torii/graphql/src/object/erc/token_transfer.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/object/erc/token_transfer.rs
- crates/torii/graphql/src/mapping.rs
🔇 Additional comments (3)
crates/torii/graphql/src/object/erc/token_balance.rs (3)
304-304: Ohayo! LGTM on the metadata field change, sensei!
The change to Option<String> properly handles cases where metadata might be null, making the code more robust.
257-257: LGTM on metadata string handling, sensei!
The use of to_owned() ensures proper ownership of the metadata string.
238-240:
Strengthen token_id parsing robustness, sensei
The current token_id parsing using split and assert could panic if the format is unexpected. Consider using a more robust approach.
-let token_id = row.token_id.split(':').collect::<Vec<&str>>();
-assert!(token_id.len() == 2);
+let token_id: Vec<&str> = row.token_id.split(':').collect();
+if token_id.len() != 2 {
+ warn!(
+ "Skipping token due to invalid token_id format: {}",
+ row.token_id
+ );
+ continue;
+}
lambda-0x
changed the title
lambda-0x
changed the title
|
✓ Commit merged in pull request #2733 |
Stack:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation