Preparation for subscribing with multiple accounts and test that confirms current issues #1516
Conversation
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
2 times, most recently
from
e264045 to
814037b
Compare
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
from
a2da0af to
a737a8c
Compare
WalkthroughThe changes across the codebase primarily focus on enhancing the Obscuro Gateway's functionality and improving the test coverage. The updates include the addition of a new Changes
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: 13
Files selected for processing (11)
- go/rpc/client.go (1 hunks)
- integration/manualtests/connection_test.go (1 hunks)
- integration/obscurogateway/events_contract.sol (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
- tools/walletextension/accountmanager/account_manager.go (11 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/common.go (2 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Files skipped from review due to trivial changes (4)
- go/rpc/client.go
- integration/manualtests/connection_test.go
- integration/obscurogateway/events_contract.sol
- tools/walletextension/common/constants.go
Additional comments (Suppressed): 14
integration/obscurogateway/obscurogateway_test.go (3)
1-32: The package name has been changed from
faucettogatewaylib. Ensure that all references to this package in the codebase have been updated accordingly. Also, the import statements have been reorganized and some new packages have been imported (github.com/ethereum/go-ethereum,github.com/ethereum/go-ethereum/accounts/abi/bind,github.com/ethereum/go-ethereum/crypto,github.com/ethereum/go-ethereum/ethclient,github.com/ethereum/go-ethereum/rlp,github.com/ethereum/go-ethereum/accounts/abi). Make sure these new packages are necessary and used in the code.48-55: The test function name has been changed from
TestObscuroGatewaytoTestObscuroGatewaySubscriptionsWithMultipleAccounts. Ensure that this new test function name accurately reflects the test's purpose. Also, verify that any references to the old test function name have been updated.61-67: A new configuration parameter
DBTypehas been added toobscuroGatewayConf. Ensure that this new parameter is handled correctly in theNewWalletExtensionContainerFromConfigfunction and that it doesn't break any existing functionality.tools/walletextension/api/utils.go (2)
12-15: The function
parseRequestnow returns a pointer tocommon.RPCRequestinstead ofaccountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type. Also, verify that thecommon.RPCRequeststruct has all the necessary fields and methods required by the callers ofparseRequest.35-38: The function
parseRequestnow returns a pointer tocommon.RPCRequestinstead ofaccountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type. Also, verify that thecommon.RPCRequeststruct has all the necessary fields and methods required by the callers ofparseRequest.tools/walletextension/subscriptions/subscriptions.go (1)
- 108-115: The TODO comment indicates that there is a decision to be made about where to store
subscriptionsIDHashandsubscriptionIDS. This is a design decision that should be made before the code is merged. Consider discussing this with your team and implementing the chosen solution.tools/walletextension/common/common.go (3)
99-103: The
RPCRequeststruct is introduced to encapsulate the structure of an RPC request. This is a good practice as it improves code readability and maintainability.106-112: The
Clonemethod is introduced to create a new instance ofRPCRequest. This is a good practice as it allows for safe copying ofRPCRequestinstances without the risk of unintentional mutation of the original instance.114-131: The
PrepareLogResponsefunction is introduced to format the log to be sent as an Eth JSON-RPC response. This function is well-structured and handles error cases appropriately. However, the comment on line 115 suggests that this function should be moved to a subscriptions package once it is used only there. This is a good suggestion as it would improve code organization and modularity.tools/walletextension/accountmanager/account_manager.go (2)
43-52: The
subscriptionsManagerfield has been added to theAccountManagerstruct and is being initialized in theNewAccountManagerfunction. This is a significant change and should be verified to ensure that it doesn't break any existing functionality. Also, ensure that thesubscriptionsManageris being used correctly throughout the codebase.312-316: The
wecommon.PrepareLogResponsefunction is being used instead of theprepareLogResponsefunction. Ensure that this change is intentional and that the new function provides the same functionality as the old one.tools/walletextension/wallet_extension.go (3)
18-23: The import statement for
accountmanagerhas been replaced withcommon. Ensure that all references toaccountmanagerin the code have been updated to usecommoninstead.72-75: The function signature for
ProxyEthRequesthas been updated to usecommon.RPCRequestinstead ofaccountmanager.RPCRequest. Ensure that all calls to this function have been updated to match the new signature.378-383: The function signature for
getStorageAtInterceptorhas been updated to usecommon.RPCRequestinstead ofaccountmanager.RPCRequest. Ensure that all calls to this function have been updated to match the new signature.
| go func() { | ||
| for idAndLog := range commonChannel { | ||
| if userConn.IsClosed() { | ||
| // Log that websocket was closed - unsubscribing is handled by periodic checks in separate goroutine | ||
| s.logger.Info("received log but websocket was closed on subscription", log.SubIDKey, idAndLog.SubID) | ||
| return | ||
| } | ||
|
|
||
| jsonResponse, err := wecommon.PrepareLogResponse(idAndLog) | ||
| if err != nil { | ||
| s.logger.Error("could not marshal log response to JSON on subscription.", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
|
|
||
| err = userConn.WriteResponse(jsonResponse) | ||
| if err != nil { | ||
| s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
The goroutine that sends all logs from commonChannel to user (via userConn) does not have a way to stop other than commonChannel being closed. This could lead to a goroutine leak if commonChannel is never closed. Consider adding a way to stop this goroutine when HandleNewSubscriptions returns.
| s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| continue |
There was a problem hiding this comment.
The error message in the log statement is not formatted correctly. The % symbol is not followed by any verb. Consider fixing the error message.
s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, idAndLog.SubID, log.ErrKey, err)
s.logger.Error("could not write the JSON log to the websocket on subscription", log.SubIDKey, idAndLog.SubID, log.ErrKey, err)| s.logger.Info(fmt.Sprintf("Subscribing client: %s for request: %s", client, req)) | ||
|
|
||
| // Subscribe using the provided rpc.Client, all subscriptions have the same channel | ||
| subscription, err := client.Subscribe(context.Background(), rpcResp, rpc.SubscribeNamespace, commonChannel, req.Params...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not call %s with params %v. Cause: %w", req.Method, req.Params, err) | ||
| } | ||
|
|
||
| return subscription, nil | ||
| } |
There was a problem hiding this comment.
The function addSubscription does not handle the case where req.Params is nil. This could lead to a panic if req.Params is nil. Consider adding a nil check for req.Params.
s.logger.Info(fmt.Sprintf("Subscribing client: %s for request: %s", client, req))
if req.Params == nil {
return nil, fmt.Errorf("could not subscribe as subscription params is nil")
}
// Subscribe using the provided rpc.Client, all subscriptions have the same channel
subscription, err := client.Subscribe(context.Background(), rpcResp, rpc.SubscribeNamespace, commonChannel, req.Params...)
if err != nil {
return nil, fmt.Errorf("could not call %s with params %v. Cause: %w", req.Method, req.Params, err)
}
return subscription, nil| //if rpcReq.Method == rpc.Unsubscribe { | ||
| // // TODO: The unsubscribe param is hash of concatenated subscriptions. | ||
| // // We need to iterate over them and unsubscribe from all subscriptions | ||
| // // fmt.Println("we need to handle unsubscribe here") | ||
| //} |
There was a problem hiding this comment.
The code for handling the rpc.Unsubscribe method is commented out. If this is intentional and for testing purposes only, remember to uncomment it before merging the changes. Also, ensure that the logic for unsubscribing from all subscriptions is implemented correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
Additional comments (Suppressed): 3
integration/obscurogateway/obscurogateway_test.go (3)
1-32: The import section has been significantly modified. Ensure that all the newly added packages are used in the code and that none of the removed packages are still required. Also, verify that the change from the
faucetpackage to thegatewaylibpackage does not affect the rest of the codebase.48-55: The test function name has been changed from
TestObscuroGatewaytoTestObscuroGatewaySubscriptionsWithMultipleAccounts. Make sure that this change is reflected in all places where this function is called or referenced. Also, ensure that the new function name accurately represents the test being performed.61-67: The
DBTypefield has been added to theobscuroGatewayConfconfiguration. Ensure that the "sqlite" value is valid and that the database type is properly handled in the code. Also, verify that this change does not affect the rest of the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- integration/obscurogateway/obscurogateway_test.go (6 hunks)
Additional comments (Suppressed): 4
integration/obscurogateway/obscurogateway_test.go (4)
1-4: The package name has been changed from
faucettogatewaylib. Ensure that all references to this package throughout the codebase have been updated to match the new name.9-33: Several new import statements have been added, and some have been removed. Ensure that all the newly added packages are being used in the code, and that none of the removed packages are still needed.
49-56: The test function name has been changed from
TestObscuroGatewaytoTestObscuroGatewaySubscriptionsWithMultipleAccounts. Make sure that this new name accurately reflects the purpose of the test.62-68: The
DBTypefield has been added to theobscuroGatewayConfconfiguration. Ensure that the value "sqlite" is appropriate for your use case and that the necessary database setup and teardown are handled correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Files selected for processing (3)
- integration/obscurogateway/obscurogateway_test.go (6 hunks)
- tools/walletextension/accountmanager/account_manager.go (11 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
Additional comments (Suppressed): 16
integration/obscurogateway/obscurogateway_test.go (4)
1-4: The package name has been changed from
faucettogatewaylib. Ensure that all references to this package throughout the codebase have been updated to reflect this change.9-33: Several new imports have been added and some old ones have been removed. Make sure that all the new imports are used in the code and that none of the removed imports were necessary.
49-56: The test function name has been changed from
TestObscuroGatewaytoTestObscuroGatewaySubscriptionsWithMultipleAccounts. Ensure that this change does not affect any test suites or scripts that may be calling this function.62-68: A new configuration parameter
DBTypehas been added. Ensure that this parameter is properly handled in theNewWalletExtensionContainerFromConfigfunction and that it doesn't break any existing functionality.tools/walletextension/accountmanager/account_manager.go (11)
8-15: The import of
github.com/obscuronet/go-obscuro/tools/walletextension/subscriptionsis new. Ensure that thesubscriptionspackage is correctly implemented and tested as it is now a part of theAccountManagerstruct.20-25: The import of
github.com/go-kit/kit/transport/http/jsonrpchas been removed. Make sure this does not affect any functionality that relied on this package.30-33: The constant
methodEthSubscriptionhas been removed. If this constant was used elsewhere in the code, ensure those references have been updated or removed.39-54: A new field
subscriptionsManagerhas been added to theAccountManagerstruct and is being initialized in theNewAccountManager()function. This is a significant change and should be thoroughly tested to ensure it works as expected and does not introduce any side effects.60-87: The
ProxyRequest()function has been updated to handle subscriptions using the newsubscriptionsManager. Ensure that this new implementation correctly handles subscriptions and does not introduce any regressions in functionality.139-145: The function signature of
executeCall()has been updated to use*wecommon.RPCRequestinstead of*RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.171-177: The function signature of
suggestAccountClient()has been updated to use*wecommon.RPCRequestinstead of*RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.286-292: The function signature of
executeSubscribe()has been updated to use*wecommon.RPCRequestinstead of*RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.307-313: The
prepareLogResponse()function has been replaced withwecommon.PrepareLogResponse(). Ensure that this new function correctly prepares log responses and does not introduce any regressions in functionality.345-351: The function signature of
submitCall()has been updated to use*wecommon.RPCRequestinstead of*RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.401-403: The
prepareLogResponse()function and theRPCRequeststruct along with itsClone()method have been removed. If these were used elsewhere in the code, ensure those references have been updated or removed.tools/walletextension/subscriptions/subscriptions.go (1)
- 33-35: The previous comment about handling the case where
req.Paramsis nil is still valid. The code should be updated to handle this case to prevent potential panics.if len(req.Params) == 0 { + if req.Params == nil { + return fmt.Errorf("could not subscribe as subscription namespace is nil") + } return fmt.Errorf("could not subscribe as no subscription namespace was provided") }
| subscriptions := make([]*gethrpc.ClientSubscription, 0, len(clients)) | ||
| subscriptionIDS := make([]string, 0, len(clients)) | ||
|
|
||
| // TODO: Create a buffered channel and perform deduplication of logs or implement additional logic to filter logs |
There was a problem hiding this comment.
The TODO comment indicates that there is a plan to create a buffered channel and implement logic to filter logs. This is a good idea as it can help to improve the performance of the system by reducing the number of logs that need to be processed and sent to the user. However, it's important to ensure that the implementation of this feature does not introduce any data races or other concurrency issues.
| go func() { | ||
| for { | ||
| if userConn.IsClosed() { | ||
| for _, subscription := range subscriptions { | ||
| subscription.Unsubscribe() | ||
| } | ||
| subscriptions = []*gethrpc.ClientSubscription{} | ||
| return | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The goroutine that checks if the user connection is closed and unsubscribes from all subscriptions for this user could potentially lead to a data race condition. If the subscriptions slice is being accessed or modified concurrently in another goroutine, it could lead to unexpected behavior. Consider using a mutex to ensure that only one goroutine can access or modify the subscriptions slice at a time.
| go func() { | ||
| for idAndLog := range commonChannel { | ||
| if userConn.IsClosed() { | ||
| // Log that websocket was closed - unsubscribing is handled by periodic checks in separate goroutine | ||
| s.logger.Info("received log but websocket was closed on subscription", log.SubIDKey, idAndLog.SubID) | ||
| return | ||
| } | ||
|
|
||
| jsonResponse, err := wecommon.PrepareLogResponse(idAndLog) | ||
| fmt.Println("We have a log: ", string(jsonResponse)) | ||
| if err != nil { | ||
| s.logger.Error("could not marshal log response to JSON on subscription.", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
| err = userConn.WriteResponse(jsonResponse) | ||
| if err != nil { | ||
| s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
The goroutine that sends all logs from the common channel to the user could potentially lead to a data race condition. If the commonChannel is being accessed or modified concurrently in another goroutine, it could lead to unexpected behavior. Consider using a mutex to ensure that only one goroutine can access or modify the commonChannel at a time.
| // We need to store subscriptionsIDHash and subscriptionIDS and have them available for unsubscribe - | ||
| // where is the best place to store them? | ||
| // - 1. option is database -> More complicated, | ||
| // can contain elements that are not relevant anymore in case of crashes, etd. | ||
| // - 2. option is in-memory storage, it si simpler, but will consume more RAM, | ||
| // easier to handle since on every crash/restart it is cleared (and also all the subscriptions are dropped by our logic) |
There was a problem hiding this comment.
The TODO comment indicates that there is a plan to store the subscriptionsIDHash and subscriptionIDS for future unsubscribe operations. The choice between database storage and in-memory storage depends on the specific requirements and constraints of the system. Database storage can provide persistence and durability, but it can be more complex and slower. In-memory storage can be faster and simpler, but it can consume more RAM and does not provide persistence. Consider the trade-offs carefully before making a decision.
| func (s *SubscriptionManager) addSubscription(client rpc.Client, req *wecommon.RPCRequest, rpcResp *interface{}, commonChannel chan common.IDAndLog) (*gethrpc.ClientSubscription, error) { | ||
| s.logger.Info(fmt.Sprintf("Subscribing client: %s for request: %s", client, req)) | ||
|
|
||
| // Subscribe using the provided rpc.Client, all subscriptions have the same channel | ||
| subscription, err := client.Subscribe(context.Background(), rpcResp, rpc.SubscribeNamespace, commonChannel, req.Params...) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not call %s with params %v. Cause: %w", req.Method, req.Params, err) | ||
| } | ||
|
|
||
| return subscription, nil | ||
| } |
There was a problem hiding this comment.
The addSubscription function does not handle the case where req.Params is nil. This could lead to a panic if req.Params is nil. Consider adding a nil check for req.Params.
+ if req.Params == nil {
+ return nil, fmt.Errorf("could not subscribe as subscription namespace is nil")
+ }
subscription, err := client.Subscribe(context.Background(), rpcResp, rpc.SubscribeNamespace, commonChannel, req.Params...)
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
from
aa60936 to
e877138
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Files selected for processing (9)
- integration/obscurogateway/events_contract.sol (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
- tools/walletextension/accountmanager/account_manager.go (11 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Files skipped from review due to trivial changes (3)
- integration/obscurogateway/events_contract.sol
- tools/walletextension/common/constants.go
- tools/walletextension/wallet_extension.go
Additional comments (Suppressed): 21
tools/walletextension/api/utils.go (2)
12-15: The function
parseRequesthas been updated to return acommon.RPCRequestinstead ofaccountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type. Also, verify that thecommon.RPCRequeststruct has the same fields and types as theaccountmanager.RPCRequeststruct to avoid any potential issues.35-38: The return type of
parseRequesthas been changed fromaccountmanager.RPCRequesttocommon.RPCRequest. This change seems to be in line with the PR summary, which mentions the use ofcommon.RPCRequestinstead ofaccountmanager.RPCRequest. However, ensure that this change does not affect other parts of the code that rely on the old return type.tools/walletextension/common/common.go (5)
3-16: The import section has been updated to include
encoding/jsonandgithub.com/go-kit/kit/transport/http/jsonrpc. Ensure that these packages are used in the code and that they are compatible with the rest of the codebase.40-46: No changes have been made to the
CalculateUserIDfunction. It continues to calculate the user ID from a public key.95-97: No changes have been made to the
CreateEncClientfunction. It continues to create an encrypted RPC client.99-112: A new
RPCRequeststruct has been introduced, along with aClonemethod. This struct and method are used to handle RPC requests. Ensure that this new struct and method are used correctly throughout the codebase.114-131: The
PrepareLogResponsefunction has been added. This function formats the log to be sent as an Eth JSON-RPC response. Ensure that this function is used correctly and that the log format is compatible with the rest of the system.tools/walletextension/subscriptions/subscriptions.go (1)
- 30-32: The previous comment about handling the case where
req.Paramsis nil has been addressed. The check forlen(req.Params) == 0will also cover the case wherereq.Paramsis nil, as the length of a nil slice is 0 in Go.integration/obscurogateway/obscurogateway_test.go (3)
3-14: The new hunk introduces several new imports from the
github.com/ethereum/go-ethereumandgithub.com/obscuronet/go-obscuro/go/common/retrypackages. Ensure that these packages are correctly installed and that their versions are compatible with the rest of the project.111-112: The error check at the end of the previous function is not included in the context. Make sure that the error being checked here is handled correctly in the function not shown in the hunks.
425-431: The new hunk modifies the
createObscuroNetworkfunction to create 5 simulated wallets instead of 1. Ensure that this change is reflected in all parts of the code that use this function and that it does not break any assumptions in the rest of the code.tools/walletextension/accountmanager/account_manager.go (10)
8-11: The import of the
subscriptionspackage is new. This package is used to manage subscriptions, which is a new feature introduced in this PR. TheSubscriptionManageris used to handle new subscriptions and forward event logs to user connections.20-32: The import of
github.com/go-kit/kit/transport/http/jsonrpchas been removed. This package was previously used for JSON-RPC over HTTP transport. The removal of this import suggests that the code no longer uses this package, likely due to the changes in how subscriptions are handled.38-44: The
AccountManagerstruct now includes asubscriptionsManagerfield. This field is used to manage subscriptions, which is a new feature introduced in this PR. TheNewAccountManagerfunction has been updated to initialize this field.59-81: The
ProxyRequestfunction has been updated to use thesubscriptionsManagerto handle new subscriptions when the request method isrpc.Subscribe. This is a change from the previous implementation, which directly executed the subscription request. The error handling has also been improved, with an error message being logged if there is an error handling the new subscriptions.134-140: The
executeCallfunction now uses thewecommon.RPCRequesttype instead of theRPCRequesttype. This change is consistent with the updates to other functions to use thewecommon.RPCRequesttype.166-172: The
suggestAccountClientfunction now uses thewecommon.RPCRequesttype instead of theRPCRequesttype. This change is consistent with the updates to other functions to use thewecommon.RPCRequesttype.281-287: The
executeSubscribefunction now uses thewecommon.RPCRequesttype instead of theRPCRequesttype. This change is consistent with the updates to other functions to use thewecommon.RPCRequesttype.302-308: The
executeSubscribefunction now uses thewecommon.PrepareLogResponsefunction instead of theprepareLogResponsefunction. This change is consistent with the updates to other functions to use functions from thewecommonpackage.340-346: The
submitCallfunction now uses thewecommon.RPCRequesttype instead of theRPCRequesttype. This change is consistent with the updates to other functions to use thewecommon.RPCRequesttype.396-398: The
prepareLogResponsefunction and theRPCRequeststruct have been removed. These were replaced by thewecommon.PrepareLogResponsefunction and thewecommon.RPCRequesttype, respectively. This change simplifies the code and reduces duplication.
| s.logger.Info(fmt.Sprintf("Subscribing client: %s for request: %s", clients[0], req)) | ||
| //ch := make(chan common.IDAndLog) | ||
|
|
||
| funnelMultipleAccountsChan := make(chan common.IDAndLog) // weary of 1 msg at a time | ||
|
|
||
| go func() { | ||
| for { | ||
| select { | ||
| case data := <-funnelMultipleAccountsChan: | ||
| jsonResponse, err := wecommon.PrepareLogResponse(data) | ||
| if err != nil { | ||
| s.logger.Error("could not marshal log response to JSON on subscription.", log.SubIDKey, data.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
|
|
||
| s.logger.Trace(fmt.Sprintf("Forwarding log from Obscuro node: %s", jsonResponse), log.SubIDKey, data.SubID) | ||
| err = userConn.WriteResponse(jsonResponse) | ||
| if err != nil { | ||
| s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, data.SubID, log.ErrKey, err) | ||
| continue | ||
| } | ||
|
|
||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
This goroutine is responsible for forwarding the logs from the Obscuro node to the user. However, there is no mechanism to stop this goroutine when it's no longer needed, which could lead to a goroutine leak. Consider adding a mechanism to stop this goroutine when the user connection is closed or when all subscriptions are unsubscribed.
| for _, client := range clients { | ||
| subscription, err := client.Subscribe(context.Background(), resp, rpc.SubscribeNamespace, funnelMultipleAccountsChan, req.Params...) | ||
| if err != nil { | ||
| return fmt.Errorf("could not call %s with params %v. Cause: %w", req.Method, req.Params, err) | ||
| } | ||
|
|
||
| // We listen for incoming messages on the subscription. | ||
| //go func() { | ||
| // for { | ||
| // select { | ||
| // case idAndLog := <-ch: | ||
| // if userConn.IsClosed() { | ||
| // s.logger.Info("received log but websocket was closed on subscription", log.SubIDKey, idAndLog.SubID) | ||
| // return | ||
| // } | ||
| // | ||
| // funnelMultipleAccountsChan <- idAndLog | ||
| // | ||
| // case err = <-subscription.Err(): | ||
| // // An error on this channel means the subscription has ended, so we exit the loop. | ||
| // if userConn != nil && err != nil { | ||
| // userConn.HandleError(err.Error()) | ||
| // } | ||
| // | ||
| // return | ||
| // } | ||
| // } | ||
| //}() | ||
|
|
||
| // We periodically check if the websocket is closed, and terminate the subscription. | ||
| go func() { | ||
| for { | ||
| if userConn.IsClosed() { | ||
| subscription.Unsubscribe() | ||
| return | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
This loop creates a subscription for each client and starts a goroutine to periodically check if the user connection is closed. If the user connection is closed, it unsubscribes the subscription and stops the goroutine. However, if an error occurs while subscribing, the function returns immediately, leaving the previously started goroutines running. Consider handling errors in a way that ensures all started goroutines are stopped before returning from the function.
| s.logger.Trace(fmt.Sprintf("Forwarding log from Obscuro node: %s", jsonResponse), log.SubIDKey, data.SubID) | ||
| err = userConn.WriteResponse(jsonResponse) | ||
| if err != nil { | ||
| s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, data.SubID, log.ErrKey, err) |
There was a problem hiding this comment.
The error message string contains a % placeholder, but no corresponding value is provided. This could lead to a confusing error message. Consider fixing the error message string.
- s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, data.SubID, log.ErrKey, err)
+ s.logger.Error("could not write the JSON log to the websocket on subscription", log.SubIDKey, data.SubID, log.ErrKey, err)| //go func() { | ||
| // for { | ||
| // select { | ||
| // case idAndLog := <-ch: | ||
| // if userConn.IsClosed() { | ||
| // s.logger.Info("received log but websocket was closed on subscription", log.SubIDKey, idAndLog.SubID) | ||
| // return | ||
| // } | ||
| // | ||
| // funnelMultipleAccountsChan <- idAndLog | ||
| // | ||
| // case err = <-subscription.Err(): | ||
| // // An error on this channel means the subscription has ended, so we exit the loop. | ||
| // if userConn != nil && err != nil { | ||
| // userConn.HandleError(err.Error()) | ||
| // } | ||
| // | ||
| // return | ||
| // } | ||
| // } | ||
| //}() |
There was a problem hiding this comment.
The commented-out code should be removed if it's not needed. Keeping it in the codebase could confuse other developers and make the code harder to maintain.
| //if len(req.Params) == 0 { | ||
| // return fmt.Errorf("could not subscribe as no subscription namespace was provided") | ||
| //} | ||
| // | ||
| //// create a chanel that will collect the data from all subscriptions | ||
| //commonChannel := make(chan common.IDAndLog) | ||
| // | ||
| //// save subscriptions | ||
| //subscriptions := make([]*gethrpc.ClientSubscription, 0, len(clients)) | ||
| //subscriptionIDS := make([]string, 0, len(clients)) | ||
| // | ||
| //// TODO: Create a buffered channel and perform deduplication of logs or implement additional logic to filter logs | ||
| // | ||
| //// Do periodic checks if userConn is closed and unsubscribe from all subscriptions for this user | ||
| //go func() { | ||
| // for { | ||
| // if userConn.IsClosed() { | ||
| // for _, subscription := range subscriptions { | ||
| // subscription.Unsubscribe() | ||
| // } | ||
| // subscriptions = []*gethrpc.ClientSubscription{} | ||
| // return | ||
| // } | ||
| // time.Sleep(100 * time.Millisecond) | ||
| // } | ||
| //}() | ||
| // | ||
| //// Send all logs from common channel to user (via userConn) | ||
| //go func() { | ||
| // for { | ||
| // select { | ||
| // case idAndLog := <-commonChannel: | ||
| // if userConn.IsClosed() { | ||
| // // Log that websocket was closed - unsubscribing is handled by periodic checks in separate goroutine | ||
| // s.logger.Info("received log but websocket was closed on subscription", log.SubIDKey, idAndLog.SubID) | ||
| // return | ||
| // } | ||
| // | ||
| // jsonResponse, err := wecommon.PrepareLogResponse(idAndLog) | ||
| // fmt.Println("We have a log: ", string(jsonResponse)) | ||
| // if err != nil { | ||
| // s.logger.Error("could not marshal log response to JSON on subscription.", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| // continue | ||
| // } | ||
| // err = userConn.WriteResponse(jsonResponse) | ||
| // if err != nil { | ||
| // s.logger.Error("could not write the JSON log to the websocket on subscription %", log.SubIDKey, idAndLog.SubID, log.ErrKey, err) | ||
| // continue | ||
| // } | ||
| // } | ||
| // } | ||
| //}() | ||
| // | ||
| //// loop over all clients and create a new subscription for each of them | ||
| //s.logger.Info(fmt.Sprintf("Subscribing to: %d clients", len(clients))) | ||
| //for _, client := range clients { | ||
| // s.logger.Info(fmt.Sprintf("Subscribing for an event with client: %s", client)) | ||
| // fmt.Println("Subscribing to logs with client: ", client) | ||
| // var subscriptionID interface{} | ||
| // subChannel := make(chan common.IDAndLog) | ||
| // subscription, err := s.addSubscription(client, req, &subscriptionID, subChannel) | ||
| // strSubscriptionID, isOK := subscriptionID.(string) | ||
| // if err != nil || !isOK { | ||
| // s.logger.Info(fmt.Sprintf("Error subscribing: %v", err)) | ||
| // continue | ||
| // } | ||
| // // If there was no error, the subscription was successful. Store it for unsubscribing in the future | ||
| // subscriptions = append(subscriptions, subscription) | ||
| // subscriptionIDS = append(subscriptionIDS, strSubscriptionID) | ||
| // s.logger.Info(fmt.Sprintf("Subscribed with subscription ID: %s", subscriptionID)) | ||
| // | ||
| // go func() { | ||
| // for { | ||
| // select { | ||
| // case data := <-subChannel: | ||
| // commonChannel <- data | ||
| // } | ||
| // } | ||
| // }() | ||
| //} | ||
| // | ||
| //// create a response with new subscriptionID by concatenating them and computing hash of the concatenated string | ||
| //combinedSubscriptionIDS := strings.Join(subscriptionIDS, "") | ||
| //// Compute Keccak-256 hash | ||
| //subscriptionsIDHash := crypto.Keccak256([]byte(combinedSubscriptionIDS)) | ||
| //// Convert hash to hex string for better readability | ||
| //*resp = fmt.Sprintf("%x", subscriptionsIDHash) | ||
| // | ||
| //// TODO: | ||
| //// We need to store subscriptionsIDHash and subscriptionIDS and have them available for unsubscribe - | ||
| //// where is the best place to store them? | ||
| //// - 1. option is database -> More complicated, | ||
| //// can contain elements that are not relevant anymore in case of crashes, etd. | ||
| //// - 2. option is in-memory storage, it si simpler, but will consume more RAM, | ||
| //// easier to handle since on every crash/restart it is cleared (and also all the subscriptions are dropped by our logic) | ||
| // | ||
| //return nil |
There was a problem hiding this comment.
The commented-out code should be removed if it's not needed. Keeping it in the codebase could confuse other developers and make the code harder to maintain.
| ], | ||
| "name": "setMessage", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, | ||
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "string", | ||
| "name": "newMessage", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "setMessage2", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| } | ||
| ] | ||
| ` | ||
|
|
||
| _, contractAddress, err := DeploySmartContract(user0.HTTPClient, user0.Wallets[0], bytecode) | ||
| require.NoError(t, err) | ||
| fmt.Println("Deployed contract address: ", contractAddress) | ||
|
|
||
| // contract abi | ||
| contractAbi, err := abi.JSON(strings.NewReader(abiString)) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if contract was deployed and call one of the implicit getter functions | ||
| // call getter for a message | ||
| resultMessage, err := getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "bar" | ||
| // interact with smart contract and cause events to be emitted | ||
| intTx1Receipt, err := InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage", user1MessageValue, contractAddress) | ||
| require.NoError(t, err) | ||
| intTx2Receipt, err := InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage2", user1MessageValue, contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "foobar" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage", user2MessageValue, contractAddress) | ||
| require.NoError(t, err) | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage2", user2MessageValue, contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // Get Tx Receipts to check if txs were included in block | ||
| fmt.Printf("Tx1 was included in block %d\n", intTx1Receipt.BlockNumber) | ||
| fmt.Printf("Tx2 was included in block %d\n", intTx2Receipt.BlockNumber) | ||
|
|
||
| time.Sleep(30 * time.Second) | ||
| fmt.Println("user0 received logs: ", len(user0logs)) | ||
| fmt.Println("user1 received logs: ", len(user1logs)) | ||
| fmt.Println("user2 received logs: ", len(user2logs)) | ||
|
|
||
| // Assert the number of logs received by each client | ||
| // user0 should see 2 lifecycle events (1 for each interaction with setMessage2) | ||
| // assert.Equal(t, 2, len(user0logs)) | ||
|
|
||
| // user1 should see 3 events (2 lifecycle events - same as user0) and event with his interaction with setMessage | ||
| // assert.Equal(t, 3, len(user1logs)) | ||
|
|
||
| // user2 should see 3 events (2 lifecycle events - same as user0) and event with his interaction with setMessage | ||
| // FIXME: Currently we receive only 2 events, because only the first account on each client actually subscribe | ||
| // assert.Equal(t, 3, len(user2logs)) | ||
|
|
||
| time.Sleep(time.Hour) | ||
| // Gracefully shutdown | ||
| err = obscuroGwContainer.Stop() | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
This new hunk introduces a new test function TestObscuroGatewaySubscriptionsWithMultipleAccounts. The function seems to be testing the Obscuro Gateway with multiple accounts. It includes the creation of a network, configuration of the Obscuro Gateway, deployment of a smart contract, interaction with the smart contract, and checking the logs received by each user. The function appears to be logically sound and follows good practices for testing. However, there are a few points to consider:
- The function includes a hardcoded bytecode and ABI for a smart contract (lines 195-289). This could make the test brittle if the contract changes. Consider loading these from a file or a separate constant that can be easily updated.
- The function includes several
time.Sleepcalls (lines 139, 179, 348, 364). These can make the test slow and potentially flaky if the expected conditions are not met within the sleep duration. Consider using a more deterministic synchronization mechanism. - The function includes several
fmt.Printlncalls (lines 144-145, 156, 160, 164, 293, 349-351). These can be useful for debugging, but they might clutter the test output in normal operation. Consider using a logger with adjustable log level, or a test-specific logger. - The function includes commented-out assertions (lines 355, 358, 362). If these are not needed, consider removing them to avoid confusion. If they are needed in the future, consider uncommenting them and fixing any issues that prevent them from passing.
- The function includes a one-hour sleep at the end (line 364). This could make the test very slow. If this is not necessary, consider removing it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
Additional comments (Suppressed): 4
integration/obscurogateway/obscurogateway_test.go (2)
6-10: The new hunk has added several Ethereum related packages. Ensure that these packages are compatible with the rest of the codebase and that they are maintained and secure.
420-420: The number of wallets created for the simulation has been increased from 1 to 5. Ensure that this change is reflected in all parts of the code that use these wallets.
tools/walletextension/subscriptions/subscriptions.go (2)
30-32: The previous issue regarding the potential panic when
req.Paramsis nil has been addressed. The function now checks ifreq.Paramsis empty and returns an error if it is.59-69: The previous issue regarding the potential panic when
req.Paramsis nil in theaddSubscriptionfunction has been addressed. The function now checks ifreq.Paramsis empty and returns an error if it is.
| }, | ||
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "string", | ||
| "name": "newMessage", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "setMessage", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, | ||
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "string", | ||
| "name": "newMessage", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "setMessage2", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| } | ||
| ] | ||
| ` | ||
|
|
||
| _, contractAddress, err := DeploySmartContract(user0.HTTPClient, user0.Wallets[0], bytecode) | ||
| require.NoError(t, err) | ||
| fmt.Println("Deployed contract address: ", contractAddress) | ||
|
|
||
| // contract abi | ||
| contractAbi, err := abi.JSON(strings.NewReader(abiString)) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if contract was deployed and call one of the implicit getter functions | ||
| // call getter for a message | ||
| resultMessage, err := getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| time.Sleep(time.Second) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "user1PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage", "user1PrivateEvent", contractAddress) | ||
| require.NoError(t, err) | ||
| _, err = InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage2", "user1PublicEvent", contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "user2PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage", "user2PrivateEvent", contractAddress) | ||
| require.NoError(t, err) | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage2", "user2PublicEvent", contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // 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 setMessage2) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 2, len(user1logs)) // FIXME (should be 3), but we have a bug when listening to events | ||
| // user2 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 3, len(user2logs)) | ||
|
|
||
| // Gracefully shutdown | ||
| err = obscuroGwContainer.Stop() | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
This new test function TestObscuroGatewaySubscriptionsWithMultipleAccounts seems to be testing multiple functionalities at once. It's generally a good practice to have each test function focus on a single functionality for better maintainability and readability. Consider breaking this test into smaller ones, each focusing on a specific functionality.
| // todo: @pedro - is that the correct approach create new go routine to check and terminate only one subscription | ||
| go checkIfUserConnIsClosedAndUnsubscribe(userConn, subscription) |
There was a problem hiding this comment.
The comment on line 51 indicates a potential issue with the current approach of creating a new goroutine for each subscription to check if the user connection is closed. This could lead to a large number of goroutines if there are many subscriptions, which could impact performance. Consider using a single goroutine that checks all subscriptions, or a more efficient method of detecting closed connections.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tools/walletextension/accountmanager/account_manager.go (8 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
Additional comments: 4
tools/walletextension/subscriptions/subscriptions.go (2)
- 32-34: The previous comment about handling the case where
req.Paramsis nil is still valid. The code should be updated to handle this case to prevent potential panics.- if len(req.Params) == 0 { + if req.Params == nil || len(req.Params) == 0 { return fmt.Errorf("could not subscribe as no subscription namespace was provided") }Committable suggestion (Beta)
if req.Params == nil || len(req.Params) == 0 { return fmt.Errorf("could not subscribe as no subscription namespace was provided") }
- 102-110: The previous comment about the function
checkIfUserConnIsClosedAndUnsubscribecontinuously checking if the user connection is closed in a loop with a sleep of 100 milliseconds is still valid. This could lead to unnecessary CPU usage. Consider using a more efficient method to detect closed connections, such as using a channel to signal when the connection is closed.tools/walletextension/accountmanager/account_manager.go (2)
- 1-13: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [4-24]
The import section has been updated to include the new
subscriptionspackage fromgithub.com/obscuronet/go-obscuro/tools/walletextension/subscriptions. This package is used to manage subscriptions across multiple clients. Thecontextandtimepackages have been removed, indicating that the new code does not require context management or time-related functions. Thegithub.com/go-kit/kit/transport/http/jsonrpcandgithub.com/obscuronet/go-obscuro/go/common/logpackages have also been removed, suggesting changes in logging and JSON-RPC transport mechanisms.
- 271-277: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [274-329]
The
submitCallfunction has been updated to handlerpc.Callandrpc.EstimateGasrequests. The function clones the original request before modifying it, ensuring that the original request can be reused. This change improves the function's safety and maintainability.
| accountClients map[gethcommon.Address]*rpc.EncRPCClient // An encrypted RPC client per registered account | ||
| subscriptionsManager *subscriptions.SubscriptionManager | ||
| logger gethlog.Logger | ||
| } | ||
|
|
||
| func NewAccountManager(unauthedClient rpc.Client, logger gethlog.Logger) *AccountManager { | ||
| return &AccountManager{ | ||
| unauthedClient: unauthedClient, | ||
| accountClients: make(map[gethcommon.Address]*rpc.EncRPCClient), | ||
| logger: logger, | ||
| unauthedClient: unauthedClient, | ||
| accountClients: make(map[gethcommon.Address]*rpc.EncRPCClient), | ||
| subscriptionsManager: subscriptions.New(logger), | ||
| logger: logger, |
There was a problem hiding this comment.
The AccountManager struct has been updated to include a subscriptionsManager field of type *subscriptions.SubscriptionManager. This field is initialized in the NewAccountManager function using the subscriptions.New(logger) call. This change allows the AccountManager to manage subscriptions across multiple clients.
- logger gethlog.Logger
+ subscriptionsManager *subscriptions.SubscriptionManager
+ logger gethlog.LoggerCommittable suggestion (Beta)
| accountClients map[gethcommon.Address]*rpc.EncRPCClient // An encrypted RPC client per registered account | |
| subscriptionsManager *subscriptions.SubscriptionManager | |
| logger gethlog.Logger | |
| } | |
| func NewAccountManager(unauthedClient rpc.Client, logger gethlog.Logger) *AccountManager { | |
| return &AccountManager{ | |
| unauthedClient: unauthedClient, | |
| accountClients: make(map[gethcommon.Address]*rpc.EncRPCClient), | |
| logger: logger, | |
| unauthedClient: unauthedClient, | |
| accountClients: make(map[gethcommon.Address]*rpc.EncRPCClient), | |
| subscriptionsManager: subscriptions.New(logger), | |
| logger: logger, | |
| type AccountManager struct { | |
| unauthedClient rpc.Client | |
| accountClients map[gethcommon.Address]*rpc.EncRPCClient | |
| subscriptionsManager *subscriptions.SubscriptionManager | |
| logger gethlog.Logger | |
| } | |
| func NewAccountManager(unauthedClient rpc.Client, logger gethlog.Logger) *AccountManager { | |
| return &AccountManager{ | |
| unauthedClient: unauthedClient, | |
| accountClients: make(map[gethcommon.Address]*rpc.EncRPCClient), | |
| subscriptionsManager: subscriptions.New(logger), | |
| logger: logger, | |
| } | |
| } |
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
from
f23b9b8 to
3104000
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (9)
- integration/obscurogateway/events_contract.sol (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
- tools/walletextension/accountmanager/account_manager.go (8 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Files skipped from review due to trivial changes (3)
- tools/walletextension/api/utils.go
- tools/walletextension/common/constants.go
- tools/walletextension/wallet_extension.go
Additional comments: 16
integration/obscurogateway/obscurogateway_test.go (2)
6-11: The new imports from the
github.com/ethereum/go-ethereumandgithub.com/obscuronet/go-obscuro/go/common/retrypackages are introduced. Ensure that these packages are used in the code and are necessary for the functionality. Also, make sure that the versions of these packages are compatible with the rest of the project.114-360: The test function
TestObscuroGatewaySubscriptionsWithMultipleAccountsis quite large and tests multiple functionalities. It's generally a good practice to have each test function focus on a single functionality for better maintainability and readability. Consider breaking this test into smaller ones, each focusing on a specific functionality. This will also make it easier to identify the cause of a test failure.420:
The number of simulated wallets has been increased from 1 to 5. Ensure that this change is reflected in the rest of the test code and that all wallets are being used as expected.tools/walletextension/subscriptions/subscriptions.go (4)
32-34: The previous comment about handling the case where
req.Paramsis nil is still valid. The code should be updated to handle this case to prevent potential panics.90-98: The previous comment about the function
checkIfUserConnIsClosedAndUnsubscribecontinuously checking if the user connection is closed in a loop with a sleep of 100 milliseconds leading to unnecessary CPU usage is still valid. The code should be updated to use a more efficient method to detect closed connections.29-68: The previous comment about the function
HandleNewSubscriptionsreturning immediately after the first successful subscription, ignoring the rest of the clients in the slice is still valid. The code should be updated to iterate over all clients and subscribing each of them before returning.100-120: The previous comment about the function
UpdateSubscriptionMappingnot handling the case whereuserSubscriptionIDorobscuroNodeSubscriptionIDis empty is still valid. The code should be updated to handle this case to prevent incorrect mappings.tools/walletextension/common/common.go (3)
6-13: The import of the
github.com/go-kit/kit/transport/http/jsonrpcandgithub.com/obscuronet/go-obscuro/go/commonpackages are new. Ensure that these packages are used in the code and that they are compatible with the existing codebase.99-112: The
RPCRequeststruct and itsClonemethod are new additions. TheClonemethod creates a new instance ofRPCRequestwith the same values. This is a good practice for maintaining immutability and preventing unintended side effects. However, note that theParamsslice is not deeply copied, meaning changes to the slice in the originalRPCRequestwill affect the clonedRPCRequestas well. If this is not the intended behavior, consider implementing a deep copy for theParamsslice.114-131: The
PrepareLogResponsefunction is a new addition. It prepares a log response in the format of an Ethereum JSON-RPC response. The function seems to be well-implemented with proper error handling. However, there is a TODO comment indicating that this function should be moved to a subscriptions package once it is used only there. It's a good practice to keep related functionalities together, so consider moving this function to the appropriate package when it's time.tools/walletextension/accountmanager/account_manager.go (7)
4-10: The import of the
subscriptionspackage fromgithub.com/obscuronet/go-obscuro/tools/walletextension/subscriptionsis new. This package is used for managing subscriptions across multiple accounts.38-48: The
AccountManagerstruct has been updated to include asubscriptionsManagerfield of type*subscriptions.SubscriptionManager. This field is initialized in theNewAccountManagerfunction using thesubscriptions.New(logger)call. This change allows theAccountManagerto manage subscriptions across multiple clients.- logger gethlog.Logger + subscriptionsManager *subscriptions.SubscriptionManager + logger gethlog.Logger
- 59-72: The
ProxyRequestfunction has been updated to handle subscriptions using the newsubscriptionsManager. If the request method isrpc.Subscribe, the function suggests a client for the subscription, handles new subscriptions, and logs any errors that occur during this process. This change allows theAccountManagerto manage subscriptions across multiple clients.- for _, client := range clients { - return m.executeSubscribe(client, rpcReq, rpcResp, userConn) - } + err = m.subscriptionsManager.HandleNewSubscriptions(clients, rpcReq, rpcResp, userConn) + if err != nil { + m.logger.Error("Error subscribing to multiple clients") + } + return err
127-127: No changes have been made to the
executeCallfunction. It continues to execute a regular RPC call by suggesting an account client based on the request parameters and the registered account clients.159-159: No changes have been made to the
suggestAccountClientfunction. It continues to suggest an account client for a regular RPC call based on the request parameters and the registered account clients.274-274: No changes have been made to the
submitCallfunction. It continues to submit a regular RPC call to the client and handles the modification of the request parameters if necessary.327-329: No changes have been made to the
prepareCallRequestfunction. It continues to prepare a call request by modifying the request parameters if necessary.
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
from
3104000 to
ce7d570
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (9)
- integration/obscurogateway/events_contract.sol (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
- tools/walletextension/accountmanager/account_manager.go (8 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Files skipped from review due to trivial changes (4)
- integration/obscurogateway/events_contract.sol
- tools/walletextension/api/utils.go
- tools/walletextension/common/constants.go
- tools/walletextension/wallet_extension.go
Additional comments: 14
integration/obscurogateway/obscurogateway_test.go (2)
6-11: The new imports from the
github.com/ethereum/go-ethereumandgithub.com/obscuronet/go-obscuro/go/common/retrypackages are added. Ensure that these packages are used in the code and are necessary for the functionality. Also, make sure that the versions of these packages are compatible with the rest of the project.114-360: This is a new test function
TestObscuroGatewaySubscriptionsWithMultipleAccounts. It seems to be testing the functionality of the Obscuro Gateway with multiple accounts. The test appears to be comprehensive, covering various aspects such as creating users, registering accounts, transferring funds, deploying and interacting with smart contracts, and checking event subscriptions. However, this test function is quite long and complex, which could make it difficult to maintain and debug. Consider breaking it down into smaller, more manageable test functions, each focusing on a specific aspect of the functionality.420:
The number of wallets created for the simulation has been increased from 1 to 5. Ensure that this change is necessary and that it does not introduce any issues with resource usage or test execution time.tools/walletextension/subscriptions/subscriptions.go (4)
32-34: The previous issue regarding the potential panic if
req.Paramsis nil has been addressed. The function now checks ifreq.Paramsis empty and returns an error if it is.90-98: The function
checkIfUserConnIsClosedAndUnsubscribestill continuously checks if the user connection is closed in a loop with a sleep of 100 milliseconds. This could lead to unnecessary CPU usage. Consider using a more efficient method to detect closed connections, such as using a channel to signal when the connection is closed.29-68: The function
HandleNewSubscriptionsstill returns immediately after the first successful subscription, ignoring the rest of the clients in the slice. This might not be the intended behavior considering the function's purpose and the comment on line 30. Consider iterating over all clients and subscribing each of them before returning.100-120: The function
UpdateSubscriptionMappingstill does not handle the case whereuserSubscriptionIDorobscuroNodeSubscriptionIDis empty. This could lead to incorrect mappings. Consider adding checks for empty strings.tools/walletextension/common/common.go (2)
6-13: The import of
github.com/go-kit/kit/transport/http/jsonrpcandgithub.com/obscuronet/go-obscuro/go/commonare new. Ensure that these packages are used in the code and are necessary for the functionality. If they are not used, they should be removed to keep the code clean and maintainable.99-112: The
RPCRequeststruct and itsClonemethod are new additions. TheClonemethod creates a new instance ofRPCRequestwith the same values. This is a good practice for maintaining immutability and preventing unexpected side effects from modifying shared instances.tools/walletextension/accountmanager/account_manager.go (6)
38-48: The
AccountManagerstruct has been updated to include asubscriptionsManagerfield of type*subscriptions.SubscriptionManager. This field is initialized in theNewAccountManagerfunction using thesubscriptions.New(logger)call. This change allows theAccountManagerto manage subscriptions across multiple clients. This is a good practice as it separates the concerns of account management and subscription management, improving the modularity and maintainability of the code.59-72: The
ProxyRequestfunction has been updated to handle subscriptions using the newsubscriptionsManager. If the request method isrpc.Subscribe, the function suggests a client for the subscription, handles new subscriptions, and logs any errors that occur during this process. This change allows theAccountManagerto manage subscriptions across multiple clients. This is a good practice as it separates the concerns of account management and subscription management, improving the modularity and maintainability of the code.127-127: No changes have been made to the
executeCallfunction. It continues to suggest an account client and execute the RPC call.159-159: No changes have been made to the
suggestAccountClientfunction. It continues to suggest an account client based on the request and the available account clients.274-274: No changes have been made to the
submitCallfunction. It continues to handle the submission of RPC calls.327-329: The
prepareLogResponsefunction and theRPCRequeststruct have been removed. These were previously used to format the log for an Eth JSON-RPC response and to represent an RPC request, respectively. The removal of these components suggests that their functionality has been moved elsewhere or is no longer needed. Please verify that this is the case and that the removal of these components does not affect the functionality of the code.
| payload := prepareRegisterPayload(userID, pk, hexAddress) | ||
|
|
||
| req, err := http.NewRequestWithContext( | ||
| context.Background(), | ||
| http.MethodPost, | ||
| url+"/v1/authenticate/?u="+userID, | ||
| strings.NewReader(payload), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json; charset=UTF-8") | ||
|
|
||
| client := &http.Client{} | ||
| response, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| defer response.Body.Close() | ||
| return io.ReadAll(response.Body) | ||
| } |
There was a problem hiding this comment.
The registerAccount function creates a new HTTP client for each registration request. This could lead to a large number of idle connections if many accounts are being registered. Consider reusing a single HTTP client for all requests.
- client := &http.Client{}
+ client := http.DefaultClient
zkokelj
force-pushed
the
ziga/og_subscribe_with_multiple_accounts
branch
from
ce7d570 to
ec63a73
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files selected for processing (9)
- integration/obscurogateway/events_contract.sol (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (7 hunks)
- tools/walletextension/accountmanager/account_manager.go (8 hunks)
- tools/walletextension/api/utils.go (3 hunks)
- tools/walletextension/common/common.go (3 hunks)
- tools/walletextension/common/constants.go (1 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
- tools/walletextension/wallet_extension.go (3 hunks)
Files skipped from review due to trivial changes (2)
- tools/walletextension/common/constants.go
- tools/walletextension/wallet_extension.go
Additional comments: 10
tools/walletextension/subscriptions/subscriptions.go (1)
- 32-34: The previous issue regarding the potential panic if
req.Paramsis nil has been addressed. The function now checks ifreq.Paramsis empty and returns an error if it is.tools/walletextension/common/common.go (2)
6-13: The new hunk introduces additional imports:
encoding/json,github.com/go-kit/kit/transport/http/jsonrpc, andgithub.com/obscuronet/go-obscuro/go/common. Ensure these new imports are used in the code and are necessary for the new functionality.99-112: The
RPCRequeststruct and itsClonemethod have been introduced. TheClonemethod creates a new instance ofRPCRequest, which can be useful to avoid mutating the originalRPCRequest. This is a good practice for maintaining data integrity.tools/walletextension/api/utils.go (3)
15-15: The
parseRequestfunction now returns a*common.RPCRequestinstead of*accountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new return type. Also, verify that thecommon.RPCRequeststruct has the same fields and methods asaccountmanager.RPCRequestto avoid any runtime errors.38-40: The
parseRequestfunction now returns a*common.RPCRequestinstead of*accountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new return type. Also, verify that thecommon.RPCRequeststruct has the same fields and methods asaccountmanager.RPCRequestto avoid any runtime errors.87-87: The
handleEthErrorfunction now takes a*common.RPCRequestinstead of*accountmanager.RPCRequest. Ensure that all calls to this function throughout the codebase have been updated to match the new parameter type. Also, verify that thecommon.RPCRequeststruct has the same fields and methods asaccountmanager.RPCRequestto avoid any runtime errors.tools/walletextension/accountmanager/account_manager.go (1)
- 164-170: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [133-282]
The
executeCallandsubmitCallfunctions remain unchanged, and they continue to handle regular RPC calls. ThesuggestAccountClientfunction also remains unchanged and continues to suggest the client to use for a regular RPC call based on the request parameters and the registered account clients. ThefindAccountInDataBytesfunction, which searches for an account in the data bytes of a request, also remains unchanged.integration/obscurogateway/obscurogateway_test.go (3)
7-12: The import of the
github.com/ethereum/go-ethereumpackage has been removed and replaced with more specific packages. This is a good practice as it reduces the overall import size and makes the dependencies more explicit. However, please ensure that all the necessary functions and types from thego-ethereumpackage are covered by the new imports.18-23: The import of the
github.com/ethereum/go-ethereumpackage has been removed. Please ensure that all the necessary functions and types from thego-ethereumpackage are covered by the other imports.59:
ThecreateObscuroNetworkfunction now takes an additional argument. Please ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- 107-353: This is a new test function
TestObscuroGatewaySubscriptionsWithMultipleAccounts. It seems to be testing the functionality of the Obscuro Gateway with multiple accounts. The test appears to be well-structured and covers a variety of scenarios including contract deployment, event subscription, and interaction with the smart contract. However, there are a few points to consider:
Error handling: There are several places where errors are checked and the test is stopped if an error occurs. This is good practice, but it might be beneficial to add more context to the error messages to make debugging easier if a test fails.
Magic numbers: There are several magic numbers used throughout the test (e.g.,
time.Sleep(5 * time.Second)). It might be beneficial to define these as constants at the top of the file or within the test function to make it easier to adjust these values if needed.Test cleanup: The test function seems to clean up by stopping the
obscuroGwContainer. This is good practice as it ensures that resources are freed after the test. However, it might be beneficial to usedeferto ensure that cleanup happens even if the test fails or panics at some point.Test assertions: The test function uses assertions to check the results of the operations. This is good practice as it ensures that the test will fail if the results are not as expected.
Overall, the test function seems to be well-written and covers a variety of scenarios. However, the points mentioned above could help improve the maintainability and robustness of the test.
+ const sleepDuration = 5 * time.Second + const amountToTransfer = 1_000_000_000_000_000_000 + const hardcodedMessageValue = "foo" + const user1MessageValue = "user1PrivateEvent" + const user2MessageValue = "user2PrivateEvent"Committable suggestion (Beta)
func TestObscuroGatewaySubscriptionsWithMultipleAccounts(t *testing.T) { // t.Skip("Commented it out until more testing is driven from this test") startPort := integration.StartPortObscuroGatewayUnitTest wallets := createObscuroNetwork(t, startPort, 5) obscuroGatewayConf := config.Config{ WalletExtensionHost: "127.0.0.1", WalletExtensionPortHTTP: startPort + integration.DefaultObscuroGatewayHTTPPortOffset, WalletExtensionPortWS: startPort + integration.DefaultObscuroGatewayWSPortOffset, NodeRPCHTTPAddress: fmt.Sprintf("127.0.0.1:%d", startPort+integration.DefaultHostRPCHTTPOffset), NodeRPCWebsocketAddress: fmt.Sprintf("127.0.0.1:%d", startPort+integration.DefaultHostRPCWSOffset), LogPath: "sys_out", VerboseFlag: false, DBType: "sqlite", } obscuroGwContainer := container.NewWalletExtensionContainerFromConfig(obscuroGatewayConf, testlog.Logger()) go func() { err := obscuroGwContainer.Start() if err != nil { t.Fatalf("error stopping WE - %s", err) } }() defer func() { err := obscuroGwContainer.Stop() if err != nil { t.Fatalf("error stopping WE - %s", err) } }() // wait for the msg bus contract to be deployed time.Sleep(sleepDuration) // make sure the server is ready to receive requests gatewayAddressHTTP := fmt.Sprintf("http://%s:%d", obscuroGatewayConf.WalletExtensionHost, obscuroGatewayConf.WalletExtensionPortHTTP) gatewayAddressWS := fmt.Sprintf("ws://%s:%d", obscuroGatewayConf.WalletExtensionHost, obscuroGatewayConf.WalletExtensionPortWS) fmt.Println("gatewayAddressHTTP: ", gatewayAddressHTTP) fmt.Println("gatewayAddressWS: ", gatewayAddressWS) // make sure the server is ready to receive requests err := waitServerIsReady(gatewayAddressHTTP) require.NoError(t, err) // Server is now ready and we can create requests // Create users user </blockquote></details></blockquote></details> </details>
| // HandleNewSubscriptions subscribes to an event with all the clients provided. | ||
| // Doing this is necessary because we have relevancy rule, and we want to subscribe sometimes with all clients to get all the events | ||
| func (sm *SubscriptionManager) HandleNewSubscriptions(clients []rpc.Client, req *wecommon.RPCRequest, resp *interface{}, userConn userconn.UserConn) error { | ||
| if len(req.Params) == 0 { | ||
| return fmt.Errorf("could not subscribe as no subscription namespace was provided") | ||
| } | ||
|
|
||
| sm.logger.Info(fmt.Sprintf("Subscribing to event %s with %d clients", req.Params, len(clients))) | ||
|
|
||
| // create a common channel for subscriptions from all clients | ||
| funnelMultipleAccountsChan := make(chan common.IDAndLog) | ||
|
|
||
| // read from a multiple accounts channel and write results to userConn | ||
| go readFromChannelAndWriteToUserConn(funnelMultipleAccountsChan, userConn, sm.logger) | ||
|
|
||
| // iterate over all clients and subscribe for each of them | ||
| for _, client := range clients { | ||
| // fmt.Println("Subscribing with client: ", client) | ||
| subscription, err := client.Subscribe(context.Background(), resp, rpc.SubscribeNamespace, funnelMultipleAccountsChan, req.Params...) | ||
| if err != nil { | ||
| return fmt.Errorf("could not call %s with params %v. Cause: %w", req.Method, req.Params, err) | ||
| } | ||
|
|
||
| // Add map subscriptionIDs | ||
| if currentNodeSubscriptionID, ok := (*resp).(string); ok { | ||
| // TODO (@ziga): Currently we use the same value for node and user subscriptionID - this will change after | ||
| // subscribing with multiple accounts | ||
| sm.UpdateSubscriptionMapping(currentNodeSubscriptionID, currentNodeSubscriptionID) | ||
| } | ||
|
|
||
| // We periodically check if the websocket is closed, and terminate the subscription. | ||
| go checkIfUserConnIsClosedAndUnsubscribe(userConn, subscription) | ||
|
|
||
| return nil | ||
| // TODO (@ziga) | ||
| // At this stage we want to use only the first account - same as before | ||
| // introduce subscribing with all accounts in another PR ) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The function HandleNewSubscriptions still returns immediately after the first successful subscription, ignoring the rest of the clients in the slice. This might not be the intended behavior considering the function's purpose and the comment on line 30. Consider iterating over all clients and subscribing each of them before returning.
| // Formats the log to be sent as an Eth JSON-RPC response. | ||
| // TODO (@ziga) - Move this code to a subscriptions package once it is used only there.. | ||
| func PrepareLogResponse(idAndLog common.IDAndLog) ([]byte, error) { | ||
| paramsMap := make(map[string]interface{}) | ||
| paramsMap[JSONKeySubscription] = idAndLog.SubID | ||
| paramsMap[JSONKeyResult] = idAndLog.Log | ||
|
|
||
| respMap := make(map[string]interface{}) | ||
| respMap[JSONKeyRPCVersion] = jsonrpc.Version | ||
| respMap[JSONKeyMethod] = methodEthSubscription | ||
| respMap[JSONKeyParams] = paramsMap | ||
|
|
||
| jsonResponse, err := json.Marshal(respMap) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not marshal log response to JSON. Cause: %w", err) | ||
| } | ||
| return jsonResponse, nil | ||
| } |
There was a problem hiding this comment.
The PrepareLogResponse function prepares a log to be sent as an Eth JSON-RPC response. The function seems to be well-structured and handles errors correctly. However, there is a TODO comment indicating that this function should be moved to a subscriptions package. It's important to follow up on this to ensure code organization and maintainability.
| // SPDX-License-Identifier: MIT | ||
| // Specify the Solidity version | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| contract SimpleMessageContract { | ||
|
|
||
| // State variable to store the message | ||
| string public message; | ||
| string public message2; | ||
|
|
||
| // Event declaration | ||
| event MessageUpdatedWithAddress(string newMessage, address indexed sender); | ||
| event Message2Updated(string newMessage); | ||
|
|
||
| // Constructor to initialize the message | ||
| constructor() { | ||
| message = "foo"; | ||
| message2 = "foo"; | ||
| } | ||
|
|
||
| // Function to set a new message | ||
| function setMessage(string memory newMessage) public { | ||
| message = newMessage; | ||
| emit MessageUpdatedWithAddress(newMessage, msg.sender); // Emit the event (only sender can see it) | ||
| } | ||
|
|
||
| function setMessage2(string memory newMessage) public { | ||
| message2 = newMessage; | ||
| emit Message2Updated(newMessage); // Emit the event (everyone can see it) | ||
| } | ||
| } |
There was a problem hiding this comment.
The previous comment about adding an onlyOwner modifier to restrict access to the setMessage and setMessage2 functions still stands. This is a security concern as currently, any address can call these functions and change the state variables message and message2. If you want to restrict this to only the contract owner or certain addresses, you should add a modifier to check the sender's address.
Here is the suggested change:
+ address public owner;
+
+ modifier onlyOwner {
+ require(msg.sender == owner, "Only the contract owner can call this function");
+ _;
+ }
+
constructor() {
+ owner = msg.sender;
message = "foo";
message2 = "foo";
}
function setMessage(string memory newMessage) public onlyOwner {
message = newMessage;
emit MessageUpdatedWithAddress(newMessage, msg.sender); // Emit the event (only sender can see it)
}
function setMessage2(string memory newMessage) public onlyOwner {
message2 = newMessage;
emit Message2Updated(newMessage); // Emit the event (everyone can see it)
}Committable suggestion (Beta)
| // SPDX-License-Identifier: MIT | |
| // Specify the Solidity version | |
| pragma solidity ^0.8.0; | |
| contract SimpleMessageContract { | |
| // State variable to store the message | |
| string public message; | |
| string public message2; | |
| // Event declaration | |
| event MessageUpdatedWithAddress(string newMessage, address indexed sender); | |
| event Message2Updated(string newMessage); | |
| // Constructor to initialize the message | |
| constructor() { | |
| message = "foo"; | |
| message2 = "foo"; | |
| } | |
| // Function to set a new message | |
| function setMessage(string memory newMessage) public { | |
| message = newMessage; | |
| emit MessageUpdatedWithAddress(newMessage, msg.sender); // Emit the event (only sender can see it) | |
| } | |
| function setMessage2(string memory newMessage) public { | |
| message2 = newMessage; | |
| emit Message2Updated(newMessage); // Emit the event (everyone can see it) | |
| } | |
| } | |
| // SPDX-License-Identifier: MIT | |
| // Specify the Solidity version | |
| pragma solidity ^0.8.0; | |
| contract SimpleMessageContract { | |
| // State variable to store the message | |
| string public message; | |
| string public message2; | |
| // Owner of the contract | |
| address public owner; | |
| // Event declaration | |
| event MessageUpdatedWithAddress(string newMessage, address indexed sender); | |
| event Message2Updated(string newMessage); | |
| // Modifier to restrict access to owner only | |
| modifier onlyOwner { | |
| require(msg.sender == owner, "Only the contract owner can call this function"); | |
| _; | |
| } | |
| // Constructor to initialize the message | |
| constructor() { | |
| owner = msg.sender; | |
| message = "foo"; | |
| message2 = "foo"; | |
| } | |
| // Function to set a new message | |
| function setMessage(string memory newMessage) public onlyOwner { | |
| message = newMessage; | |
| emit MessageUpdatedWithAddress(newMessage, msg.sender); // Emit the event (only sender can see it) | |
| } | |
| function setMessage2(string memory newMessage) public onlyOwner { | |
| message2 = newMessage; | |
| emit Message2Updated(newMessage); // Emit the event (everyone can see it) | |
| } | |
| } |
| func joinObscuroGateway(url string) (string, error) { | ||
| statusCode, userID, err := fasthttp.Get(nil, fmt.Sprintf("%s/v1/join/", url)) | ||
| if err != nil || statusCode != 200 { | ||
| return "", fmt.Errorf(fmt.Sprintf("Failed to get userID. Status code: %d, err: %s", statusCode, err)) | ||
| } | ||
| return string(userID), nil | ||
| } |
There was a problem hiding this comment.
The joinObscuroGateway function is not handling non-200 status codes correctly. If the status code is not 200, it will still attempt to return the user ID, which could lead to unexpected behavior. Consider handling non-200 status codes separately.
- if err != nil || statusCode != 200 {
- return "", fmt.Errorf(fmt.Sprintf("Failed to get userID. Status code: %d, err: %s", statusCode, err))
- }
- return string(userID), nil
+ if err != nil {
+ return "", fmt.Errorf("Failed to get userID: %v", err)
+ }
+ if statusCode != 200 {
+ return "", fmt.Errorf("Failed to get userID. Status code: %d", statusCode)
+ }
+ return string(userID), nil
zkokelj
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration/obscurogateway/obscurogateway_test.go (6 hunks)
- tools/walletextension/accountmanager/account_manager.go (8 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
Additional comments: 10
integration/obscurogateway/obscurogateway_test.go (3)
13-21: The import statements have been reorganized and additional packages have been imported. Ensure that these new packages are being used in the code and are necessary for the functionality. Unused imports can lead to unnecessary dependencies and can make the code harder to maintain.
60-60: The function
createObscuroNetworknow takes an additional argument1. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.535-538: The function
createObscuroNetworknow takes an additional argumentnrSimWallets. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.tools/walletextension/subscriptions/subscriptions.go (1)
- 88-96: The function
checkIfUserConnIsClosedAndUnsubscribestill continuously checks if the user connection is closed in a loop with a sleep of 100 milliseconds. This could lead to unnecessary CPU usage. Consider using a more efficient method to detect closed connections, such as using a channel to signal when the connection is closed.tools/walletextension/accountmanager/account_manager.go (6)
4-11: The import of the
subscriptionspackage fromgithub.com/obscuronet/go-obscuro/tools/walletextension/subscriptionsis new. This package is used for managing subscriptions in theAccountManagerstruct. The import of thecontextandtimepackages has been removed, indicating that these packages are no longer used in this file.45-50: The
NewAccountManagerfunction has been updated to initialize thesubscriptionsManagerfield in theAccountManagerstruct. This change allows theAccountManagerto manage subscriptions across multiple clients. The initialization of thesubscriptionsManagerfield is done using thesubscriptions.New(logger)function.134-136: No significant changes in these lines. The
executeCallfunction is still acquiring a read lock onaccountsMutexbefore suggesting an account client for the RPC request.165-171: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [168-172]
No significant changes in these lines. The
suggestAccountClientfunction is still returning the first (and only) client if there is only one client inaccClients.
- 280-286: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [283-287]
No significant changes in these lines. The
submitCallfunction is still cloning the request if the method isrpc.Callorrpc.EstimateGasto avoid modifying the original request.
- 336-338: No significant changes in these lines. The
parseRequestfunction is still returning the parsed request.
| }, | ||
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "string", | ||
| "name": "newMessage", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "setMessage", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, | ||
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "string", | ||
| "name": "newMessage", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "setMessage2", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| } | ||
| ] | ||
| ` | ||
|
|
||
| _, contractAddress, err := DeploySmartContract(user0.HTTPClient, user0.Wallets[0], bytecode) | ||
| require.NoError(t, err) | ||
| fmt.Println("Deployed contract address: ", contractAddress) | ||
|
|
||
| // contract abi | ||
| contractAbi, err := abi.JSON(strings.NewReader(abiString)) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if contract was deployed and call one of the implicit getter functions | ||
| // call getter for a message | ||
| resultMessage, err := getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| time.Sleep(time.Second) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "user1PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage", "user1PrivateEvent", contractAddress) | ||
| require.NoError(t, err) | ||
| _, err = InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], contractAbi, "setMessage2", "user1PublicEvent", contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "user2PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage", "user2PrivateEvent", contractAddress) | ||
| require.NoError(t, err) | ||
| _, err = InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], contractAbi, "setMessage2", "user2PublicEvent", contractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractAddress, contractAbi, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // 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 setMessage2) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 3, len(user1logs)) | ||
| // user2 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 2, len(user2logs)) | ||
|
|
||
| // Gracefully shutdown | ||
| err = obscuroGwContainer.Stop() | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
This is a new test function TestObscuroGatewaySubscriptionsWithMultipleAccounts. It seems to be testing the functionality of the Obscuro Gateway with multiple accounts. The test seems to be well-structured and covers a lot of different scenarios. However, it's a very long function which makes it hard to understand and maintain. Consider breaking it down into smaller, more manageable functions. This will make the code easier to read and debug, and it will also make it easier to reuse code in the future.
| func (m *AccountManager) ProxyRequest(rpcReq *wecommon.RPCRequest, rpcResp *interface{}, userConn userconn.UserConn) error { | ||
| if rpcReq.Method == rpc.Subscribe { | ||
| clients, err := m.suggestSubscriptionClient(rpcReq) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // fetch the clients from a topic | ||
| for _, client := range clients { | ||
| return m.executeSubscribe(client, rpcReq, rpcResp, userConn) | ||
| err = m.subscriptionsManager.HandleNewSubscriptions(clients, rpcReq, rpcResp, userConn) | ||
| if err != nil { | ||
| m.logger.Error("Error subscribing to multiple clients") | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| return m.executeCall(rpcReq, rpcResp) | ||
| } |
There was a problem hiding this comment.
The ProxyRequest function has been updated to handle subscriptions using the new subscriptionsManager. If the request method is rpc.Subscribe, the function suggests a client for the subscription, handles new subscriptions, and logs any errors that occur during this process. This change allows the AccountManager to manage subscriptions across multiple clients. However, the error message logged at line 71 is generic and does not provide specific details about the error. It would be more helpful to include the actual error message in the log.
- m.logger.Error("Error subscribing to multiple clients")
+ m.logger.Error("Error subscribing to multiple clients: ", err)Committable suggestion (Beta)
| func (m *AccountManager) ProxyRequest(rpcReq *wecommon.RPCRequest, rpcResp *interface{}, userConn userconn.UserConn) error { | |
| if rpcReq.Method == rpc.Subscribe { | |
| clients, err := m.suggestSubscriptionClient(rpcReq) | |
| if err != nil { | |
| return err | |
| } | |
| // fetch the clients from a topic | |
| for _, client := range clients { | |
| return m.executeSubscribe(client, rpcReq, rpcResp, userConn) | |
| err = m.subscriptionsManager.HandleNewSubscriptions(clients, rpcReq, rpcResp, userConn) | |
| if err != nil { | |
| m.logger.Error("Error subscribing to multiple clients") | |
| } | |
| return err | |
| } | |
| return m.executeCall(rpcReq, rpcResp) | |
| } | |
| func (m *AccountManager) ProxyRequest(rpcReq *wecommon.RPCRequest, rpcResp *interface{}, userConn userconn.UserConn) error { | |
| if rpcReq.Method == rpc.Subscribe { | |
| clients, err := m.suggestSubscriptionClient(rpcReq) | |
| if err != nil { | |
| return err | |
| } | |
| err = m.subscriptionsManager.HandleNewSubscriptions(clients, rpcReq, rpcResp, userConn) | |
| if err != nil { | |
| m.logger.Error("Error subscribing to multiple clients: ", err) | |
| } | |
| return err | |
| } | |
| return m.executeCall(rpcReq, rpcResp) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files selected for processing (5)
- integration/common/utils.go (2 hunks)
- integration/obscurogateway/events_contract.go (1 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
- tools/walletextension/subscriptions/subscriptions.go (1 hunks)
Additional comments: 6
integration/common/utils.go (1)
- 7-7: The import
github.com/ethereum/go-ethereum/accounts/abihas been added. Ensure that this package is used in the code and that it is compatible with the rest of the codebase.integration/obscurogateway/obscurogateway_test.go (5)
13-20: The new hunk imports additional packages that are necessary for the new functionality introduced in this PR. These packages are related to Ethereum accounts and bindings, and Ethereum's crypto and RLP (Recursive Length Prefix) encoding. The imports look fine.
94-96: The new hunk comments out the execution of some tests and adds a new test
testMultipleAccountsSubscription. This is likely done to focus on the new functionality being introduced and avoid interference from other tests. However, it's important to ensure that all tests are eventually re-enabled and pass to ensure the overall functionality of the system is intact.446-458: This new hunk introduces a function
ComputeContractAddressthat computes the Ethereum contract address for a given sender address and nonce. The function uses RLP encoding and Keccak-256 hashing, which are standard techniques in Ethereum. The function looks fine.460-478: This new hunk introduces a function
TransferETHToAddressthat transfers a certain amount of Ether from a wallet to a given address. The function creates a transaction, signs it, sends it, and waits for the receipt. The function looks fine.480-492: This new hunk introduces a function
getStringValueFromSmartContractGetterthat gets a string value from a smart contract getter method. The function uses thebindpackage to interact with the smart contract. The function looks fine.
| if len(req.Params) == 0 { | ||
| return fmt.Errorf("could not subscribe as no subscription namespace was provided") | ||
| } |
There was a problem hiding this comment.
The function HandleNewSubscriptions now checks if req.Params is empty but does not handle the case where req.Params is nil. This could lead to a panic if req.Params is nil. Consider adding a nil check for req.Params.
if len(req.Params) == 0 {
+ if req.Params == nil {
+ return fmt.Errorf("could not subscribe as subscription namespace is nil")
}
return fmt.Errorf("could not subscribe as no subscription namespace was provided")
}| func checkIfUserConnIsClosedAndUnsubscribe(userConn userconn.UserConn, subscription *gethrpc.ClientSubscription) { | ||
| for { | ||
| if userConn.IsClosed() { | ||
| subscription.Unsubscribe() | ||
| return | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function checkIfUserConnIsClosedAndUnsubscribe still continuously checks if the user connection is closed in a loop with a sleep of 100 milliseconds. This could lead to unnecessary CPU usage. Consider using a more efficient method to detect closed connections, such as using a channel to signal when the connection is closed.
| func (sm *SubscriptionManager) UpdateSubscriptionMapping(userSubscriptionID string, obscuroNodeSubscriptionID string) { | ||
| existingUserIDs, exists := sm.subscriptionMappings[userSubscriptionID] | ||
|
|
||
| if !exists { | ||
| sm.subscriptionMappings[userSubscriptionID] = []string{obscuroNodeSubscriptionID} | ||
| return | ||
| } | ||
|
|
||
| // Check if obscuroNodeSubscriptionID already exists to avoid duplication | ||
| alreadyExists := false | ||
| for _, existingID := range existingUserIDs { | ||
| if obscuroNodeSubscriptionID == existingID { | ||
| alreadyExists = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !alreadyExists { | ||
| sm.subscriptionMappings[userSubscriptionID] = append(existingUserIDs, obscuroNodeSubscriptionID) | ||
| } | ||
| } |
There was a problem hiding this comment.
The function UpdateSubscriptionMapping still does not handle the case where userSubscriptionID or obscuroNodeSubscriptionID is empty. This could lead to incorrect mappings. Consider adding checks for empty strings.
+ if userSubscriptionID == "" || obscuroNodeSubscriptionID == "" {
+ return fmt.Errorf("userSubscriptionID or obscuroNodeSubscriptionID cannot be empty")
+ }
existingUserIDs, exists := sm.subscriptionMappings[userSubscriptionID]| // automatically join OG | ||
| userID, err := joinObscuroGateway(serverAddressHTTP) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // create clients | ||
| httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + userID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + userID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &GatewayUser{ | ||
| UserID: userID, | ||
| Wallets: wallets, | ||
| HTTPClient: httpClient, | ||
| WSClient: wsClient, | ||
| ServerAddressHTTP: serverAddressHTTP, | ||
| ServerAddressWS: serverAddressWS, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The NewUser function creates HTTP and WebSocket clients for the user. However, it does not check if the server addresses provided are valid URLs. This could lead to unexpected behavior or errors when trying to dial the server. Consider validating the server addresses before using them.
+ _, err := url.ParseRequestURI(serverAddressHTTP)
+ if err != nil {
+ return nil, fmt.Errorf("invalid HTTP server address: %v", err)
+ }
+ _, err = url.ParseRequestURI(serverAddressWS)
+ if err != nil {
+ return nil, fmt.Errorf("invalid WebSocket server address: %v", err)
+ }| message := fmt.Sprintf("Register %s for %s", userID, strings.ToLower(hexAddress)) | ||
| prefixedMessage := fmt.Sprintf("\u0019Ethereum Signed Message:\n%d%s", len(message), message) | ||
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | ||
| sig, err := crypto.Sign(messageHash, pk) | ||
| if err != nil { | ||
| fmt.Printf("Failed to sign message: %v\n", err) | ||
| } | ||
| sig[64] += 27 | ||
| signature := "0x" + hex.EncodeToString(sig) | ||
| payload := fmt.Sprintf("{\"signature\": \"%s\", \"message\": \"%s\"}", signature, message) | ||
| return payload | ||
| } | ||
|
|
There was a problem hiding this comment.
The prepareRegisterPayload function does not handle the error from crypto.Sign. If an error occurs during signing, the function will continue to execute and may produce an invalid signature. Consider handling this error.
- if err != nil {
- fmt.Printf("Failed to sign message: %v\n", err)
- }
+ if err != nil {
+ return "", fmt.Errorf("failed to sign message: %v", err)
+ }| require.NoError(t, err) | ||
|
|
||
| var amountToTransfer int64 = 1_000_000_000_000_000_000 | ||
| // Transfer some funds to user1 and user2 wallets, because they need it 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) | ||
| _, err = TransferETHToAddress(user0.HTTPClient, user0.Wallets[0], user2.Wallets[0].Address(), amountToTransfer) | ||
| require.NoError(t, err) | ||
| _, err = TransferETHToAddress(user0.HTTPClient, user0.Wallets[0], user2.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) | ||
| err = user2.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
|
|
||
| // deploy errors contract | ||
| //_, contractAddress, err := deploySmartContract(user0.HTTPClient, user0.Wallets[0], eventsContractBytecode) | ||
| //require.NoError(t, err) | ||
| //fmt.Println("Deployed contract address: ", contractAddress) | ||
|
|
||
| // 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) | ||
|
|
||
| resultMessage, err := getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "user1PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage", "user1PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage2", "user1PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "user2PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage", "user2PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage2", "user2PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // 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 setMessage2) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 3, len(user1logs)) | ||
| // user2 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 2, len(user2logs)) | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
This new hunk introduces a new test function testMultipleAccountsSubscription. The test creates three users, each with multiple wallets. It transfers funds to these wallets, deploys a smart contract, and interacts with it to emit events. The test then checks if the correct number of events are received by each user. The test seems to be well-structured and covers the new functionality introduced in this PR. However, there are a few points that could be improved:
-
The test function is quite long and does a lot of things. It might be beneficial to break it down into smaller, more manageable functions. This would improve readability and maintainability.
-
The test function uses a lot of magic numbers (e.g.,
amountToTransfer,Gas,GasPrice,FromBlock,ToBlock). It would be better to define these as constants at the top of the file or pass them as parameters to the function. -
The test function uses
time.Sleepto wait for certain operations to complete. This could lead to flaky tests if the operations take longer than expected. It would be better to use some form of synchronization or polling to wait for these operations. -
The test function uses
fmt.Printffor logging. It would be better to use a proper logging framework that supports different log levels and formats. -
The test function uses
assertfor checking conditions. While this is fine for most cases, it might be better to userequirefor conditions that must be true for the test to continue. This would stop the test immediately when such a condition fails, avoiding potential panics or confusing errors later in the test. -
The test function does not check the return values of
subscribeToEvents. It would be good to check these to ensure that the subscriptions were successful. -
The test function does not check the contents of the logs received by each user. It would be good to check these to ensure that the correct events were received.
| //// SPDX-License-Identifier: MIT | ||
| //pragma solidity ^0.8.0; | ||
| // | ||
| //contract SimpleMessageContract { | ||
| // | ||
| //// State variable to store the message | ||
| //string public message; | ||
| //string public message2; | ||
| // | ||
| //// Event declaration | ||
| //event MessageUpdatedWithAddress(string newMessage, address indexed sender); | ||
| //event Message2Updated(string newMessage); | ||
| // | ||
| //// Constructor to initialize the message | ||
| //constructor() { | ||
| //message = "foo"; | ||
| //message2 = "foo"; | ||
| //} | ||
| // | ||
| //// Function to set a new message | ||
| //function setMessage(string memory newMessage) public { | ||
| //message = newMessage; | ||
| //emit MessageUpdatedWithAddress(newMessage, msg.sender); // Emit the event (only sender can see it) | ||
| //} | ||
| // | ||
| //function setMessage2(string memory newMessage) public { | ||
| //message2 = newMessage; | ||
| //emit Message2Updated(newMessage); // Emit the event (everyone can see it) | ||
| //} | ||
| //} |
There was a problem hiding this comment.
The commented out code should be removed. It's generally not a good practice to leave large blocks of commented out code in the codebase. If this code is not being used, it should be removed. If it's needed for future reference, it should be documented elsewhere (e.g., in a design document or a separate branch).
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration/common/utils.go (2 hunks)
- integration/obscurogateway/gateway_user.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (4 hunks)
Additional comments: 6
integration/common/utils.go (2)
12-13: The import
github.com/ethereum/go-ethereum/accounts/abiis added but it's not clear why it's needed in this context. Please verify if this import is necessary.134-162: The function
InteractWithSmartContractis added. It interacts with a smart contract by packing the method name and parameters into data, creating a transaction, signing it, sending it, and waiting for the receipt. The function seems to be well-structured and handles errors properly. However, the gas price is hardcoded to 1 which might not be optimal in all network conditions. Consider using a dynamic gas price strategy.- GasPrice: gethcommon.Big1, + GasPrice: gethcommon.Big2, // or any other dynamic gas price strategyCommittable suggestion (Beta)
func InteractWithSmartContract(client *ethclient.Client, wallet wallet.Wallet, contractAbi abi.ABI, methodName string, methodParam string, contractAddress gethcommon.Address) (*types.Receipt, error) { contractInteractionData, err := contractAbi.Pack(methodName, methodParam) if err != nil { return nil, err } interactionTx := types.LegacyTx{ Nonce: wallet.GetNonceAndIncrement(), To: &contractAddress, Gas: uint64(1_000_000), GasPrice: gethcommon.Big2, // or any other dynamic gas price strategy Data: contractInteractionData, } signedTx, err := wallet.SignTransaction(&interactionTx) if err != nil { return nil, err } err = client.SendTransaction(context.Background(), signedTx) if err != nil { return nil, err } txReceipt, err := AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 2*time.Second) if err != nil { return nil, err } return txReceipt, nil }integration/obscurogateway/obscurogateway_test.go (4)
13-20: The new hunk introduces additional imports for Ethereum accounts and cryptography. These are likely used for the new functionalities introduced in this PR, such as handling multiple accounts and deploying smart contracts. Ensure that these new imports are used in the code and are necessary.
94-96: A new test function
testMultipleAccountsSubscriptionis added to the test suite. This test is likely to verify the new functionality of handling multiple account subscriptions.446-457: The new hunk introduces the
ComputeContractAddressfunction. This function computes the address of a contract by RLP encoding the sender's address and nonce, computing the Keccak-256 hash of the encoded byte array, and returning the last 20 bytes of the hash as the contract address. This function seems to be correctly implemented according to the Ethereum contract address computation rules.479-491: The new hunk introduces the
getStringValueFromSmartContractGetterfunction. This function gets a string value from a smart contract getter method. It binds the contract, creates a call options object, makes the call, and returns the result. The function seems to be correctly implemented according to the Ethereum contract interaction rules.
| // GatewayUser TODO (@ziga) refactor GatewayUser and integrate it with OGlib. | ||
| // GatewayUser is a struct that includes everything a gateway user has and uses (userID, wallets, http & ws addresses and client ) | ||
| type GatewayUser struct { | ||
| UserID string | ||
| Wallets []wallet.Wallet | ||
| HTTPClient *ethclient.Client | ||
| WSClient *ethclient.Client | ||
| ServerAddressHTTP string | ||
| ServerAddressWS string | ||
| } |
There was a problem hiding this comment.
The GatewayUser struct is well defined with all necessary fields for a user. However, it's not clear why both HTTP and WebSocket clients are needed. If they serve different purposes, consider adding comments to clarify their roles. If they serve the same purpose, consider using only one client to reduce complexity.
| func NewUser(wallets []wallet.Wallet, serverAddressHTTP string, serverAddressWS string) (*GatewayUser, error) { | ||
| // automatically join OG | ||
| userID, err := joinObscuroGateway(serverAddressHTTP) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // create clients | ||
| httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + userID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + userID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &GatewayUser{ | ||
| UserID: userID, | ||
| Wallets: wallets, | ||
| HTTPClient: httpClient, | ||
| WSClient: wsClient, | ||
| ServerAddressHTTP: serverAddressHTTP, | ||
| ServerAddressWS: serverAddressWS, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The NewUser function creates a new user and initializes the HTTP and WebSocket clients. However, it does not validate the server addresses before using them. This could lead to unexpected behavior or errors when trying to dial the server. Consider validating the server addresses before using them.
+ _, err := url.ParseRequestURI(serverAddressHTTP)
+ if err != nil {
+ return nil, fmt.Errorf("invalid HTTP server address: %v", err)
+ }
+ _, err = url.ParseRequestURI(serverAddressWS)
+ if err != nil {
+ return nil, fmt.Errorf("invalid WebSocket server address: %v", err)
+ }| func (u GatewayUser) RegisterAccounts() error { | ||
| for _, w := range u.Wallets { | ||
| response, err := registerAccount(u.ServerAddressHTTP, u.UserID, w.PrivateKey(), w.Address().Hex()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("Successfully registered address %s for user: %s with response: %s \n", w.Address().Hex(), u.UserID, response) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The RegisterAccounts function registers each wallet in the user's wallet list. However, if the registration of one account fails, the function returns an error immediately and does not attempt to register the remaining accounts. Consider continuing to register the remaining accounts and returning a list of errors instead.
+ var errs []error
+ for _, w := range u.Wallets {
+ response, err := registerAccount(u.ServerAddressHTTP, u.UserID, w.PrivateKey(), w.Address().Hex())
+ if err != nil {
+ errs = append(errs, err)
+ continue
+ }
+ fmt.Printf("Successfully registered address %s for user: %s with response: %s \n", w.Address().Hex(), u.UserID, response)
+ }
+ if len(errs) > 0 {
+ return fmt.Errorf("errors occurred during registration: %v", errs)
+ }
+ return nil| func registerAccount(url string, userID string, pk *ecdsa.PrivateKey, hexAddress string) ([]byte, error) { | ||
| payload := prepareRegisterPayload(userID, pk, hexAddress) | ||
|
|
||
| req, err := http.NewRequestWithContext( | ||
| context.Background(), | ||
| http.MethodPost, | ||
| url+"/v1/authenticate/?u="+userID, | ||
| strings.NewReader(payload), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json; charset=UTF-8") | ||
|
|
||
| client := &http.Client{} | ||
| response, err := client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| defer response.Body.Close() | ||
| return io.ReadAll(response.Body) | ||
| } |
There was a problem hiding this comment.
The registerAccount function creates a new HTTP client for each registration request. This could lead to a large number of idle connections if many accounts are being registered. Consider reusing a single HTTP client for all requests.
- client := &http.Client{}
+ client := http.DefaultClient| func prepareRegisterPayload(userID string, pk *ecdsa.PrivateKey, hexAddress string) string { | ||
| message := fmt.Sprintf("Register %s for %s", userID, strings.ToLower(hexAddress)) | ||
| prefixedMessage := fmt.Sprintf("\u0019Ethereum Signed Message:\n%d%s", len(message), message) | ||
| messageHash := crypto.Keccak256([]byte(prefixedMessage)) | ||
| sig, err := crypto.Sign(messageHash, pk) | ||
| if err != nil { | ||
| fmt.Printf("Failed to sign message: %v\n", err) | ||
| } | ||
| sig[64] += 27 | ||
| signature := "0x" + hex.EncodeToString(sig) | ||
| payload := fmt.Sprintf("{\"signature\": \"%s\", \"message\": \"%s\"}", signature, message) | ||
| return payload | ||
| } |
There was a problem hiding this comment.
The prepareRegisterPayload function does not handle the error from crypto.Sign. If an error occurs during signing, the function will continue to execute and may produce an invalid signature. Consider handling this error.
- if err != nil {
- fmt.Printf("Failed to sign message: %v\n", err)
- }
+ if err != nil {
+ return "", fmt.Errorf("failed to sign message: %v", err)
+ }| require.NoError(t, err) | ||
| err = user2.RegisterAccounts() | ||
| require.NoError(t, err) | ||
|
|
||
| var amountToTransfer int64 = 1_000_000_000_000_000_000 | ||
| // Transfer some funds to user1 and user2 wallets, because they need it 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) | ||
| _, err = TransferETHToAddress(user0.HTTPClient, user0.Wallets[0], user2.Wallets[0].Address(), amountToTransfer) | ||
| require.NoError(t, err) | ||
| _, err = TransferETHToAddress(user0.HTTPClient, user0.Wallets[0], user2.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) | ||
| err = user2.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
|
|
||
| // deploy errors contract | ||
| //_, contractAddress, err := deploySmartContract(user0.HTTPClient, user0.Wallets[0], eventsContractBytecode) | ||
| //require.NoError(t, err) | ||
| //fmt.Println("Deployed contract address: ", contractAddress) | ||
|
|
||
| // 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) | ||
|
|
||
| resultMessage, err := getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "user1PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage", "user1PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage2", "user1PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "user2PrivateEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage", "user2PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage2", "user2PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // 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 setMessage2) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 3, len(user1logs)) | ||
| // user2 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 2, len(user2logs)) | ||
| } |
There was a problem hiding this comment.
The new hunk introduces the testMultipleAccountsSubscription function. This function tests the functionality of handling multiple account subscriptions. It creates multiple users, each with multiple wallets, registers the accounts, transfers funds between them, deploys a smart contract, interacts with it, and subscribes to events. The function seems to cover a lot of different aspects of the new functionality and should provide a good test of the system. However, there are a few points that could be improved:
-
The function uses
fmt.Printffor logging. It would be better to use a proper logging framework that supports different log levels and formats. -
The function uses a lot of magic numbers (e.g.,
amountToTransfer,Gas,GasPrice). It would be better to define these as constants at the top of the file or pass them as parameters to the function. -
The function does not check the return values of the
TransferETHToAddresscalls. It would be good to check these to ensure that the transfers were successful. -
The function does not check the return values of the
InteractWithSmartContractcalls. It would be good to check these to ensure that the interactions were successful. -
The function does not check the return values of the
subscribeToEventscalls. It would be good to check these to ensure that the subscriptions were successful. -
The function does not handle errors from the
subscribeToEventscalls. It would be good to add error handling for these calls. -
The function does not handle the case where the
subscribeToEventscalls return an error. It would be good to add error handling for this case. -
The function does not handle the case where the
InteractWithSmartContractcalls return an error. It would be good to add error handling for this case. -
The function does not handle the case where the
TransferETHToAddresscalls return an error. It would be good to add error handling for this case. -
The function does not handle the case where the
getStringValueFromSmartContractGettercalls return an error. It would be good to add error handling for this case.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/obscurogateway/obscurogateway_test.go (7} hunks)
Additional comments: 5
integration/obscurogateway/obscurogateway_test.go (5)
13-17: The new hunk introduces additional imports from the
github.com/ethereum/go-ethereumpackage. These imports aregithub.com/ethereum/go-ethereum/accounts/abiandgithub.com/ethereum/go-ethereum/accounts/abi/bind. These packages are used for interacting with Ethereum smart contracts, which is consistent with the changes described in the PR summary.88-100: The new hunk modifies the test function signatures in the test map to include a
wallet.Walletparameter. This change is consistent with the PR summary, which mentions updates to user management and subscription logic. The new test functiontestMultipleAccountsSubscriptionis also added to the test map.108-219: The new hunk introduces the
testMultipleAccountsSubscriptionfunction. This function tests the subscription logic with multiple accounts. It creates three users, each with different wallets, registers their accounts, transfers funds to them, deploys a smart contract, and then tests the subscription logic by interacting with the smart contract and checking the received events. The function seems to be correctly implemented according to the Ethereum transaction rules and the PR summary.255-260: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [249-260]
The new hunk modifies the
testErrorHandlingfunction signature to include awallet.Walletparameter. This change is consistent with the PR summary, which mentions updates to user management and subscription logic. The function now also registers an account using the provided wallet.
- 288-301: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [291-302]
The new hunk modifies the
testErrorsRevertedArePassedfunction signature to include awallet.Walletparameter. This change is consistent with the PR summary, which mentions updates to user management and subscription logic. The function now also registers an account using the provided wallet.
| func TransferETHToAddress(client *ethclient.Client, wallet wallet.Wallet, toAddress gethcommon.Address, amount int64) (*types.Receipt, error) { | ||
| transferTx1 := types.LegacyTx{ | ||
| Nonce: wallet.GetNonceAndIncrement(), | ||
| To: &toAddress, | ||
| Value: big.NewInt(amount), | ||
| Gas: uint64(1_000_000), | ||
| GasPrice: gethcommon.Big1, | ||
| Data: nil, | ||
| } | ||
| signedTx, err := wallet.SignTransaction(&transferTx1) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = client.SendTransaction(context.Background(), signedTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return integrationCommon.AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 2*time.Second) | ||
| } |
There was a problem hiding this comment.
The new hunk introduces the TransferETHToAddress function. This function transfers a specified amount of ETH from a wallet to an address. It creates a transaction, signs it, sends it, and waits for the receipt. The function seems to be correctly implemented according to the Ethereum transaction rules. However, it would be better to define the gas limit and gas price as constants at the top of the file or pass them as parameters to the function.
|
|
||
| // TODO (@ziga) - use OGlib for registering accounts | ||
| func registerAccount(url string, userID string, pk *ecdsa.PrivateKey, hexAddress string) ([]byte, error) { | ||
| payload := prepareRegisterPayload(userID, pk, hexAddress) |
There was a problem hiding this comment.
Let's use the oglib here too.
| func joinObscuroGateway(url string) (string, error) { | ||
| ogClient := lib.NewObscuroGatewayLibrary(url, url) | ||
| err := ogClient.Join() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| return ogClient.UserID(), nil | ||
| } |
There was a problem hiding this comment.
Any need for this func ?
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| resultMessage, err = getStringValueFromSmartContractGetter(contractReceipt.ContractAddress, eventsContractABI, "message", user1.HTTPClient) |
There was a problem hiding this comment.
Can we use the same approach as the errors test ?
Something in the lines of :
pack, _ := errorsContractABI.Pack("force_require")
_, err = ethStdClient.CallContract(context.Background(), ethereum.CallMsg{
From: w.Address(),
To: &receipt.ContractAddress,
Data: pack,
}, nil)
```
| // prefunded wallet | ||
| w := wallet.NewInMemoryWalletFromConfig(genesis.TestnetPrefundedPK, integration.ObscuroChainID, testlog.Logger()) | ||
|
|
||
| // run the tests against the exis | ||
| for name, test := range map[string]func(*testing.T, string, string){ | ||
| for name, test := range map[string]func(*testing.T, string, string, wallet.Wallet){ |
There was a problem hiding this comment.
The wallet is known, there's no need to pass it as an argument. Have a look at testAreTxsMinted roughly line 110.
There was a problem hiding this comment.
if we create wallet instance in each test tests don't work as expected. (I assume nonce is not correct). Each test separately was ok. (I even tried changing the order and it didn't help).
There was a problem hiding this comment.
Makes sense 👍 we need to find a different funding approach, but something for the future perhaps.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/gateway_user.go (1} hunks)
- integration/obscurogateway/obscurogateway_test.go (8} hunks)
Additional comments: 7
integration/obscurogateway/gateway_user.go (1)
- 55-64: As per the previous comment, the
PrintUserAccountsBalancesfunction is directly printing the balance of each account to the console. This might not be desirable in a production environment. Consider returning the balances to the caller instead, so they can decide how to handle it.integration/obscurogateway/obscurogateway_test.go (6)
- 11-19: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [4-16]
The import of
"bytes"is new in this hunk. Ensure that it is used in the code and is not an unnecessary import.
87-96: The function signature for the test functions has been changed to include a
wallet.Walletparameter. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.242-249: The function signature for
testAreTxsMintedhas been changed to include awallet.Walletparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.266-272: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [269-276]
The function signature for
testErrorHandlinghas been changed to include awallet.Walletparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
311-318: The function signature for
testErrorsRevertedArePassedhas been changed to include awallet.Walletparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.458-476: The new function
transferETHToAddressis introduced. It transfers a specified amount of ETH from one address to another. The function creates a transaction, signs it, sends it, and waits for the receipt. The function seems to be well-structured and logically sound.
| func NewUser(wallets []wallet.Wallet, serverAddressHTTP string, serverAddressWS string) (*GatewayUser, error) { | ||
| ogClient := lib.NewObscuroGatewayLibrary(serverAddressHTTP, serverAddressWS) | ||
|
|
||
| // automatically join | ||
| ogClient.Join() | ||
|
|
||
| // create clients | ||
| httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &GatewayUser{ | ||
| Wallets: wallets, | ||
| HTTPClient: httpClient, | ||
| WSClient: wsClient, | ||
| ogClient: ogClient, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The NewUser function creates an HTTP and a WebSocket client by dialing the server addresses. However, there is no error handling or retry logic in case the dialing fails. This could lead to a situation where the function returns a GatewayUser with a nil HTTP or WebSocket client, which could cause runtime errors later on. Consider adding retry logic and/or returning an error if the dialing fails.
- httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID())
- if err != nil {
- return nil, err
- }
- wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID())
- if err != nil {
- return nil, err
- }
+ httpClient, err := dialWithRetry(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID())
+ if err != nil {
+ return nil, err
+ }
+ wsClient, err := dialWithRetry(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID())
+ if err != nil {
+ return nil, err
+ }Committable suggestion (Beta)
| func NewUser(wallets []wallet.Wallet, serverAddressHTTP string, serverAddressWS string) (*GatewayUser, error) { | |
| ogClient := lib.NewObscuroGatewayLibrary(serverAddressHTTP, serverAddressWS) | |
| // automatically join | |
| ogClient.Join() | |
| // create clients | |
| httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &GatewayUser{ | |
| Wallets: wallets, | |
| HTTPClient: httpClient, | |
| WSClient: wsClient, | |
| ogClient: ogClient, | |
| }, nil | |
| } | |
| func NewUser(wallets []wallet.Wallet, serverAddressHTTP string, serverAddressWS string) (*GatewayUser, error) { | |
| ogClient := lib.NewObscuroGatewayLibrary(serverAddressHTTP, serverAddressWS) | |
| // automatically join | |
| ogClient.Join() | |
| // create clients | |
| httpClient, err := dialWithRetry(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| wsClient, err := dialWithRetry(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &GatewayUser{ | |
| Wallets: wallets, | |
| HTTPClient: httpClient, | |
| WSClient: wsClient, | |
| ogClient: ogClient, | |
| }, nil | |
| } |
| func (u GatewayUser) RegisterAccounts() error { | ||
| for _, w := range u.Wallets { | ||
| u.ogClient.RegisterAccount(w.PrivateKey(), w.Address()) | ||
| fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID()) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The RegisterAccounts function registers each wallet in the Wallets slice with the Obscuro Gateway. However, it does not handle any errors that might occur during the registration process. Consider adding error handling to this function.
- u.ogClient.RegisterAccount(w.PrivateKey(), w.Address())
- fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID())
+ err := u.ogClient.RegisterAccount(w.PrivateKey(), w.Address())
+ if err != nil {
+ return fmt.Errorf("failed to register address %s for user: %s: %w", w.Address().Hex(), u.ogClient.UserID(), err)
+ }
+ fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID())Committable suggestion (Beta)
| func (u GatewayUser) RegisterAccounts() error { | |
| for _, w := range u.Wallets { | |
| u.ogClient.RegisterAccount(w.PrivateKey(), w.Address()) | |
| fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID()) | |
| } | |
| return nil | |
| } | |
| func (u GatewayUser) RegisterAccounts() error { | |
| for _, w := range u.Wallets { | |
| err := u.ogClient.RegisterAccount(w.PrivateKey(), w.Address()) | |
| if err != nil { | |
| return fmt.Errorf("failed to register address %s for user: %s: %w", w.Address().Hex(), u.ogClient.UserID(), err) | |
| } | |
| fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID()) | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/gateway_user.go (1} hunks)
- integration/obscurogateway/obscurogateway_test.go (8} hunks)
Additional comments: 5
integration/obscurogateway/obscurogateway_test.go (5)
- 11-19: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [4-16]
The import of
"bytes"is added. Ensure that this import is used in the code.
87-96: The function signature for the test functions in the test map has been updated to include a
wallet.Walletparameter. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.275-280: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [269-280]
The function signature for the test function
testErrorHandlinghas been updated to include awallet.Walletparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- 308-321: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [311-322]
The function signature for the test function
testErrorsRevertedArePassedhas been updated to include awallet.Walletparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- 458-476: The function
transferETHToAddressis introduced. It transfers a specified amount of ETH from a wallet to a specified address. The function creates a transaction, signs it, sends it, and waits for the receipt. The function seems to be well-structured and logically sound.
| func NewUser(wallets []wallet.Wallet, serverAddressHTTP string, serverAddressWS string) (*GatewayUser, error) { | ||
| ogClient := lib.NewObscuroGatewayLibrary(serverAddressHTTP, serverAddressWS) | ||
|
|
||
| // automatically join | ||
| err := ogClient.Join() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // create clients | ||
| httpClient, err := ethclient.Dial(serverAddressHTTP + "/v1/" + "?u=" + ogClient.UserID()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| wsClient, err := ethclient.Dial(serverAddressWS + "/v1/" + "?u=" + ogClient.UserID()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &GatewayUser{ | ||
| Wallets: wallets, | ||
| HTTPClient: httpClient, | ||
| WSClient: wsClient, | ||
| ogClient: ogClient, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The NewUser function is creating HTTP and WebSocket clients by appending the user ID to the server address. This could potentially expose the user ID in logs or network monitoring tools, which might be a security concern. Consider using a more secure method to authenticate the user with the server, such as using HTTP headers or secure cookies.
| func (u GatewayUser) RegisterAccounts() error { | ||
| for _, w := range u.Wallets { | ||
| err := u.ogClient.RegisterAccount(w.PrivateKey(), w.Address()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("Successfully registered address %s for user: %s.\n", w.Address().Hex(), u.ogClient.UserID()) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The RegisterAccounts function is registering each wallet in the user's wallet list. However, it stops and returns an error if it fails to register any of the wallets. This could potentially leave some wallets unregistered. Consider continuing to attempt to register the remaining wallets even if one fails, and return a list of the wallets that failed to register.
| func (u GatewayUser) PrintUserAccountsBalances() error { | ||
| for _, w := range u.Wallets { | ||
| balance, err := u.HTTPClient.BalanceAt(context.Background(), w.Address(), nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Println("Balance for account ", w.Address().Hex(), " - ", balance.String()) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
As per the previous comment, the PrintUserAccountsBalances function is directly printing the balance of each account to the console. This might not be desirable in a production environment. Consider returning the balances to the caller instead, so they can decide how to handle it.
- fmt.Println("Balance for account ", w.Address().Hex(), " - ", balance.String())
+ balances = append(balances, balance.String())Committable suggestion (Beta)
| func (u GatewayUser) PrintUserAccountsBalances() error { | |
| for _, w := range u.Wallets { | |
| balance, err := u.HTTPClient.BalanceAt(context.Background(), w.Address(), nil) | |
| if err != nil { | |
| return err | |
| } | |
| fmt.Println("Balance for account ", w.Address().Hex(), " - ", balance.String()) | |
| } | |
| return nil | |
| } | |
| func (u GatewayUser) GetUserAccountsBalances() ([]string, error) { | |
| balances := []string{} | |
| for _, w := range u.Wallets { | |
| balance, err := u.HTTPClient.BalanceAt(context.Background(), w.Address(), nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| balances = append(balances, balance.String()) | |
| } | |
| return balances, nil | |
| } |
| err = user0.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
| err = user1.PrintUserAccountsBalances() | ||
| require.NoError(t, err) | ||
| err = user2.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) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| pack, _ := eventsContractABI.Pack("message2") | ||
| result, err := user1.HTTPClient.CallContract(context.Background(), ethereum.CallMsg{ | ||
| From: user1.Wallets[0].Address(), | ||
| To: &contractReceipt.ContractAddress, | ||
| Data: pack, | ||
| }, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| resultMessage := string(bytes.TrimRight(result[64:], "\x00")) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if the value is the same as hardcoded in smart contract | ||
| hardcodedMessageValue := "foo" | ||
| assert.Equal(t, hardcodedMessageValue, resultMessage) | ||
|
|
||
| // subscribe with all three users for all events in deployed contract | ||
| var user0logs []types.Log | ||
| var user1logs []types.Log | ||
| var user2logs []types.Log | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user0.WSClient, &user0logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user1.WSClient, &user1logs) | ||
| subscribeToEvents([]gethcommon.Address{contractReceipt.ContractAddress}, nil, user2.WSClient, &user2logs) | ||
|
|
||
| // user1 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the first in OG | ||
| user1MessageValue := "user1PublicEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage", "user1PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user1.HTTPClient, user1.Wallets[0], eventsContractABI, "setMessage2", "user1PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| pack, _ = eventsContractABI.Pack("message2") | ||
| result, err = user1.HTTPClient.CallContract(context.Background(), ethereum.CallMsg{ | ||
| From: user1.Wallets[0].Address(), | ||
| To: &contractReceipt.ContractAddress, | ||
| Data: pack, | ||
| }, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| resultMessage = string(bytes.TrimRight(result[64:], "\x00")) | ||
| assert.Equal(t, user1MessageValue, resultMessage) | ||
|
|
||
| // user2 calls setMessage and setMessage2 on deployed smart contract with the account | ||
| // that was registered as the second in OG | ||
| user2MessageValue := "user2PublicEvent" | ||
| // interact with smart contract and cause events to be emitted | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage", "user2PrivateEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
| _, err = integrationCommon.InteractWithSmartContract(user2.HTTPClient, user2.Wallets[1], eventsContractABI, "setMessage2", "user2PublicEvent", contractReceipt.ContractAddress) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if value was changed in the smart contract with the interactions above | ||
| pack, _ = eventsContractABI.Pack("message2") | ||
| result, err = user1.HTTPClient.CallContract(context.Background(), ethereum.CallMsg{ | ||
| From: user1.Wallets[0].Address(), | ||
| To: &contractReceipt.ContractAddress, | ||
| Data: pack, | ||
| }, nil) | ||
| require.NoError(t, err) | ||
| resultMessage = string(bytes.TrimRight(result[64:], "\x00")) | ||
| assert.Equal(t, user2MessageValue, resultMessage) | ||
|
|
||
| // 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 setMessage2) | ||
| assert.Equal(t, 2, len(user0logs)) | ||
| // user1 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 3, len(user1logs)) | ||
| // user2 should see three events (two lifecycle events - same as user0) and event with his interaction with setMessage | ||
| assert.Equal(t, 2, len(user2logs)) | ||
| } |
There was a problem hiding this comment.
The function testMultipleAccountsSubscription is introduced. It tests the ability of multiple accounts to subscribe to events. The function creates three users, each with multiple wallets, registers the accounts, transfers funds to the wallets, deploys a smart contract, interacts with the contract, and subscribes to events. The function then checks the number of logs received by each user. The function seems to be well-structured and logically sound. However, it is quite long and does a lot of things. Consider breaking it down into smaller, more manageable functions for better readability and maintainability.
| func subscribeToEvents(addresses []gethcommon.Address, topics [][]gethcommon.Hash, client *ethclient.Client, logs *[]types.Log) { | ||
| // Make a subscription | ||
| filterQuery := ethereum.FilterQuery{ | ||
| Addresses: addresses, | ||
| FromBlock: big.NewInt(0), // todo (@ziga) - without those we get errors - fix that and make them configurable | ||
| ToBlock: big.NewInt(10000), | ||
| Topics: topics, | ||
| } | ||
| logsCh := make(chan types.Log) | ||
|
|
||
| subscription, err := client.SubscribeFilterLogs(context.Background(), filterQuery, logsCh) | ||
| if err != nil { | ||
| fmt.Printf("Failed to subscribe to filter logs: %v\n", err) | ||
| } | ||
| // todo (@ziga) - unsubscribe when it is fixed... | ||
| // defer subscription.Unsubscribe() // cleanup | ||
|
|
||
| // Listen for logs in a goroutine | ||
| go func() { | ||
| for { | ||
| select { | ||
| case err := <-subscription.Err(): | ||
| fmt.Printf("Error from logs subscription: %v\n", err) | ||
| return | ||
| case log := <-logsCh: | ||
| // append logs to be visible from the main thread | ||
| *logs = append(*logs, log) | ||
| } | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
The function subscribeToEvents is introduced. It subscribes to events from certain addresses and appends received logs to a given slice. The function creates a filter query, makes a subscription, and listens for logs in a goroutine. The function looks fine, but there are a few points that could be improved:
-
The function uses
fmt.Printffor logging. It would be better to use a proper logging framework that supports different log levels and formats. -
The function does not return any value. It would be good to return the subscription so that it can be unsubscribed later.
-
The function does not handle the case where the subscription is closed. It would be good to add a case for
subscription.Closed()in the select statement to handle this. -
The function uses a lot of magic numbers (e.g.,
FromBlock,ToBlock). It would be better to define these as constants at the top of the file or pass them as parameters to the function.
Why this change is needed
We currently subscribe to events only with first address that the user has registered (in case address is not found in topics).
This is not the behaviour we want as we want users to receive all the events that are relevant to them (not just to the first account they added to the gateway)
What changes were made as part of this PR
TestObscuroGatewaySubscriptionsWithMultipleAccountswas added to confirm current problems and to enable easier testing and debugging when it comes to subscriptions.ubscriptionspackage was added to handle logic for subscribingThis PR intentionally doesn't enable multiple account subscriptions. It will be added in a future PR to avoid too big PRs.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks