refactor(torii-grpc): filter by token id & refactor token protos #3016
Conversation
Larkooo
changed the title
|
Ohayo sensei! WalkthroughThe changes update the data representations for token-related entities. In the gRPC protocol and client/server implementations, several fields in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as gRPC Client
participant S as gRPC Server
participant D as Conversion Utility
participant Sub as Token Subscriber
C->>S: Send Token Update Request
S->>D: Convert token_id (hex to U256), contract_address, metadata to bytes
D-->>S: Return converted data
S->>Sub: Dispatch Token Update
sequenceDiagram
participant C as gRPC Client
participant S as gRPC Server
participant D as Conversion Utility
participant Sub as Balance Subscriber
C->>S: Send Token Balance Update Request
S->>D: Convert balance, token_id, account & contract addresses to bytes
D-->>S: Return converted balance data
S->>Sub: Dispatch Balance Update
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
🧹 Nitpick comments (1)
crates/torii/grpc/proto/types.proto (1)
164-171: Ohayo! The Token message changes look great, sensei!Converting token-related fields from string to bytes is a solid improvement. This change:
- Reduces encoding/decoding overhead
- Provides more efficient storage for binary data
- Better represents blockchain data types
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/src/client.rs(2 hunks)crates/torii/grpc/src/server/mod.rs(2 hunks)crates/torii/grpc/src/server/subscriptions/token.rs(3 hunks)crates/torii/grpc/src/server/subscriptions/token_balance.rs(3 hunks)crates/torii/grpc/src/types/mod.rs(3 hunks)crates/torii/grpc/src/types/schema.rs(1 hunks)crates/torii/sqlite/src/error.rs(1 hunks)crates/torii/sqlite/src/types.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: clippy
🔇 Additional comments (19)
crates/torii/grpc/src/server/mod.rs (6)
16-16: Ohayo sensei! Great addition ofU256.This change nicely streamlines large integer handling and aligns with the new token byte conversion strategy.
95-95: Ohayo sensei! Good catch on adding UTF-8 error handling.This addition enhances robustness by accommodating invalid UTF-8 scenarios.
103-105: Ohayo sensei! Consider validating input hex strings.
U256::from_be_hexmight panic or yield unexpected results if the string is invalid. Handling or logging any parse errors would improve resilience.
113-113: Ohayo sensei! Straightforward conversion to byte array.This snippet cleanly aligns with the new byte-based approach for metadata.
121-123: Ohayo sensei! Consider verifying hex integrity.As with
token_id, ensure the input forbalanceis valid. Explicit error handling would help detect malformed strings.
129-131: Ohayo sensei! Validatetoken_idhex input.Provide a clear error response if
token_idis invalid hex, rather than risking hidden errors.crates/torii/sqlite/src/error.rs (1)
38-39: Ohayo sensei! Great addition for UTF-8 error coverage.Introducing
FromUtf8supports thorough error handling when string conversions fail.crates/torii/grpc/src/server/subscriptions/token.rs (3)
8-8: Ohayo sensei! Good call introducingU256here.This maintains consistency with the large integer handling approach across the codebase.
122-124: Ohayo sensei! Mind double-checking hex validity?
U256::from_be_hexdoes not appear to return aResult, so confirm it never panics on unsupported formats.
137-142: Ohayo sensei! Conversion logic looks consistent.The byte vectors for
token_id,contract_address, andmetadataalign with your revised proto definitions.crates/torii/grpc/src/server/subscriptions/token_balance.rs (2)
135-140: Well-structured token balance processing!Ohayo sensei! The conversion of token balances to byte representation is well implemented with proper error handling and robust hex string parsing.
157-164: Clean and efficient response construction!The response construction using byte vectors is clean and efficient, properly utilizing the
to_bytes_be()andto_vec()methods.crates/torii/grpc/src/types/schema.rs (1)
23-24: Well-structured error handling enhancement!Ohayo sensei! The addition of the
FromUtf8error variant with transparent error propagation is a clean enhancement to the error handling system.crates/torii/grpc/src/types/mod.rs (3)
41-41: Consistent type usage for token_id!Ohayo sensei! The change from
StringtoU256fortoken_idin bothTokenandTokenBalancestructs ensures type consistency across the codebase.Also applies to: 68-68
53-54: Robust conversion implementation!The conversion implementation properly handles:
- Byte representation for token_id and contract_address
- UTF-8 validation for metadata with proper error handling
Also applies to: 58-58
75-78: Clean TokenBalance conversion implementation!The conversion of TokenBalance fields to their byte representations is well-structured and consistent.
crates/torii/grpc/proto/types.proto (1)
173-178: Looking good, sensei! TokenBalance message is properly aligned with Token changes.The conversion to bytes for all blockchain-related fields maintains consistency across the protocol.
crates/torii/grpc/src/client.rs (2)
155-155: Nice choice using U256::ZERO as the default value, sensei!This aligns perfectly with the proto changes and provides a proper numeric type for token IDs.
401-401: Perfect consistency with Token changes, sensei!Using U256::ZERO as the default value for token_id in TokenBalance maintains type safety and consistency.
| contract_address: Felt::from_str(&value.contract_address) | ||
| .unwrap() | ||
| .to_bytes_be() | ||
| .to_vec(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Avoid using unwrap() for production-critical parsing.
Felt::from_str(&value.contract_address).unwrap() will panic on invalid input. A graceful error return would strengthen stability.
| account_address: Felt::from_str(&value.account_address).unwrap().to_bytes_be().to_vec(), | ||
| contract_address: Felt::from_str(&value.contract_address) | ||
| .unwrap() | ||
| .to_bytes_be() | ||
| .to_vec(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Minimize panics by replacing unwrap().
Using map_err(...) aligns with the pattern you've adopted elsewhere. This keeps your service durable under unexpected input.
| @@ -129,6 +129,7 @@ pub struct Event { | |||
| pub struct OptimisticToken { | |||
| pub id: String, | |||
| pub contract_address: String, | |||
| pub token_id: String, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using U256 for token_id instead of String.
Ohayo! The token_id field is defined as String in both OptimisticToken and Token structs, but other files in the codebase use U256 for token identifiers. This inconsistency could lead to unnecessary type conversions and potential bugs.
Apply this diff to maintain type consistency across the codebase:
- pub token_id: String,
+ pub token_id: U256,Also applies to: 144-144
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
107-110:⚠️ Potential issueOhayo sensei! Avoid using
unwrap()for production-critical parsing.
Felt::from_str(&value.contract_address).unwrap()will panic on invalid input. A graceful error return would strengthen stability.- contract_address: Felt::from_str(&value.contract_address) - .unwrap() - .to_bytes_be() - .to_vec(), + contract_address: Felt::from_str(&value.contract_address) + .map_err(|e| Error::ParseStr(e))? + .to_bytes_be() + .to_vec(),
125-129:⚠️ Potential issueOhayo sensei! Minimize panics by replacing
unwrap().Using
map_err(...)aligns with the pattern you've adopted elsewhere. This keeps your service durable under unexpected input.- account_address: Felt::from_str(&value.account_address).unwrap().to_bytes_be().to_vec(), + account_address: Felt::from_str(&value.account_address) + .map_err(|e| Error::ParseStr(e))? + .to_bytes_be() + .to_vec(),
🧹 Nitpick comments (2)
crates/torii/grpc/src/client.rs (1)
159-159: Ohayo sensei! Consider using a constant for default values.Instead of using
U256::ZEROdirectly in multiple places, consider defining a constant likeDEFAULT_TOKEN_IDto improve maintainability.+const DEFAULT_TOKEN_ID: U256 = U256::ZERO; - token_id: U256::ZERO, + token_id: DEFAULT_TOKEN_ID, - token_id: U256::ZERO, + token_id: DEFAULT_TOKEN_ID,Also applies to: 414-414
crates/torii/grpc/src/server/mod.rs (1)
821-839: Ohayo sensei! Consider using a query builder pattern.The current SQL query construction could benefit from a builder pattern to improve readability and maintainability.
struct QueryBuilder { base: String, conditions: Vec<String>, bind_values: Vec<String>, } impl QueryBuilder { fn new(base: &str) -> Self { Self { base: base.to_string(), conditions: Vec::new(), bind_values: Vec::new(), } } fn add_condition_if_not_empty<T>( mut self, values: &[T], column: &str, value_formatter: impl Fn(&T) -> String, ) -> Self { if !values.is_empty() { let placeholders = vec!["?"; values.len()].join(", "); self.conditions.push(format!("{} IN ({})", column, placeholders)); self.bind_values.extend(values.iter().map(value_formatter)); } self } fn build(self) -> (String, Vec<String>) { let mut query = self.base; if !self.conditions.is_empty() { query += &format!(" WHERE {}", self.conditions.join(" AND ")); } (query, self.bind_values) } }Usage:
let (query, bind_values) = QueryBuilder::new("SELECT * FROM tokens") .add_condition_if_not_empty(&contract_addresses, "contract_address", |addr| format!("{:#x}", addr)) .add_condition_if_not_empty(&token_ids, "token_id", |id| u256_to_sql_string(&(*id).into())) .build();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/torii/client/src/client/mod.rs(4 hunks)crates/torii/grpc/proto/world.proto(4 hunks)crates/torii/grpc/src/client.rs(12 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/server/subscriptions/token.rs(6 hunks)crates/torii/grpc/src/server/subscriptions/token_balance.rs(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
🔇 Additional comments (22)
crates/torii/grpc/src/server/subscriptions/token_balance.rs (7)
8-8: Ohayo sensei! The new import ofU256is correct and aligns with the usage of big-integer operations in this file.
34-36: Ohayo sensei! Adding thetoken_idsfield is a good approach to let subscribers filter by token.
51-51: Ohayo sensei! The addition oftoken_idsinadd_subscriberand collecting them into aHashSetensures efficient membership checks.Also applies to: 66-66
79-79: Ohayo sensei! Updating subscriber logic to includetoken_idshere is consistent with theadd_subscriberchanges, nice job!Also applies to: 95-95
164-168: Ohayo sensei! The check for an emptytoken_idsfilter is well-handled.
172-175: Ohayo sensei! Converting addresses and balances into byte arrays is consistent and helps ensure a standard binary representation.
137-148:❓ Verification inconclusive
Ohayo sensei! Great usage of
U256::from_be_hexfor safely parsing big integers from hex strings. Consider verifying all upstream usages oftoken_idto ensure they always contain valid hex.Here's a helpful script to check if any partial or invalid hex strings might be passed:
🏁 Script executed:
#!/bin/bash rg -A 10 'token_id\s*='Length of output: 28587
Ohayo sensei! Great work using
U256::from_be_hexto safely parse hex strings for big integers. I ran a repo-wide search and noticed several patterns fortoken_id: while the GRPC subscriptions (like incrates/torii/grpc/src/server/subscriptions/token_balance.rs) correctly trim the "0x" prefix before parsing, other modules (such as some GraphQL handlers) split thetoken_idby':'and expect a two-part format. This is perfectly fine as long as the upstream producers oftoken_idare consistent with the expected format.
- Action Required:
- Please verify that every upstream usage of
token_idprovides a valid hex string (or the expected format) before parsing withU256::from_be_hex.- Confirm that the differing approaches (hex parsing vs. split-and-assert) are intentional and well-documented for their respective data sources.
crates/torii/grpc/src/server/subscriptions/token.rs (8)
8-8: Ohayo sensei! The new import forU256is nicely integrated for handling big-integer tokens.
31-33: Ohayo sensei! Introducingtoken_idsinTokenSubscriberis consistent with the rest of the subscription logic.
47-47: Ohayo sensei! Good job adding and collectingtoken_idsinadd_subscriberfor better filtering.Also applies to: 59-59
67-72: Ohayo sensei! Great extension toupdate_subscriberto handletoken_ids. This ensures dynamic subscription updates remain flexible.
85-86: Ohayo sensei! Assigning updatedtoken_idsto the subscriber again is consistent and ensures correctness.
133-135: Ohayo sensei! UsingFelt::from_strandU256::from_be_hexfor parsing is a neat approach. Watch for invalid strings that could cause parse errors.
145-148: Ohayo sensei! Skipping unknown token IDs keeps the subscription targeted. This is well-structured.
153-154: Ohayo sensei! Converting these fields to big-endian byte arrays is consistent with the rest of the system. This ensures a uniform representation for tokens.Also applies to: 158-158
crates/torii/client/src/client/mod.rs (6)
5-5: Ohayo sensei! Bringing inU256is consistent with the big-integer usage across the codebase.
112-112: Ohayo sensei! Great addition of thetoken_idsparameter in thetokens()method. That helps narrow the retrieved tokens.Also applies to: 115-116, 118-121
128-129: Ohayo sensei! Extendingtoken_balancesto accepttoken_idsis well-coordinated, ensuring more targeted balance queries.Also applies to: 131-135
247-248: Ohayo sensei! Subscribing to token balance updates withtoken_idsis a nice improvement. This change aligns with the server-side updates.Also applies to: 250-253
256-256: Ohayo sensei! Passingtoken_idstoupdate_token_balance_subscriptionensures consistent subscription updates.Also applies to: 262-262
275-299: Ohayo sensei! These new methodson_token_updatedandupdate_token_subscriptionprovide robust filtering for token updates, reflecting the subscription improvements across the codebase.crates/torii/grpc/proto/world.proto (1)
84-85: Ohayo sensei! The protocol changes look good!The addition of
token_idsfields asbytestype is consistent across all token-related messages, providing a clean interface for token ID filtering.Also applies to: 100-101, 123-124, 133-134
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 (3)
crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/server/subscriptions/token.rs(6 hunks)crates/torii/grpc/src/server/subscriptions/token_balance.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/src/server/subscriptions/token.rs
- crates/torii/grpc/src/server/mod.rs
🔇 Additional comments (4)
crates/torii/grpc/src/server/subscriptions/token_balance.rs (4)
8-8: Ohayo! Nice addition of token ID filtering support!The new
token_idsfield follows the same clean pattern as the existing address filters, with clear documentation explaining the empty set behavior.Also applies to: 34-36
47-52: Method signatures updated consistently, sensei!The addition of
token_idsparameter follows the same pattern as other filters, maintaining consistency in the API design.Also applies to: 74-80
164-167: Token ID filtering looks great, sensei!The implementation follows the same pattern as address filtering, with proper empty set handling for opt-out behavior.
172-175: Byte vector conversion looks good!The conversion to byte vectors using
to_be_bytes()andto_bytes_be()is implemented correctly.
| let contract_address = | ||
| Felt::from_str(&balance.contract_address).map_err(ParseError::FromStr)?; | ||
| let account_address = | ||
| Felt::from_str(&balance.account_address).map_err(ParseError::FromStr)?; | ||
| let token_id = U256::from_be_hex(balance.token_id.trim_start_matches("0x")); | ||
| let balance = U256::from_be_hex(balance.balance.trim_start_matches("0x")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Consider adding error handling for hex parsing, sensei!
While moving the parsing outside the loop is great for performance, the from_be_hex calls for token_id and balance don't handle invalid hex strings. Consider adding proper error handling:
- let token_id = U256::from_be_hex(balance.token_id.trim_start_matches("0x"));
- let balance = U256::from_be_hex(balance.balance.trim_start_matches("0x"));
+ let token_id = U256::from_be_hex(balance.token_id.trim_start_matches("0x"))
+ .map_err(|_| ParseError::InvalidHex(balance.token_id.clone()))?;
+ let balance = U256::from_be_hex(balance.balance.trim_start_matches("0x"))
+ .map_err(|_| ParseError::InvalidHex(balance.balance.clone()))?;You'll need to add a new variant to ParseError:
pub enum ParseError {
FromStr(String),
InvalidHex(String),
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
- Coverage 56.21% 56.16% -0.05%
==========================================
Files 436 436
Lines 58737 58904 +167
==========================================
+ Hits 33019 33084 +65
- Misses 25718 25820 +102 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes