refactor(torii-grpc): event subscription with multiple clauses #2555
Conversation
WalkthroughOhayo, sensei! This pull request introduces changes to the Changes
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: 2
🧹 Outside diff range and nitpick comments (2)
crates/torii/grpc/proto/world.proto (1)
111-111: Ohayo, sensei! This change looks great!The modification to
SubscribeEventsRequestto userepeated types.EntityKeysClauseinstead of a singletypes.KeysClauseis a solid improvement. It allows for more flexible and efficient event subscriptions by supporting multiple entity keys in a single request.This change aligns well with the PR objectives and is consistent with other parts of the file, such as the
SubscribeEntitiesRequestmessage.Consider updating the service documentation for the
SubscribeEventsRPC to reflect this change in capability. For example:// Subscribe to events for multiple entity keys rpc SubscribeEvents (SubscribeEventsRequest) returns (stream SubscribeEventsResponse);This will help users of the service understand the new functionality at a glance.
crates/torii/grpc/src/server/subscriptions/mod.rs (1)
74-74: Correct typos in comments: 'athis' should be 'at this'There's a typo in the comments on lines 74 and 120:
'athis index'should be'at this index'.- // athis index + // at this indexAlso applies to: 120-120
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- crates/torii/grpc/proto/world.proto (1 hunks)
- crates/torii/grpc/src/server/mod.rs (1 hunks)
- crates/torii/grpc/src/server/subscriptions/entity.rs (2 hunks)
- crates/torii/grpc/src/server/subscriptions/event.rs (3 hunks)
- crates/torii/grpc/src/server/subscriptions/event_message.rs (2 hunks)
- crates/torii/grpc/src/server/subscriptions/mod.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
crates/torii/grpc/src/server/mod.rs (1)
837-841: LGTM! The changes enhance event subscription capabilities.Ohayo, sensei! The modifications to the
subscribe_eventsmethod look good. The new implementation now supports multiple entity keys, which should provide more flexibility in event subscriptions.To ensure consistency across the codebase, let's verify the usage of this updated method:
✅ Verification successful
Ohayo, sensei! It looks like the previous shell scripts had some hiccups. Let's try running corrected scripts to ensure everything is in order:
Ohayo, sensei! 🎉 The verification is complete, and there are no remaining usages of the old
subscribe_eventsmethod signature withKeysClause. All instances have been successfully updated to useVec<proto::types::EntityKeysClause>.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the old method signature and verify the new usage. # Test 1: Check for any remaining usage of the old method signature echo "Checking for old method signature usage:" rg --type rust 'subscribe_events.*KeysClause' # Test 2: Verify the new method usage echo "Verifying new method usage:" rg --type rust 'subscribe_events.*Vec<proto::types::EntityKeysClause>'Length of output: 276
Script:
#!/bin/bash # Description: Comprehensive search for any usages of `subscribe_events` to ensure no old method signatures remain. echo "Searching for all occurrences of `subscribe_events` in the codebase:" rg --type rust 'subscribe_events' echo "Searching specifically for any `subscribe_events` calls with `KeysClause` parameters:" rg --type rust 'subscribe_events\s*\(.*KeysClause.*\)'Length of output: 1154
Script:
#!/bin/bash # Description: Comprehensive search for any usages of `subscribe_events` to ensure no old method signatures remain. echo 'Searching for all occurrences of subscribe_events in the codebase:' rg --type rust 'subscribe_events' echo 'Searching specifically for any subscribe_events calls with KeysClause parameters:' rg --type rust 'subscribe_events\s*\(.*KeysClause.*\)'Length of output: 938
crates/torii/grpc/src/server/subscriptions/event.rs (5)
22-22: Ohayo sensei! Importingmatch_keysfunctionThe addition of the
match_keysimport ensures that the function is available for use in event processing. This is essential for the new key matching logic.
25-25: Ohayo sensei! ImportingEntityKeysClauseAdding
EntityKeysClausefromcrate::typesenables handling multiple entity key clauses in subscriptions. This expands the flexibility of event subscriptions.
32-32: Ohayo sensei! Good job updatingkeystoVec<EntityKeysClause>By changing
keysto a vector ofEntityKeysClause, you allow subscribers to handle multiple key clauses, enhancing the flexibility of the event subscription system.
45-45: Ohayo sensei! Great update toadd_subscribermethod signatureModifying
add_subscriberto accept aVec<EntityKeysClause>aligns with the updatedEventSubscriberstructure and allows subscribers to subscribe to multiple key clauses.
112-112: Ohayo sensei! Nice refactoring withmatch_keysfunctionUsing
match_keysto encapsulate key matching logic simplifiesprocess_eventand improves code readability. This abstraction enhances maintainability.crates/torii/grpc/src/server/subscriptions/event_message.rs (3)
21-21: Correctly importedmatch_entity_keysfunctionOhayo, sensei! The import statement properly includes the
match_entity_keysfunction, facilitating its use in this module.
24-24: Updated import forEntityKeysClauseOhayo, sensei! The import statement for
EntityKeysClauseis correctly adjusted, reflecting changes in the codebase.
124-124: Simplified conditional logic withmatch_entity_keysfunctionOhayo, sensei! Utilizing
match_entity_keysto handle the matching logic streamlines the code, enhancing clarity and maintainability.crates/torii/grpc/src/server/subscriptions/entity.rs (3)
22-22: Ohayo, sensei! The import ofmatch_entity_keysenhances modularity.Adding
use super::match_entity_keys;correctly imports the function, promoting code reusability and cleaner code structure.
25-25: Ohayo, sensei! The addition ofEntityKeysClauseimport is appropriate.Importing
EntityKeysClauseensures proper handling of subscription clauses based on entity keys.
132-132: Ohayo, sensei! Excellent refactoring usingmatch_entity_keys.Replacing the complex conditional logic with the
match_entity_keysfunction simplifies the code and enhances readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- crates/torii/client/src/client/mod.rs (2 hunks)
- crates/torii/grpc/src/client.rs (2 hunks)
- crates/torii/grpc/src/server/mod.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
crates/torii/grpc/src/client.rs (1)
207-209: Ohayo, sensei! Great improvement to thesubscribe_eventsmethod!The change from
Option<KeysClause>toVec<EntityKeysClause>enhances the flexibility of event subscriptions. This modification allows for multiple entity key clauses, which is a significant improvement in functionality.The implementation correctly handles the new input type, mapping it to the expected format for the gRPC request. The rest of the method remains consistent with the existing error handling and stream processing logic, which is excellent.
crates/torii/client/src/client/mod.rs (2)
16-16: Ohayo, sensei! Updated import includesEntityKeysClause.Including
EntityKeysClausein the import statement aligns with its usage in the updated method signatures.
162-162: Update inon_starknet_eventmethod signature requires attention.The method now accepts
keys: Vec<EntityKeysClause>instead ofkeys: Option<KeysClause>. Please ensure all calls toon_starknet_eventare updated accordingly to prevent any mismatches.To verify all usages are updated, you can run:
✅ Verification successful
Ohayo, sensei! To ensure all calls to
on_starknet_eventare correctly updated, please run the following script:
Ohayo, sensei! All calls to
on_starknet_eventhave been correctly updated. No mismatches found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `on_starknet_event` and check their arguments. # Test: Search for the method usage. Expect: Calls passing `Vec<EntityKeysClause>`. rg --type rust -A 2 'on_starknet_event\('Length of output: 245
Script:
#!/bin/bash # Description: Verify all calls to `on_starknet_event` pass `Vec<EntityKeysClause>`. # Search for all call sites and display the line with the call and the line(s) with arguments. rg --type rust 'on_starknet_event\(' -A 2Length of output: 245
crates/torii/grpc/src/server/mod.rs (2)
1193-1193: Confirm correct handling insubscribe_eventsrequest handlerThe request handler correctly extracts the
keysfrom theSubscribeEventsRequestand passes them to the updatedsubscribe_eventsmethod.
837-841: Verify that all usages ofsubscribe_eventsare updatedOhayo, sensei! Since the signature of
subscribe_eventshas changed, please ensure that all calls to this method across the codebase have been updated to match the new signature accepting a vector ofEntityKeysClause.Run the following script to locate all usages of
subscribe_events:✅ Verification successful
Further verification required for
subscribe_eventsusageOhayo, sensei! The initial search only identified the definitions of
subscribe_events. To ensure that all calls to this method across the codebase have been updated to use the new signature withVec<proto::types::EntityKeysClause>, please run the following script:
Verification successful! All usages of
subscribe_eventshave been updated appropriately.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `subscribe_events` in the Rust codebase. # Expected: All calls should use the updated signature with `Vec<proto::types::EntityKeysClause>`. rg --type rust --word-regexp 'subscribe_events\(' -A 3Length of output: 976
Script:
#!/bin/bash # Description: Find all calls to `subscribe_events` in the Rust codebase, excluding function definitions. rg --type rust --word-regexp 'subscribe_events\(' -A 3 | grep -v 'fn subscribe_events'Length of output: 807
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
=======================================
Coverage 69.59% 69.59%
=======================================
Files 388 389 +1
Lines 50018 50015 -3
=======================================
Hits 34809 34809
+ Misses 15209 15206 -3 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes