feat(torii-graphql): subscriptions for tokens and token balances #2999
Conversation
Larkooo
force-pushed
the
torii-graphql-tokens
branch
from
4de044d to
4263ab9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2999 +/- ##
==========================================
- Coverage 56.96% 56.62% -0.34%
==========================================
Files 430 431 +1
Lines 57052 57637 +585
==========================================
+ Hits 32500 32638 +138
- Misses 24552 24999 +447 ☔ View full report in Codecov by Sentry. |
WalkthroughOhayo, sensei! This pull request renames a token-related constant from Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant GE as GraphQL Engine
participant TO as TokenObject Resolver
participant DB as SQLite DB
C->>GE: Subscribe to tokenUpdated
GE->>TO: Invoke tokenUpdated subscription
TO->>DB: Query token data (contract type)
DB->>TO: Return token data (Erc20/Erc721 details)
TO->>GE: Stream token update response
GE->>C: Deliver token update
sequenceDiagram
participant C as Client
participant GE as GraphQL Engine
participant EB as ErcBalanceObject Resolver
participant DB as SQLite DB
C->>GE: Subscribe to tokenBalanceUpdated with accountAddress
GE->>EB: Invoke tokenBalanceUpdated subscription
EB->>DB: Execute SQL query for token balance
DB->>EB: Return token balance data
EB->>GE: Create subscription output (edge update)
GE->>C: Stream token balance update
Possibly related issues
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
🧹 Nitpick comments (4)
crates/torii/graphql/src/object/erc/erc_token.rs (2)
117-132: Ohayo sensei, consider unifying the naming convention with your union type.
Right now,type_name(&self) -> "Token"can be confusing when you've also introduced"ERC__Token"in the constants. Harmonizing both layers would reduce potential ambiguity in your schema.
134-253: Ohayo sensei, add some logging for dropped errors to improve observability.
When your DB query fails (line 161) or you encounter an unrecognizedcontract_type(line 229), the code currently discards the event by returningNone. Consider the following diff to log these cases before returning:- Err(_) => return None, + Err(err) => { + tracing::warn!("Failed to fetch row for token {}: {:?}", token.id, err); + return None; + } - _ => return None, + _ => { + tracing::warn!("Encountered unknown contract type: {}", contract_type); + return None; + }This helps with debugging, especially when subscription updates fail unexpectedly.
crates/torii/graphql/src/object/erc/token_balance.rs (1)
93-162: Ohayo sensei, handle subscription query errors more gracefully.
If thesqlxquery fails (line 137), the code silently drops the update. Logging this error will make troubleshooting much easier, as shown here:- Err(_) => return None, + Err(err) => { + tracing::warn!("Failed to fetch row for token balance {}: {:?}", token_balance.id, err); + return None; + }Also, if this subscription logic duplicates parts of
tokenUpdated, consider refactoring for better reuse.crates/torii/graphql/src/mapping.rs (1)
194-201: Well-defined token type mapping!The new
TOKEN_TYPE_MAPPINGincludes all essential fields with appropriate non-nullable types. Consider adding documentation comments to describe the purpose and usage of this mapping, similar to the TODO comment on the metadata mapping.Add a documentation comment like this:
+ // Mapping for token types, supporting both ERC20 and ERC721 tokens through a union type pub static ref TOKEN_TYPE_MAPPING: TypeMapping = IndexMap::from([
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/graphql/src/constants.rs(1 hunks)crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/erc/erc_token.rs(2 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(2 hunks)crates/torii/graphql/src/schema.rs(2 hunks)
🔇 Additional comments (7)
crates/torii/graphql/src/object/erc/erc_token.rs (1)
1-3: Ohayo sensei, these imports nicely set up your subscriptions and DB interactions!
Everything looks consistent for handling GraphQL dynamic fields, subscription fields, and SQLx usage.Also applies to: 5-8, 11-12
crates/torii/graphql/src/object/erc/token_balance.rs (1)
2-3: Ohayo sensei, the newly added subscription/broker imports fit right in.
They look good and set the stage for your real-time token balance updates.Also applies to: 10-10, 12-13
crates/torii/graphql/src/constants.rs (1)
39-39: Ohayo sensei, the newTOKEN_UNION_TYPE_NAMEconstant is well introduced!
This consistent naming across your schema clarifies the union type for tokens.crates/torii/graphql/src/schema.rs (3)
15-15: LGTM! Import changes align with token subscriptions feature.The renaming to
TOKEN_UNION_TYPE_NAMEand addition ofTokenObjectsupport the new token subscriptions functionality.Also applies to: 19-19
134-134: Ohayo! Nice addition of TokenObject as a Resolvable object, sensei!The
TokenObjectis correctly registered as a Resolvable object, which is essential for implementing the subscription functionality.
142-144: Well-structured union type definition!The union type correctly includes both ERC20 and ERC721 token types, providing a flexible type system for token subscriptions.
crates/torii/graphql/src/mapping.rs (1)
149-149: Consistent type mapping updates, sensei!The token balance and transfer mappings have been correctly updated to use
TOKEN_UNION_TYPE_NAME, maintaining consistency across the schema.Also applies to: 156-156
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/torii/graphql/src/object/erc/token_balance.rs (2)
141-144: Consider enhancing error handling, sensei!The error handling for
BalanceQueryResultRaw::from_rowcould be more informative.Consider adding context to the error:
- let row = match BalanceQueryResultRaw::from_row(&row) { - Ok(row) => row, - Err(_) => return None, - }; + let row = match BalanceQueryResultRaw::from_row(&row) { + Ok(row) => row, + Err(e) => { + tracing::error!("Failed to parse balance query result: {}", e); + return None; + } + };
94-228: Consider subscription performance implications, sensei!The subscription implementation queries the database for each token balance update. This could lead to performance issues with many subscribers.
Consider implementing:
- Caching frequently accessed token data
- Batching database queries
- Rate limiting for subscriptions
crates/torii/graphql/src/object/erc/erc_token.rs (1)
358-461: Consider subscription performance, sensei!The subscription implementation could benefit from some optimizations.
Consider implementing:
- Connection pooling for database queries
- Caching token metadata
- Rate limiting for subscriptions
- Batching database queries when multiple tokens are updated simultaneously
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/torii/graphql/src/mapping.rs(3 hunks)crates/torii/graphql/src/object/connection/mod.rs(1 hunks)crates/torii/graphql/src/object/erc/erc_token.rs(2 hunks)crates/torii/graphql/src/object/erc/token_balance.rs(2 hunks)crates/torii/graphql/src/schema.rs(3 hunks)crates/torii/sqlite/src/executor/erc.rs(3 hunks)crates/torii/sqlite/src/executor/mod.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/graphql/src/mapping.rs
- crates/torii/graphql/src/schema.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ensure-wasm
- GitHub Check: build
🔇 Additional comments (8)
crates/torii/sqlite/src/executor/erc.rs (2)
15-15: Ohayo, sensei! The imports look good!The new imports for
BrokerMessageandTokentypes are correctly added to support the token registration and subscription functionality.Also applies to: 19-19
339-364: Ohayo, sensei! Nice improvements to token registration!The changes enhance type safety and add subscription support:
- Using
sqlx::query_as::<_, Token>for type-safe queries.- Properly handling optional token fetch.
- Adding token registration message for real-time updates.
crates/torii/sqlite/src/executor/mod.rs (4)
26-26: Ohayo, sensei! The import looks good!The new import for the
Tokentype is correctly added to support the token registration functionality.
51-51: Ohayo, sensei! The broker message looks good!The new
TokenRegistered(Token)variant is correctly added to support token registration subscriptions.
704-722: Ohayo, sensei! Nice improvements to ERC20 token registration!The changes enhance type safety and add subscription support:
- Using
sqlx::query_as::<_, Token>for type-safe queries.- Adding token registration message for real-time updates.
842-842: Ohayo, sensei! The broker message handling looks good!The new
TokenRegisteredvariant is correctly handled to publish token registration events.crates/torii/graphql/src/object/connection/mod.rs (1)
74-74: Ohayo! Nice type annotation addition, sensei!The explicit type annotation
Option<u64>improves code clarity and maintainability.crates/torii/graphql/src/object/erc/token_balance.rs (1)
94-228:⚠️ Potential issueOhayo! Let's secure this subscription implementation, sensei!
The SQL query in the subscription is vulnerable to SQL injection as it directly interpolates the token balance ID into the query string.
Replace string interpolation with parameterized query:
- let query = format!( - "SELECT b.id, t.contract_address, t.name, t.symbol, \ - t.decimals, b.balance, b.token_id, t.metadata, \ - c.contract_type - FROM {} b - JOIN tokens t ON b.token_id = t.id - JOIN contracts c ON t.contract_address = \ - c.contract_address - WHERE b.id = ?", - TOKEN_BALANCE_TABLE - ); + let query = format!( + "SELECT b.id, t.contract_address, t.name, t.symbol, \ + t.decimals, b.balance, b.token_id, t.metadata, \ + c.contract_type + FROM {table} b + JOIN tokens t ON b.token_id = t.id + JOIN contracts c ON t.contract_address = \ + c.contract_address + WHERE b.id = ?", + table = TOKEN_BALANCE_TABLE + );Likely an incorrect or invalid review comment.
|
|
||
| let mut conditions = Vec::new(); | ||
| if let Some(addr) = contract_address { | ||
| conditions.push(format!("t.contract_address = '{}'", addr)); |
There was a problem hiding this comment.
Ohayo! Let's secure this query, sensei!
The SQL query is vulnerable to SQL injection as it directly interpolates the contract address.
Use parameterized query instead:
- conditions.push(format!("t.contract_address = '{}'", addr));
+ conditions.push("t.contract_address = ?".to_string());And update the query execution:
let mut query = sqlx::query(&query);
if let Some(addr) = contract_address {
query = query.bind(addr);
}| fn create_token_metadata_from_row(row: &SqliteRow) -> sqlx::Result<ErcTokenType> { | ||
| let contract_type: String = row.get("contract_type"); | ||
|
|
||
| Ok(match contract_type.to_lowercase().as_str() { | ||
| "erc20" => { | ||
| let token = Erc20Token { | ||
| contract_address: row.get("contract_address"), | ||
| name: row.get("name"), | ||
| symbol: row.get("symbol"), | ||
| decimals: row.get("decimals"), | ||
| amount: "0".to_string(), | ||
| }; | ||
| ErcTokenType::Erc20(token) | ||
| } | ||
| "erc721" => { | ||
| // contract_address:token_id | ||
| let id = row.get::<String, _>("id"); | ||
| let token_id = id.split(':').collect::<Vec<&str>>()[1].to_string(); | ||
|
|
||
| let metadata_str: String = row.get("metadata"); | ||
| 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 contract_address: String = row.get("contract_address"); | ||
| let image_path = format!("{}/image", contract_address); | ||
|
|
||
| (metadata_str, metadata_name, metadata_description, metadata_attributes, image_path) | ||
| }; | ||
|
|
||
| let token = Erc721Token { | ||
| name: row.get("name"), | ||
| metadata: metadata_str, | ||
| contract_address: row.get("contract_address"), | ||
| symbol: row.get("symbol"), | ||
| token_id, | ||
| metadata_name, | ||
| metadata_description, | ||
| metadata_attributes, | ||
| image_path, | ||
| }; | ||
| ErcTokenType::Erc721(token) | ||
| } | ||
| _ => return Err(sqlx::Error::RowNotFound), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's reduce code duplication, sensei!
The token metadata creation logic is duplicated between create_token_metadata_from_row and the subscription handler.
Extract the metadata parsing logic into a separate function:
fn parse_metadata(metadata_str: &str, contract_address: &str) -> (String, Option<String>, Option<String>, Option<String>, String) {
if metadata_str.is_empty() {
return (String::new(), None, None, None, String::new());
}
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!("{}/image", contract_address);
(metadata_str.to_string(), metadata_name, metadata_description, metadata_attributes, image_path)
}| serde_json::from_str(&metadata_str) | ||
| .expect("metadata is always json"); | ||
| let metadata_name = metadata.get("name").map(|v| { |
There was a problem hiding this comment.
Handle JSON parsing errors gracefully, sensei!
Using expect for JSON parsing could cause panic in production.
Replace with proper error handling:
- 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(m) => m,
+ Err(e) => {
+ tracing::error!("Failed to parse metadata JSON: {}", e);
+ return None;
+ }
+ };📝 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.
| serde_json::from_str(&metadata_str) | |
| .expect("metadata is always json"); | |
| let metadata_name = metadata.get("name").map(|v| { | |
| let metadata: serde_json::Value = match serde_json::from_str(&metadata_str) { | |
| Ok(m) => m, | |
| Err(e) => { | |
| tracing::error!("Failed to parse metadata JSON: {}", e); | |
| return None; | |
| } | |
| }; | |
| let metadata_name = metadata.get("name").map(|v| { |
Summary by CodeRabbit
Refactor
New Features