fix(torii-grpc): handle case token id for erc20 #3041
Conversation
|
Ohayo sensei! Here’s the breakdown of the changes: WalkthroughThe update refines how token IDs are handled within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Subscriber
Client->>Service: Send balance update (token_id string)
Service->>Service: Split token_id by ":" and check for two parts
alt Valid format
Service->>Service: Convert second part to U256
else Invalid format
Service->>Service: Set token_id to None
end
Service->>Service: Verify existence and membership of token_id
Service->>Subscriber: Process and prepare response (token_id bytes or empty vector)
Subscriber-->>Client: Return SubscribeTokenBalancesResponse
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/subscriptions/token_balance.rs (1)
146-151: Ohayo sensei! Consider enhancing the token ID parsing logic.While the changes handle the ERC20 token ID case well, we could make the code more robust and clearer:
- let id: Vec<&str> = balance.token_id.split(':').collect(); - let token_id: Option<U256> = if id.len() == 2 { - Some(U256::from_be_hex(id[1].trim_start_matches("0x"))) - } else { - None - }; + let token_id: Option<U256> = balance.token_id + .split(':') + .nth(1) + .map(|id| U256::from_be_hex(id.trim_start_matches("0x")));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/subscriptions/token_balance.rs(3 hunks)
🔇 Additional comments (2)
crates/torii/grpc/src/server/subscriptions/token_balance.rs (2)
170-174: LGTM! Clean handling of optional token ID in filter check.The filter check correctly handles the optional token ID case while maintaining the existing filtering logic.
182-182: LGTM! Idiomatic handling of optional token ID in response.The response construction elegantly handles the optional token ID using idiomatic Rust patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: build
- GitHub Check: clippy
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
130-132:❓ Verification inconclusive
Ensure consistent token ID handling across the codebase.
The token ID handling in the
Fromimplementation doesn't align with the new SQL filtering logic. While the SQL query splits the token ID by colon, this conversion processes the entire token ID as a hex string.Please verify if the token ID conversion should also consider the colon separator:
🏁 Script executed:
#!/bin/bash # Description: Search for other token ID processing instances # to ensure consistent handling echo "Searching for token_id processing in the codebase..." rg -A 5 'token_id.*trim_start_matches\("0x"\)'Length of output: 1667
Ohayo, sensei! Heads up on token ID consistency.
It appears that both
crates/torii/grpc/src/server/mod.rsandcrates/torii/grpc/src/server/subscriptions/token.rsuse the same token conversion with.trim_start_matches("0x"), which currently does not account for any colon-separated values like those applied in the SQL filtering. This discrepancy might lead to token balance mismatches. Please verify if the conversion logic should also split the token ID by a colon to ensure consistency with the SQL query.
| conditions | ||
| .push(format!("SUBSTR(token_id, INSTR(token_id, ':') + 1) IN ({})", placeholders)); |
There was a problem hiding this comment.
Ohayo sensei! Consider handling cases where token_id doesn't contain a colon.
The current implementation assumes all token IDs contain a colon, but this might not always be true. If a token_id doesn't contain a colon, INSTR(token_id, ':') will return 0, making SUBSTR start from position 1, which could lead to incorrect matches.
Consider using a CASE statement to handle both formats:
-conditions.push(format!("SUBSTR(token_id, INSTR(token_id, ':') + 1) IN ({})", placeholders));
+conditions.push(format!(
+ "CASE
+ WHEN INSTR(token_id, ':') > 0 THEN SUBSTR(token_id, INSTR(token_id, ':') + 1)
+ ELSE token_id
+ END IN ({})",
+ placeholders
+));📝 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.
| conditions | |
| .push(format!("SUBSTR(token_id, INSTR(token_id, ':') + 1) IN ({})", placeholders)); | |
| conditions | |
| .push(format!( | |
| "CASE | |
| WHEN INSTR(token_id, ':') > 0 THEN SUBSTR(token_id, INSTR(token_id, ':') + 1) | |
| ELSE token_id | |
| END IN ({})", | |
| placeholders | |
| )); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3041 +/- ##
=======================================
Coverage 56.19% 56.20%
=======================================
Files 437 437
Lines 58821 58822 +1
=======================================
+ Hits 33057 33058 +1
Misses 25764 25764 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
121-138: Ohayo sensei! Consider handling potential hex prefix inconsistencies.The token ID parsing logic assumes the hex string will have a "0x" prefix, but this might not always be true. Consider normalizing the hex string before parsing.
- U256::from_be_hex(id[1].trim_start_matches("0x")) + U256::from_be_hex(id[1].trim_start_matches("0x").trim_start_matches("0X"))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(4 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
882-884: Ohayo sensei! This SQL query might fail for token IDs without colons.The current implementation assumes all token IDs contain a colon, but this might not always be true. If a token_id doesn't contain a colon,
SUBSTR(token_id, INSTR(token_id, ':') + 1)will return an empty string, which could lead to incorrect matches.Consider using a CASE statement to handle both formats:
- .push(format!("SUBSTR(token_id, INSTR(token_id, ':') + 1) IN ({})", placeholders)); + .push(format!( + "CASE + WHEN INSTR(token_id, ':') > 0 THEN SUBSTR(token_id, INSTR(token_id, ':') + 1) + ELSE token_id + END IN ({})", + placeholders + ));
Summary by CodeRabbit