Ten gateway use http instead of ws #1743
Conversation
WalkthroughThe recent updates involve enhancing testing capabilities, refining the account management system, and differentiating between HTTP and WebSocket protocols across the application. A new test function was added to verify subscription topics, and existing functions were updated to handle the separation of HTTP and WebSocket addresses. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- integration/obscurogateway/tengateway_test.go (3 hunks)
- tools/walletextension/accountmanager/account_manager.go (6 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/container/walletextension_container.go (4 hunks)
- tools/walletextension/useraccountmanager/user_account_manager.go (3 hunks)
- tools/walletextension/useraccountmanager/user_account_manager_test.go (2 hunks)
- tools/walletextension/wallet_extension.go (4 hunks)
Additional comments: 22
tools/walletextension/common/constants.go (1)
- 40-40: The addition of the
HTTPProtocolconstant is consistent with the existing constants and follows the correct syntax.tools/walletextension/useraccountmanager/user_account_manager_test.go (2)
12-12: The addition of the
"http://test"argument in theNewUserAccountManagerfunction call aligns with the updated function signature to accommodate HTTP and WebSocket addresses.53-53: The addition of the extra empty string arguments in the
NewUserAccountManagerfunction call is consistent with the updated function signature.tools/walletextension/useraccountmanager/user_account_manager.go (4)
16-22: The
UserAccountManagerstruct has been updated to include separate fields for HTTP and WebSocket RPC addresses, which aligns with the architectural changes to support both protocols.27-33: The
NewUserAccountManagerfunction now correctly takes two additional parameters for the HTTP and WebSocket addresses, consistent with the updated struct.46-46: The
NewAccountManagerfunction within theUserAccountManagerstruct has been updated to use the WebSocket RPC address, which is appropriate for the context of subscriptions that still use WebSocket connections.99-99: The creation of a new client using the WebSocket address within the
GetUserAccountManagermethod is consistent with the need to maintain WebSocket connections for certain operations.tools/walletextension/container/walletextension_container.go (4)
41-43: The differentiation between WebSocket and HTTP protocols for the host RPC bind addresses is correctly implemented, aligning with the architectural changes.
55-55: The
userAccountManagerinitialization now correctly uses both the HTTP and WebSocket addresses, which is consistent with the updated requirements.88-88: The creation of a new client using the WebSocket address for the default user account manager is appropriate and aligns with the changes made to support both protocols.
113-120: The instantiation of the
WalletExtensionwith separate fields for HTTP and WebSocket addresses is correctly implemented and necessary for the new communication protocol structure.tools/walletextension/accountmanager/account_manager.go (4)
37-46: The
AccountManagerstruct has been updated to include a map for HTTP clients and a WebSocket address, which is consistent with the need to support both HTTP and WebSocket connections.50-57: The
NewAccountManagerfunction has been correctly updated to initialize the new fields in theAccountManagerstruct.78-78: The
AddClientfunction now adds clients to the HTTP clients map, which is consistent with the updated struct and the architectural changes.114-150: The new functions
suggestSubscriptionClient,filterClients, andcreateClientsForAllAccountsare correctly implemented to handle subscriptions using WebSocket clients, which is necessary due to the retention of WebSocket connections for subscription purposes.tools/walletextension/wallet_extension.go (4)
34-35: The
WalletExtensionstruct now includes separate fields for HTTP and WebSocket addresses, which is necessary for the new communication protocol structure.47-64: The
Newfunction of theWalletExtensionhas been updated to accept separate HTTP and WebSocket addresses and correctly initializes the new fields in the struct.171-171: The creation of a new encrypted network client using the HTTP address within the
SubmitViewingKeymethod is consistent with the updated struct and the need to support HTTP connections.243-251: The
AddAddressToUsermethod correctly creates a new encrypted client using the HTTP address, which aligns with the changes made to support both protocols.integration/obscurogateway/tengateway_test.go (3)
- 669-669: The
transferETHToAddressfunction is used to transfer ETH to a specified address within the tests. Ensure that this function is robust and handles all potential error cases, as transactions are a critical part of the test setup.Verification successful
The output from the
rgcommand shows that thetransferETHToAddressfunction is called multiple times within thetengateway_test.gofile. After each call, there is arequire.NoError(t, err)statement. This indicates that the test is using therequirepackage from thetestifylibrary to assert that no error was returned by thetransferETHToAddressfunction. If an error is returned, therequire.NoErrorfunction will fail the test and stop execution.This is a common and appropriate way to handle errors in test functions in Go, as it ensures that the test will not proceed if a critical setup step, like transferring ETH, fails.
Based on this information, it can be concluded that the
transferETHToAddressfunction is robust in terms of error handling, and the calls to this function within the test suite are correctly handling the potential errors by asserting that no error should occur.* 669-669: The `createTenNetwork` function is responsible for setting up a test network. It's important to ensure that this function is reliable and that the network setup is consistent with the requirements of the tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the transferETHToAddress function is used consistently and correctly throughout the test suite. rg --type go 'transferETHToAddress'Length of output: 1196
Script:
#!/bin/bash # Examine the definition of transferETHToAddress to check for error handling ast-grep --lang go --pattern 'func transferETHToAddress($_, $_, $_, $_) ($_,$_){ $$$ }' integration/obscurogateway/tengateway_test.goLength of output: 1757
Script:
#!/bin/bash # Verify that the calls to transferETHToAddress handle the error that might be returned. rg --type go 'transferETHToAddress' integration/obscurogateway/tengateway_test.go | rg 'err :='Length of output: 95
Script:
#!/bin/bash # Search for the calls to transferETHToAddress and get the surrounding lines of code to inspect error handling. rg --type go -C 3 'transferETHToAddress' integration/obscurogateway/tengateway_test.goLength of output: 1736
Verification successful
The output from the shell script indicates that the
createTenNetworkfunction is defined and called within theintegration/obscurogateway/tengateway_test.gofile. The function is invoked with two parameters:t, which is likely a reference to the testing framework's*testing.Tobject, andstartPort, which is an integer that probably specifies the starting port number for the test network.Given this information, it appears that the
createTenNetworkfunction is indeed used within the test suite, and its usage is consistent with the typical pattern of setting up a network for testing purposes. However, without further context on the implementation details of thecreateTenNetworkfunction and how it interacts with the rest of the test suite, we cannot fully verify the correctness of its usage.To gain more insight into the implementation of the
createTenNetworkfunction and to verify its correctness, we would need to review the function's definition and its calls in more detail.
The output from the shell script provides a snippet of the
createTenNetworkfunction definition in theintegration/obscurogateway/tengateway_test.gofile. The function takes two parameters: a pointer to the testing framework's*testing.Tobject and an integerstartPort. The comments and code within the function suggest that it is responsible for creating a simulated Ten network for testing purposes, with the following steps:
- It specifies the number of nodes in the network (
numberOfNodes := 1).- It creates simulated wallets using
params.NewSimWallets.- It defines simulation parameters (
simParams) including the number of nodes, average block duration, and mock contract libraries for management and ERC20 contracts.- It sets the starting port for the network (
StartPort: startPort).This function appears to be a setup utility for the test suite, configuring a simulated environment with one node and mock contracts, which is a common practice for integration tests that require a network-like environment.
Given this context, the
createTenNetworkfunction seems to be used correctly within the test suite, as it is designed to set up a test network with specified parameters. The usage of this function in the test suite is consistent with its intended purpose.* 669-669: The `waitServerIsReady` function is used to wait for the server to be ready to receive requests. It's important to ensure that this function is robust and handles all potential error cases, as a server not being ready could lead to failed tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the createTenNetwork function is used consistently and correctly throughout the test suite. rg --type go 'createTenNetwork'Length of output: 208
Script:
#!/bin/bash # Review the implementation of the createTenNetwork function and its usage in the test suite. rg --type go -A 10 'func createTenNetwork'Length of output: 1038
Verification successful
The
waitServerIsReadyfunction appears in two test files:integration/obscuroscan/obscuroscan_test.goandintegration/obscurogateway/tengateway_test.go. In both instances, the function is used to ensure the server is ready before proceeding with the tests. The function is defined in both files, suggesting that it is a utility function specific to each test suite.Given this information, it seems that the function is used consistently within the context of the test suites for
obscuroscanandtengateway. However, without reviewing the implementation details of thewaitServerIsReadyfunction, we cannot fully verify the robustness and error handling within the function itself.To proceed with the verification, we should review the implementation of the
waitServerIsReadyfunction in both test files to ensure it handles potential error cases effectively.
The implementation of the
waitServerIsReadyfunction in bothintegration/obscuroscan/obscuroscan_test.goandintegration/obscurogateway/tengateway_test.goappears to be identical. The function waits for up to 30 seconds, checking every 500 milliseconds to see if the server is ready by making a GET request to the server's health endpoint. If a connection error occurs, it continues to retry until the timeout is reached. For other types of errors, it returns immediately.This implementation seems to handle the case where the server is not immediately ready by allowing some time for it to boot up and become responsive. The use of a health endpoint is a common pattern for checking server readiness, and the function's logic to handle connection errors by retrying suggests that it is designed to be robust against transient startup issues.
Given the output from the scripts, the
waitServerIsReadyfunction is consistently implemented across the two test files and includes basic error handling for connection-related issues. Therefore, the review comment about ensuring that the function is robust and handles all potential error cases is addressed by the current implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the waitServerIsReady function is used consistently and correctly throughout the test suite. rg --type go 'waitServerIsReady'Length of output: 387
Script:
#!/bin/bash # Review the implementation of the waitServerIsReady function in both test files. rg --type go -A 10 'func waitServerIsReady' integration/obscuroscan/obscuroscan_test.go rg --type go -A 10 'func waitServerIsReady' integration/obscurogateway/tengateway_test.goLength of output: 907
| func testSubscriptionTopics(t *testing.T, httpURL, wsURL string, w wallet.Wallet) { | ||
| user0, err := NewUser([]wallet.Wallet{w}, httpURL, wsURL) | ||
| require.NoError(t, err) | ||
| fmt.Printf("Created user with encryption token: %s\n", user0.tgClient.UserID()) | ||
|
|
||
| user1, err := NewUser([]wallet.Wallet{datagenerator.RandomWallet(integration.TenChainID), datagenerator.RandomWallet(integration.TenChainID)}, httpURL, wsURL) | ||
| require.NoError(t, err) | ||
| fmt.Printf("Created user with encryption token: %s\n", user0.tgClient.UserID()) | ||
|
|
||
| // register all the accounts for that user | ||
| err = user0.RegisterAccounts() | ||
| require.NoError(t, err) | ||
| err = user1.RegisterAccounts() | ||
| require.NoError(t, err) | ||
|
|
||
| var amountToTransfer int64 = 1_000_000_000_000_000_000 | ||
| // Transfer some funds to user1 to be able to make transactions | ||
| _, err = transferETHToAddress(user0.HTTPClient, user0.Wallets[0], user1.Wallets[0].Address(), amountToTransfer) | ||
| require.NoError(t, err) | ||
| time.Sleep(5 * time.Second) | ||
| _, err = transferETHToAddress(user0.HTTPClient, user0.Wallets[0], user1.Wallets[1].Address(), amountToTransfer) | ||
| require.NoError(t, err) | ||
|
|
||
| // Print balances of all registered accounts to check if all accounts have funds | ||
| err = user0.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
| err = user1.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
|
|
||
| // deploy events contract | ||
| deployTx := &types.LegacyTx{ | ||
| Nonce: w.GetNonceAndIncrement(), | ||
| Gas: uint64(1_000_000), | ||
| GasPrice: gethcommon.Big1, | ||
| Data: gethcommon.FromHex(eventsContractBytecode), | ||
| } | ||
|
|
||
| signedTx, err := w.SignTransaction(deployTx) | ||
| require.NoError(t, err) | ||
|
|
||
| err = user0.HTTPClient.SendTransaction(context.Background(), signedTx) | ||
| require.NoError(t, err) | ||
|
|
||
| contractReceipt, err := integrationCommon.AwaitReceiptEth(context.Background(), user0.HTTPClient, signedTx.Hash(), time.Minute) | ||
| require.NoError(t, err) | ||
|
|
||
| // user0 subscribes to all events from that smart contract, user1 only an event with a topic of his first account | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var topics [][]gethcommon.Hash | ||
| t1 := gethcommon.BytesToHash(user1.Wallets[1].Address().Bytes()) | ||
| topics = append(topics, nil) | ||
| topics = append(topics, []gethcommon.Hash{t1}) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, topics, user1.WSClient, &user1logs) | ||
|
|
||
| // TODO: add a comment | ||
| _, err = integrationCommon.InteractWithSmartContract(user0.HTTPClient, user0.Wallets[0], eventsContractABI, "setMessage", "user0Event1", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user0.HTTPClient, user0.Wallets[0], eventsContractABI, "setMessage", "user0Event2", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage", "user1Event1", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[1], eventsContractABI, "setMessage", "user1Event2", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // wait a few seconds to be completely sure all events arrived | ||
| time.Sleep(time.Second * 3) | ||
|
|
||
| // Assert the number of logs received by each client | ||
| // user0 should see two lifecycle events (1 for each interaction with the smart contract) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see only one event (the other is filtered out because of the topic filter) | ||
| assert.Equal(t, 1, len(user1logs)) | ||
| } |
There was a problem hiding this comment.
The new test function testSubscriptionTopics has been added to verify the functionality of subscription topics over WebSocket connections. It's important to ensure that this test covers all relevant cases and that the TODO comment on line 304 is addressed before merging.
There was a problem hiding this comment.
@zkokelj The check for the TODO comment in the testSubscriptionTopics function didn't return any results, which suggests that the TODO has indeed been addressed. If there's anything else you need me to look into, please let me know!
\
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the content of the testSubscriptionTopics function to verify the resolution of the TODO.
ast-grep --lang go --pattern $'func testSubscriptionTopics($_, $_, $_, $_) {
$$$
// TODO: $_
$$$
}'
Length of output: 106
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/obscurogateway/tengateway_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/obscurogateway/tengateway_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/tengateway_test.go (3 hunks)
- tools/walletextension/accountmanager/account_manager.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- integration/obscurogateway/tengateway_test.go
- tools/walletextension/accountmanager/account_manager.go
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 (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- tools/walletextension/accountmanager/account_manager.go
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 (2)
- integration/obscurogateway/tengateway_test.go (3 hunks)
- tools/walletextension/accountmanager/account_manager.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/obscurogateway/tengateway_test.go
Additional comments: 13
tools/walletextension/accountmanager/account_manager.go (13)
- 4-4: The import of the
bytespackage is new and should be used in the file. Ensure that it is indeed utilized in the code changes.- 14-14: The import of the
storagepackage is new. Verify that the new storage functionality is integrated correctly and securely handles data.- 37-46: The
AccountManagerstruct has been modified to include new fields for HTTP clients and WebSocket binding addresses. Ensure that these fields are properly initialized and used throughout the file.- 50-57: The
NewAccountManagerfunction has been updated to initialize the new fields in theAccountManagerstruct. Confirm that the parameters passed to this function are correctly assigned to the struct fields.- 67-68: The
GetAllAddressesWithClientsfunction now returns addresses associated with HTTP clients. Ensure that the conversion fromgethcommon.Addresstostringis correct and that the addresses are properly formatted.- 78-78: The
AddClientfunction has been updated to add HTTP clients to theaccountClientsHTTPmap. Verify that the client is correctly stored and that thread safety is maintained with the mutex.- 75-84: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [81-114]
The
ProxyRequestfunction has been modified to handle subscription requests using WebSocket clients. Ensure that the logic for handling subscriptions and unsubscribing is correct and that errors are properly handled.
- 117-150: The
suggestSubscriptionClientfunction is new and creates WebSocket clients for subscriptions. Verify that the clients are created correctly and that the function handles errors appropriately.- 164-217: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [152-182]
The
filterAccountsfunction is new and filters accounts based on the subscription filter criteria. Ensure that the filter logic is correct and that the function properly handles JSON marshaling and unmarshaling.
- 185-195: The
createClientsForAccountsfunction is new and creates WebSocket clients for all accounts. Verify that the clients are created correctly and that the function handles errors appropriately.- 202-214: The
executeCallfunction has been modified to handle Ten RPC requests. Ensure that the logic for selecting the appropriate client is correct and that the function handles errors appropriately.- 1-24: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [230-260]
The helper functions
parseParams,checkForFromField, andsearchDataFieldForAccountare used to process RPC request parameters. Verify that these functions correctly parse and handle the parameters, and that they properly interact with theaccClientsmap.
- 1-24: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [260-320]
The
submitCallandsetFromFieldIfMissingfunctions are used to submit RPC calls and ensure thefromfield is set. Verify that these functions correctly handle the request and response objects, and that they properly interact with therpc.EncRPCClient.
Why this change is needed
We want to use http connections from the gateway to the validator. Having web socket connections open for a lot of users can cause gateway to crash. It is also problematic if we want to have it behind load balancer.
Websocket connections are still needed for subscriptions.
https://github.com/ten-protocol/ten-internal/issues/2505
What changes were made as part of this PR
Changed ws connection to http, creating ws connections for subscriptions + added a test for subscription topics
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks
https://github.com/ten-protocol/ten-test/actions/runs/7504133752/job/20430444139