feat(torii-graphql): add erc1155 to union #3057
Conversation
|
Ohayo, sensei! WalkthroughThis pull request introduces comprehensive support for ERC1155 tokens in the GraphQL layer. It adds two new constants for ERC1155 token identification, a static type mapping for ERC1155 token attributes, and new object definitions—including dedicated structs and enum variants—to handle ERC1155 tokens. Additionally, the token metadata parsing and resolver logic are updated in both token creation and token balance processes to correctly extract and map ERC1155-specific data from database rows. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (7)
crates/torii/graphql/src/object/erc/erc_token.rs (6)
65-80: Ohayo sensei, new BasicObject looks great.The
Erc1155TokenObjectstructure parallels the pattern for other token objects. However, there's some duplication with the existing Erc20/Erc721. Consider a shared trait or macro if you'd like to reduce code repetition in the future.
111-123: Ohayo sensei, data fields look on point.
Erc1155Tokenfields match the specs for referencing metadata and are consistent with Erc20/Erc721. You might consider using numeric types foramountortoken_idif there's a potential for numeric operations, but if your domain logic stores them as strings, this is fine.
161-184: Ohayo sensei, your to_field_value expansion is working well.The approach for
Erc1155is in line with existing token logic. If you'd like to reduce duplication betweenErc721andErc1155, consider extracting shared metadata logic into a helper function.
509-567: Ohayo sensei, ephemeral subscription logic is consistent.This block for subscription updates is quite similar to Erc721. Reusing a helper that processes metadata might keep your code DRY and flexible.
637-679: Ohayo sensei, metadata extraction for ERC1155 is consistent.Everything is in harmony with ERC721 parsing. If repetition becomes unmanageable, factor out a shared routine to parse token metadata.
681-683: Ohayo sensei, your unknown contract error path is safe.Returning
RowNotFoundis straightforward, but you could consider a more explicit error (e.g.UnsupportedContractType) if you'd like more clarity.crates/torii/graphql/src/object/erc/token_balance.rs (1)
216-271: Consider extracting duplicated ERC1155 token handling logic.Ohayo sensei! The ERC1155 token handling logic is duplicated in both the subscription handler and connection output handler. Consider extracting this into a shared helper function to improve maintainability and reduce code duplication.
Here's a suggested implementation:
fn create_erc1155_token(row: &BalanceQueryResultRaw) -> ErcTokenType { let token_id = row.token_id.split(':').collect::<Vec<&str>>(); assert!(token_id.len() == 2); let metadata_str = row.metadata.clone(); let ( metadata_str, metadata_name, metadata_description, metadata_attributes, image_path, ) = if metadata_str.is_empty() { (String::new(), None, None, None, String::new()) } else { let metadata: serde_json::Value = serde_json::from_str(&metadata_str).expect("metadata is always json"); let metadata_name = metadata.get("name") .map(|v| v.to_string().trim_matches('"').to_string()); let metadata_description = metadata.get("description") .map(|v| v.to_string().trim_matches('"').to_string()); let metadata_attributes = metadata.get("attributes") .map(|v| v.to_string().trim_matches('"').to_string()); let image_path = format!("{}/{}", token_id.join("/"), "image"); ( metadata_str, metadata_name, metadata_description, metadata_attributes, image_path, ) }; let token_metadata = Erc1155Token { name: row.name.clone(), metadata: metadata_str, contract_address: row.contract_address.clone(), symbol: row.symbol.clone(), token_id: token_id[1].to_string(), amount: row.balance.clone(), metadata_name, metadata_description, metadata_attributes, image_path, }; ErcTokenType::Erc1155(token_metadata) }Then use it in both places:
- // In subscription handler and connection output handler - "erc1155" => { - // ... duplicated code ... - } + "erc1155" => create_erc1155_token(&row),Also applies to: 489-540
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/torii/graphql/src/constants.rs(2 hunks)crates/torii/graphql/src/mapping.rs(1 hunks)crates/torii/graphql/src/object/erc/erc_token.rs(6 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (7)
crates/torii/graphql/src/object/erc/erc_token.rs (3)
11-11: Ohayo sensei, this addition is neat.No issues with adding the
use tracing::warn;import. This aligns with the usage in the unknown contract type handling.
15-20: Ohayo sensei, sweet expansions to coverage.Adding the ERC1155 constants here is consistent. Everything is in line with existing patterns.
86-86: Ohayo sensei, the enum variant is well integrated.This extension of
ErcTokenTypeto includeErc1155is consistent with the code's architecture.crates/torii/graphql/src/constants.rs (2)
44-44: Ohayo sensei, your new constant is spiffy.Defining
ERC1155_TYPE_NAMEis consistent with the naming convention for other ERC token types.
59-59: Ohayo sensei, your new token name constant looks good.This addition aligns well with existing references for ERC1155 tokens. Perfect synergy.
crates/torii/graphql/src/mapping.rs (1)
180-192: Ohayo sensei, the new ERC1155 token type mapping is on point.This complements the existing mappings for ERC20 and ERC721. Nicely consistent.
crates/torii/graphql/src/object/erc/token_balance.rs (1)
25-25: LGTM!Ohayo! The import of
Erc1155Tokenis correctly added alongsideErc721Token.
| serde_json::from_str(&metadata_str) | ||
| .expect("metadata is always json"); |
There was a problem hiding this comment.
Replace expect() with proper error handling.
Ohayo sensei! Using expect() for JSON parsing could cause panics in production. Consider using proper error handling instead.
Here's a suggested implementation:
- serde_json::from_str(&metadata_str).expect("metadata is always json");
+ serde_json::from_str(&metadata_str).map_err(|e| {
+ sqlx::Error::Protocol(format!("Failed to parse metadata JSON: {}", e))
+ })?;Also applies to: 505-506
| let token_id = | ||
| row.token_id.split(':').collect::<Vec<&str>>(); | ||
| assert!(token_id.len() == 2); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate token ID format before splitting.
Ohayo sensei! The token ID splitting logic assumes a specific format without validation. Consider adding proper validation before splitting.
Here's a suggested implementation:
fn validate_and_split_token_id(token_id: &str) -> Result<Vec<&str>, sqlx::Error> {
let parts: Vec<&str> = token_id.split(':').collect();
if parts.len() != 2 {
return Err(sqlx::Error::Protocol(
format!("Invalid token ID format: {}. Expected format: contract_address:token_id", token_id)
));
}
Ok(parts)
}Then use it in the code:
- let token_id = row.token_id.split(':').collect::<Vec<&str>>();
- assert!(token_id.len() == 2);
+ let token_id = validate_and_split_token_id(&row.token_id)?;Also applies to: 490-492
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3057 +/- ##
==========================================
- Coverage 57.65% 57.46% -0.20%
==========================================
Files 439 439
Lines 59437 59657 +220
==========================================
+ Hits 34267 34279 +12
- Misses 25170 25378 +208 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/graphql/src/schema.rs (1)
145-148: Consider documenting the token type order, sensei! 📝The token types in the union follow a specific order (ERC20 -> ERC721 -> ERC1155). While this works correctly, it would be helpful to document whether this order is significant for the application logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/graphql/src/schema.rs(3 hunks)
🔇 Additional comments (3)
crates/torii/graphql/src/schema.rs (3)
14-15: Ohayo! The import changes look great, sensei! 🎉The imports are well-organized and correctly include the new ERC1155 token support.
Also applies to: 19-21
136-136: Nice work registering the ERC1155 object, sensei! ✨The Erc1155TokenObject is properly registered as a Basic variant, maintaining consistency with other ERC token objects.
147-148: Excellent union type extension, sensei! 🌟The ERC1155 type is properly added to the token union, enabling GraphQL queries to handle ERC1155 tokens alongside ERC20 and ERC721 tokens.
Summary by CodeRabbit