Gateway unsubscribe for multiple subscribed clients #1637
Conversation
|
Warning Rate Limit Exceeded@zkokelj has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 13 minutes and 21 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per repository. WalkthroughThe changes primarily focus on enhancing the subscription and unsubscription functionalities in the codebase. The modifications include the introduction of new constants, changes in function signatures, removal of unused functions, and addition of new test functions. The Changes
TipsChat with CodeRabbit Bot (
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... Files selected for processing (5)
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/rpc/client.go (1 hunks)
- go/rpc/encrypted_client.go (2 hunks)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
- tools/walletextension/accountmanager/account_manager.go (2 hunks)
- tools/walletextension/subscriptions/subscriptions.go (4 hunks)
Additional comments: 11
go/rpc/client.go (1)
- 34-40: The addition of the "Unsubscribe" constant is consistent with the introduction of an unsubscribe functionality. Ensure that this constant is used correctly in the codebase where the "eth_unsubscribe" method is needed.
go/rpc/encrypted_client.go (2)
125-130: Ensure that the removal of
setResultToSubIDfunction and its invocation doesn't affect the functionality of theSubscribemethod. Verify that the subscription ID is being handled correctly in the new implementation.205-210: Ensure that the removal of
setResultToSubIDfunction and its invocation doesn't affect the functionality of theexecuteSensitiveCallmethod. Verify that the subscription ID is being handled correctly in the new implementation.tools/walletextension/accountmanager/account_manager.go (2)
- 61-68: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-74]
The
ProxyRequestfunction now handles subscribing to events. It uses thesuggestSubscriptionClientfunction to determine the clients to subscribe to and then callsHandleNewSubscriptionson thesubscriptionsManager. Error handling and logging are also included.
- 78-88: The
ProxyRequestfunction now also handles unsubscribing from events. It validates the parameters, ensuring that a subscription ID is provided, and then callsHandleUnsubscribeon thesubscriptionsManager.+ if rpcReq.Method == rpc.Unsubscribe { + if len(rpcReq.Params) != 1 { + return fmt.Errorf("one parameter (subscriptionID) expected, %d parameters received", len(rpcReq.Params)) + } + subscriptionID, ok := rpcReq.Params[0].(string) + if !ok { + return fmt.Errorf("subscriptionID needs to be a string. Got: %d", rpcReq.Params[0]) + } + m.subscriptionsManager.HandleUnsubscribe(subscriptionID, rpcResp) + return nil + }tools/walletextension/subscriptions/subscriptions.go (4)
22-31: The
SubscriptionManagerstruct has been updated to handle*gethrpc.ClientSubscriptioninstead of string IDs. This change is reflected in thesubscriptionMappingsmap and the constructor functionNew().55-62: The
HandleNewSubscriptionsfunction now updates the subscription mapping with a*gethrpc.ClientSubscriptioninstead of a string. This change is reflected in the call toUpdateSubscriptionMapping().114-139: The
UpdateSubscriptionMappingfunction has been modified to handle*gethrpc.ClientSubscriptioninstead of string IDs. It checks if the subscription already exists in the slice before adding it, which prevents duplicate subscriptions.159-174: A new
HandleUnsubscribefunction has been added to handle unsubscribing from multiple subscriptions associated with a user subscription ID. It locks theSubscriptionManagerstruct with a mutex to prevent data races, unsubscribes from all subscriptions associated with the user subscription ID, and then deletes the user subscription ID from the subscription mappings.integration/obscurogateway/obscurogateway_test.go (2)
93-97: The addition of the new test functions
testUnsubscribeandtestClosingConnectionWhileSubscribedto the test suite is a good practice to ensure the new functionality works as expected.599-604: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [588-617]
The
subscribeToEventsfunction now returns anethereum.Subscriptionobject, which is used in the new test functions to unsubscribe. This is a necessary change to support the new unsubscribe functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- go/rpc/client.go (1 hunks)
- go/rpc/encrypted_client.go (2 hunks)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
- tools/walletextension/accountmanager/account_manager.go (2 hunks)
- tools/walletextension/subscriptions/subscriptions.go (4 hunks)
Files skipped from review due to trivial changes (1)
- go/rpc/client.go
Additional comments: 13
go/rpc/encrypted_client.go (2)
125-130: The function
Subscribenow returns an error if the subscription type is notSubscriptionTypeLogs. This change might affect other parts of the codebase that use this function with different subscription types. Ensure that this change doesn't break any existing functionality.205-210: The
executeSensitiveCallfunction now encrypts the arguments before making the RPC call. This is a good security practice as it prevents sensitive data from being exposed in transit. However, ensure that the server side is able to decrypt and correctly interpret these encrypted arguments.tools/walletextension/accountmanager/account_manager.go (3)
- 61-68: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-74]
The
ProxyRequestfunction now handles subscription requests. It first identifies the appropriate clients for the subscription usingsuggestSubscriptionClient. Then, it callsHandleNewSubscriptionsto handle the subscription for the identified clients. If an error occurs, it logs the error and returns it. This change is significant as it allows handling multiple accounts for a single user request.
78-88: The
ProxyRequestfunction now also handles unsubscription requests. It validates the parameters for the unsubscribe method and calls theHandleUnsubscribefunction of thesubscriptionsManager. This change is significant as it allows handling unsubscription requests.75-91: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-88]
Ensure that the
ProxyRequestfunction is being called with the correct parameters throughout the codebase. Also, verify that the error handling forsuggestSubscriptionClientandHandleNewSubscriptionsis done correctly.tools/walletextension/subscriptions/subscriptions.go (4)
22-31: The
SubscriptionManagerstruct has been updated to store slices of*gethrpc.ClientSubscriptioninstead of strings in thesubscriptionMappingsmap. This change should be verified across the codebase to ensure compatibility.55-62: The
HandleNewSubscriptionsfunction now takes an additional parameter&sm.muand callscheckIfUserConnIsClosedAndUnsubscribewith an additional parameter. This change should be verified across the codebase to ensure compatibility.114-139: The
UpdateSubscriptionMappingfunction now takes a*gethrpc.ClientSubscriptioninstead of a string for the subscription. This change should be verified across the codebase to ensure compatibility.159-174: A new
HandleUnsubscribefunction has been added to handle unsubscribing from subscriptions. This function should be tested thoroughly to ensure it works as expected.integration/obscurogateway/obscurogateway_test.go (4)
93-97: The new test functions
testUnsubscribeandtestClosingConnectionWhileSubscribedhave been added to the test suite. Ensure that these tests are passing and that they cover all the edge cases related to the new unsubscribe functionality.388-436: The
testUnsubscribefunction tests the unsubscribe functionality. It creates a user, registers accounts, deploys a contract, subscribes to an event, triggers an event, unsubscribes, and then triggers another event to verify that no new events are received after unsubscribing. The logic seems correct.438-494: The
testClosingConnectionWhileSubscribedfunction tests the behavior of the system when the websocket connection is closed while subscribed to events. It creates a user, registers accounts, deploys a contract, subscribes to an event, closes the websocket connection, triggers an event, re-establishes the connection, triggers another event, and then unsubscribes. The logic seems correct.599-604: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [588-617]
The
subscribeToEventsfunction now returns anethereum.Subscription. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/rpc/encrypted_client.go (4 hunks)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
- tools/walletextension/subscriptions/subscriptions.go (4 hunks)
Additional comments: 14
go/rpc/encrypted_client.go (4)
89-95: The
Subscribemethod has been updated to remove theresultparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.125-130: The
Subscribemethod now returns a*rpc.ClientSubscriptionand anerror. Ensure that all calls to this function throughout the codebase have been updated to handle the new return types.205-210: The
executeSensitiveCallfunction has been updated. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.334-339: The
IsSensitiveMethodfunction has been updated. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.integration/obscurogateway/obscurogateway_test.go (4)
93-97: The new test functions
testUnsubscribeandtestClosingConnectionWhileSubscribedhave been added to the test suite. Ensure that these tests are passing and that they cover all the edge cases related to the unsubscribe functionality and closing the connection while subscribed.388-436: The
testUnsubscribefunction tests the unsubscribe functionality. It creates a user, registers accounts, deploys a contract, subscribes to events, triggers events, unsubscribes, and checks that no more events are received after unsubscribing. The logic seems correct.438-494: The
testClosingConnectionWhileSubscribedfunction tests the behavior when the websocket connection is closed while subscribed. It creates a user, registers accounts, deploys a contract, subscribes to events, closes the connection, triggers events, checks that no events are received, re-establishes the connection, triggers events again, checks that no events are received (since closing the connection unsubscribes), and finally calls unsubscribe. The logic seems correct.599-604: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [588-617]
The
subscribeToEventsfunction now returns anethereum.Subscriptionobject. This change is consistent with the new unsubscribe functionality. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.tools/walletextension/subscriptions/subscriptions.go (6)
21-32: The
SubscriptionManagerstruct has been updated to store a map of*gethrpc.ClientSubscriptionslices instead of strings. This change allows for multiple subscriptions per userSubscriptionID, which is a significant change in the subscription handling logic.55-65: The
HandleNewSubscriptionsmethod now uses a new methodcheckIfUserConnIsClosedAndUnsubscribeto handle subscription termination. This change allows for more efficient handling of closed connections and subscription termination.103-132: The
unsubscribeAndRemovemethod has been added to handle unsubscribing and removing subscriptions from thesubscriptionMappings. This method is a key part of the new subscription handling logic and is used in bothcheckIfUserConnIsClosedAndUnsubscribeandHandleUnsubscribe.134-140: The
checkIfUserConnIsClosedAndUnsubscribemethod checks if the user connection is closed and if so, unsubscribes and removes the subscription. This method is used inHandleNewSubscriptionsto handle subscription termination when the user connection is closed.142-167: The
UpdateSubscriptionMappingmethod now takes a*gethrpc.ClientSubscriptioninstead of a string. This change aligns with the new subscription handling logic where multiple subscriptions can be associated with a single userSubscriptionID.187-202: A new method
HandleUnsubscribehas been added to handle unsubscribing and removing subscriptions. This method is a key part of the new subscription handling logic and allows for unsubscribing from all subscriptions associated with a specific userSubscriptionID.
| subscription.Unsubscribe() | ||
|
|
||
| subscriptions, exists := sm.subscriptionMappings[userSubscriptionID] | ||
| if !exists { | ||
| return | ||
| } |
There was a problem hiding this comment.
Is this possible to happen ? that a subscription is cancelled but there isn't one in our mapping ?
There was a problem hiding this comment.
Should not happen, added it here to be 100% sure not to crash. I added a log that will clearly tell us if & what happened.
Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/accountmanager/account_manager.go (2 hunks)
Additional comments: 2
tools/walletextension/accountmanager/account_manager.go (2)
- 61-68: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-74]
The
ProxyRequestfunction now handlesrpc.Subscriberequests. It uses thesuggestSubscriptionClientfunction to determine the clients to subscribe to, and then callsHandleNewSubscriptionson thesubscriptionsManager. Error handling and logging are also included.
- 78-88: The
ProxyRequestfunction now also handlesrpc.Unsubscriberequests. It validates the parameters and then callsHandleUnsubscribeon thesubscriptionsManager.+ if rpcReq.Method == rpc.Unsubscribe { + if len(rpcReq.Params) != 1 { + return fmt.Errorf("one parameter (subscriptionID) expected, %d parameters received", len(rpcReq.Params)) + } + subscriptionID, ok := rpcReq.Params[0].(string) + if !ok { + return fmt.Errorf("subscriptionID needs to be a string. Got: %v", rpcReq.Params[0]) + } + m.subscriptionsManager.HandleUnsubscribe(subscriptionID, rpcResp) + return nil + }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/subscriptions/subscriptions.go (4 hunks)
Additional comments: 7
tools/walletextension/subscriptions/subscriptions.go (7)
19-32: The
SubscriptionManagerstruct has been updated to store a map ofgethrpc.ClientSubscriptionpointers instead of strings. This change allows for more direct interaction with the subscriptions.55-63: The
HandleNewSubscriptionsfunction now includes a goroutine that checks if the user connection is closed and unsubscribes if it is. This is a good practice to prevent unnecessary resource usage.103-132: The
unsubscribeAndRemovefunction is a new addition that unsubscribes a given subscription and removes it from thesubscriptionMappingsmap. This function is used to clean up subscriptions when they are no longer needed.134-140: The
checkIfUserConnIsClosedAndUnsubscribefunction is another new addition that continuously checks if a user connection is closed and unsubscribes the associated subscription if it is. This function is used to ensure that subscriptions are cleaned up when a user connection is closed.142-167: The
UpdateSubscriptionMappingfunction has been updated to handle the newgethrpc.ClientSubscriptionpointers. It now checks if a subscription already exists in the map before adding it, which is a good practice to prevent duplicate entries.187-201: The
HandleUnsubscribefunction is a new addition that handles unsubscribing from all subscriptions associated with a given user subscription ID. This function is used to clean up all subscriptions when a user wants to unsubscribe.107-112: In response to the previous comment, it is possible for a subscription to be cancelled but not exist in the mapping. This could occur if the subscription was already removed from the mapping by another process. The current code handles this case correctly by checking if the subscription exists in the mapping before trying to remove it.
Why this change is needed
Since we (can) have multiple accounts/addresses registered per user and Obscuro Gateway subscribes for all of them. We return new subscriptionID. And in case users wants to unsubscribe we want to make sure the gateway also unsubscribes from all subscriptions to the node that are/were related to that specific subscriptionID.
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks