feat(torii-grpc): subscribe to token updates (metadata etc.) #2990
Conversation
WalkthroughOhayo, sensei! This PR enhances the gRPC service's token functionality by adding a Changes
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/grpc/src/server/subscriptions/token.rs (4)
34-59: Ohayo sensei! Consider using a globally unique subscription ID generator.Storing subscription IDs generated with a random u64 may risk collisions over a large number of subscriptions. Using a time-based or atomic counter-based ID might reduce the already small probability of collisions.
60-60: Ohayo sensei! Fix Rust formatting to pass CI.The CI pipeline indicates code formatting issues on lines 60, 98, and 135. Please run Rust’s built-in formatter (
cargo fmt) to comply with the project formatting guidelines.Also applies to: 98-98, 135-135
🧰 Tools
🪛 GitHub Actions: ci
[warning] 60-60: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
111-120: Ohayo sensei! Consider graceful shutdown forpublish_updates.When shutting down, the infinite loop in
publish_updatesmay continue receiving from the channel until it empties or indefinitely if no complete signal is sent. You could add a cancellation mechanism or a select statement that also listens for a shutdown request to ensure a clean exit.
122-162: Ohayo sensei! Confirm the performance ofprocess_balance_updateunder high load.Looping over all subscribers in
process_balance_updatemay become costly if there are many subscribers. Consider using a more scalable data structure or partitioning subscriptions by contract address to avoid iterating the entire map.🧰 Tools
🪛 GitHub Actions: ci
[warning] 135-135: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/proto/world.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(3 hunks)crates/torii/grpc/src/server/subscriptions/mod.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/token.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/grpc/src/server/subscriptions/mod.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/subscriptions/token.rs
[warning] 60-60: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
[warning] 98-98: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
[warning] 135-135: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
crates/torii/grpc/src/server/mod.rs
[warning] 56-56: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
[warning] 1256-1256: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
🔇 Additional comments (6)
crates/torii/grpc/src/server/subscriptions/token.rs (1)
91-110: Ohayo sensei! Verify concurrency in theService::newmethod.Spawning a new task to run
publish_updatesmight lead to potential race conditions if other tasks modify shared states unexpectedly. Ensure that the references tosubs_managerremain valid for the duration of the spawned task, and confirm that the manager’s concurrency design is robust (e.g. no risk of partial updates or deadlocks).🧰 Tools
🪛 GitHub Actions: ci
[warning] 98-98: Code formatting issues detected. Please adhere to the Rust formatting guidelines.
crates/torii/grpc/src/server/mod.rs (2)
59-59: Ohayo sensei! Great job consolidating these imports.All relevant
proto::worldentities are neatly grouped. This improves readability and maintainability.
1246-1247: Ohayo sensei! Confirm consistent naming for token subscription streams.The introduction of
SubscribeTokensResponseStreamand aliasing it withSubscribeTokensStreamis appropriate. Just ensure a consistent naming convention throughout the codebase so it’s easy to locate references.Also applies to: 1258-1259
crates/torii/grpc/proto/types.proto (1)
165-165: Ohayo sensei! Validate the usage oftoken_idin upstream logic.Adding this field is helpful to distinguish tokens, but ensure that instrumentation code and any references to
Tokenin logic or tests properly handletoken_idso we don’t accidentally break backward compatibility.crates/torii/grpc/proto/world.proto (2)
43-45: Ohayo sensei: New RPC method for token updates!The addition of the
SubscribeTokensRPC method correctly follows the structure of existing subscription RPCs. This new method enhances the service's token handling capability by providing a stream of token updates. Please ensure that client documentation and integration tests are updated accordingly to include this new branch of functionality.
91-95: Ohayo sensei: Clean and clear SubscribeTokensResponse message!The new
SubscribeTokensResponsemessage neatly encapsulates atypes.Token, aligning well with the new RPC. Do verify that theTokenmessage in the importedtypes.protoincludes thetoken_idfield as per the overarching PR objectives, to guarantee that token updates carry the correct metadata.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/torii/grpc/src/server/subscriptions/token.rs (4)
23-23: Update LOG_TARGET constant valueOhayo sensei! The LOG_TARGET constant appears to be copied from a balance-related module. The value should be updated to reflect the token module.
-pub(crate) const LOG_TARGET: &str = "torii::grpc::server::subscriptions::balance"; +pub(crate) const LOG_TARGET: &str = "torii::grpc::server::subscriptions::token";
63-80: Optimize update_subscriber to reduce lock contentionThe current implementation acquires a read lock followed by a write lock. This can be optimized by using a single write lock.
pub async fn update_subscriber(&self, id: u64, contract_addresses: Vec<Felt>) { - let sender = { - let subscribers = self.subscribers.read().await; - if let Some(subscriber) = subscribers.get(&id) { - subscriber.sender.clone() - } else { - return; // Subscriber not found, exit early - } - }; - - self.subscribers.write().await.insert( - id, - TokenSubscriber { - contract_addresses: contract_addresses.into_iter().collect(), - sender, - }, - ); + let mut subscribers = self.subscribers.write().await; + if let Some(subscriber) = subscribers.get(&id) { + subscribers.insert( + id, + TokenSubscriber { + contract_addresses: contract_addresses.into_iter().collect(), + sender: subscriber.sender.clone(), + }, + ); + } }
48-50: Improve error handling for initial responseThe initial response error is silently ignored. Consider logging the error or handling it appropriately.
- let _ = sender - .send(Ok(SubscribeTokenBalancesResponse { subscription_id, balance: None })) - .await; + if let Err(e) = sender + .send(Ok(SubscribeTokenBalancesResponse { subscription_id, balance: None })) + .await + { + error!(target = LOG_TARGET, error = %e, "Failed to send initial response"); + return Err(Error::Custom("Failed to send initial response".into())); + }
109-113: Improve error handling in publish_updatesConsider implementing a retry mechanism or graceful shutdown for repeated errors.
while let Some(balance) = balance_receiver.recv().await { + let mut consecutive_errors = 0; if let Err(e) = Self::process_balance_update(&subs, &balance).await { error!(target = LOG_TARGET, error = %e, "Processing balance update."); + consecutive_errors += 1; + if consecutive_errors > 3 { + error!(target = LOG_TARGET, "Too many consecutive errors, shutting down."); + break; + } + } else { + consecutive_errors = 0; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/src/server/mod.rs(3 hunks)crates/torii/grpc/src/server/subscriptions/mod.rs(1 hunks)crates/torii/grpc/src/server/subscriptions/token.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/src/server/subscriptions/mod.rs
- crates/torii/grpc/src/server/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
crates/torii/grpc/src/server/subscriptions/token.rs (1)
25-32: LGTM! Well-structured subscriber implementationThe TokenSubscriber struct is well-documented and uses appropriate data structures for efficient contract address lookups.
Larkooo
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2990 +/- ##
==========================================
- Coverage 57.14% 57.06% -0.08%
==========================================
Files 429 430 +1
Lines 56834 56948 +114
==========================================
+ Hits 32477 32498 +21
- Misses 24357 24450 +93 ☔ 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)
800-805: Consider adding error handling for empty contract addresses.The
subscribe_tokensimplementation is clean, but it might benefit from validating the input.Consider adding validation:
async fn subscribe_tokens( &self, contract_addresses: Vec<Felt>, ) -> Result<Receiver<Result<SubscribeTokensResponse, tonic::Status>>, Error> { + if contract_addresses.is_empty() { + return Err(Error::from(QueryError::MissingParam("contract_addresses".into()))); + } self.token_manager.add_subscriber(contract_addresses).await }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/proto/world.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/proto/world.proto
- crates/torii/grpc/proto/types.proto
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (3)
crates/torii/grpc/src/server/mod.rs (3)
37-37: LGTM! Clean integration of TokenManager.The import and struct field additions are well-organized and follow the existing pattern.
Also applies to: 132-132
149-149: LGTM! Proper initialization and service setup.The
TokenManageris correctly initialized and its service is spawned following the same pattern as other managers.Also applies to: 172-172, 184-184
1298-1299: LGTM! Well-structured gRPC implementation.The type alias and trait implementation follow the established patterns in the codebase. The error handling in the gRPC implementation properly maps internal errors to gRPC status codes.
Also applies to: 1310-1310, 1358-1373
There was a problem hiding this comment.
As mentioned to you @Larkooo except the weird hex value returned in some occasion where the hex value contains a 1 like this: 0x0000000000000000000000000000000000000000000000000010000000000069, where 0x69 is expected, works fine!
Let's investigate in subsequent work done on tokens. 👍
Summary by CodeRabbit