remove creating all clients at startup #1662
Conversation
WalkthroughThe changes across various files in the wallet extension toolset involve the introduction of new functions and parameters to enhance the management of user accounts and their associated clients. The 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: 7
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tools/walletextension/accountmanager/account_manager.go (1 hunks)
- tools/walletextension/container/walletextension_container.go (3 hunks)
- tools/walletextension/useraccountmanager/user_account_manager.go (3 hunks)
- tools/walletextension/useraccountmanager/user_account_manager_test.go (3 hunks)
Additional comments: 4
tools/walletextension/useraccountmanager/user_account_manager.go (2)
23-30:
The constructor has been updated to accept new parameters for lazy initialization.113-121:
The new helper functionaddressAlreadyExistsis correctly implemented.tools/walletextension/container/walletextension_container.go (1)
- 61-65:
Consider adding error handling for theAddAndReturnAccountManagermethod to ensure account managers are added successfully.tools/walletextension/accountmanager/account_manager.go (1)
- 54-64:
The implementation ofGetAllAddressesWithClientscorrectly acquires a read lock and defers its release, ensuring thread-safe access to theaccountClientsmap. The function iterates over the map keys and collects the hexadecimal representations of the addresses.
| func TestAddingAndGettingUserAccountManagers(t *testing.T) { | ||
| unauthedClient, _ := rpc.NewNetworkClient("ws://test") | ||
| userAccountManager := NewUserAccountManager(unauthedClient, log.New()) | ||
| userID1 := "user1" | ||
| userID2 := "user2" | ||
| userAccountManager := NewUserAccountManager(unauthedClient, log.New(), nil, "ws://test") | ||
| userID1 := "4A6F686E20446F65" | ||
| userID2 := "7A65746F65A2676F" | ||
|
|
||
| // Test adding and getting account manager for userID1 | ||
| userAccountManager.AddAndReturnAccountManager(userID1) |
There was a problem hiding this comment.
The NewUserAccountManager function call in the test TestAddingAndGettingUserAccountManagers is missing the new databaseStorage and hostRPCBindAddr parameters that were mentioned in the summary of changes. The test will not compile without passing these new required arguments.
- userAccountManager := NewUserAccountManager(unauthedClient, log.New(), nil, "ws://test")
+ userAccountManager := NewUserAccountManager(unauthedClient, log.New(), databaseStorage, "ws://test")Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestAddingAndGettingUserAccountManagers(t *testing.T) { | |
| unauthedClient, _ := rpc.NewNetworkClient("ws://test") | |
| userAccountManager := NewUserAccountManager(unauthedClient, log.New()) | |
| userID1 := "user1" | |
| userID2 := "user2" | |
| userAccountManager := NewUserAccountManager(unauthedClient, log.New(), nil, "ws://test") | |
| userID1 := "4A6F686E20446F65" | |
| userID2 := "7A65746F65A2676F" | |
| // Test adding and getting account manager for userID1 | |
| userAccountManager.AddAndReturnAccountManager(userID1) | |
| func TestAddingAndGettingUserAccountManagers(t *testing.T) { | |
| unauthedClient, _ := rpc.NewNetworkClient("ws://test") | |
| userAccountManager := NewUserAccountManager(unauthedClient, log.New(), databaseStorage, "ws://test") | |
| userID1 := "4A6F686E20446F65" | |
| userID2 := "7A65746F65A2676F" | |
| // Test adding and getting account manager for userID1 | |
| userAccountManager.AddAndReturnAccountManager(userID1) |
There was a problem hiding this comment.
This test is not testing restart. Not including those two parameters does not have an effect on current test.
There was a problem hiding this comment.
@zkokelj Understood, the parameters will be omitted as they are not relevant to the current test scope. Thank you for the clarification.
| // start the database | ||
| databaseStorage, err := storage.New(config.DBType, config.DBConnectionURL, config.DBPathOverride) | ||
| if err != nil { | ||
| logger.Crit("unable to create database to store viewing keys ", log.ErrKey, err) | ||
| } | ||
| userAccountManager := useraccountmanager.NewUserAccountManager(unAuthedClient, logger, databaseStorage, hostRPCBindAddr) | ||
|
|
||
| // Get all the data from the database and add all the clients for all users | ||
| // todo (@ziga) - implement lazy loading for clients to reduce number of connections and speed up loading |
There was a problem hiding this comment.
Remove the outdated TODO comment as lazy loading has been implemented.
- // todo (@ziga) - implement lazy loading for clients to reduce number of connections and speed up loadingCommitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // todo (@ziga) - implement lazy loading for clients to reduce number of connections and speed up loading |
There was a problem hiding this comment.
@zkokelj Thank you for the update. If there's anything else that needs attention, please let me know!
| return userAccManager, nil | ||
| } | ||
| return nil, fmt.Errorf("UserAccountManager doesn't exist for user: %s", userID) | ||
|
|
||
| databseAccounts, err := m.storage.GetAccounts(userIDbytes) |
There was a problem hiding this comment.
databseAccounts -> databaseAccounts
| // log that we don't have a storage, but still return existing userAccountManager | ||
| if m.storage == nil { | ||
| m.logger.Error("storage is nil in UserAccountManager") | ||
| return userAccManager, nil | ||
| } |
There was a problem hiding this comment.
This should never happen - but if it's used for tests then it's fine we just need a comment
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (3)
- tools/walletextension/container/walletextension_container.go (3 hunks)
- tools/walletextension/useraccountmanager/user_account_manager.go (3 hunks)
- tools/walletextension/wallet_extension.go (2 hunks)
Additional comments: 4
tools/walletextension/container/walletextension_container.go (3)
7-12: The import of
github.com/ethereum/go-ethereum/commonhas been removed as mentioned in the summary. Ensure that there are no usages of this package left in the file that would lead to compilation errors.37-51: The changes made here reflect the new strategy of deferring client creation. The error handling for the creation of
unAuthedClientanddatabaseStoragehas been updated to terminate the program if an error occurs, which aligns with the previous review comments. This is a critical change that ensures the program does not continue running in an unstable state.59-66: The loop that previously created clients for all users has been removed, which is in line with the new strategy of lazy client creation. This should reduce the number of connections at startup. Ensure that the
userAccountManageris correctly handling the creation of clients when they are actually needed.tools/walletextension/useraccountmanager/user_account_manager.go (1)
- 115-122:
TheaddressAlreadyExistshelper function is correctly implemented.
| ) | ||
|
|
||
| type UserAccountManager struct { | ||
| userAccountManager map[string]*accountmanager.AccountManager | ||
| unauthenticatedClient rpc.Client | ||
| storage storage.Storage | ||
| hostRPCBinAddr string |
There was a problem hiding this comment.
Typo in the struct field name.
- hostRPCBinAddr string
+ hostRPCBindAddr stringCommitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| hostRPCBinAddr string | |
| hostRPCBindAddr string |
| } | ||
|
|
||
| // create a new client | ||
| encClient, err := wecommon.CreateEncClient(m.hostRPCBinAddr, account.AccountAddress, userPrivateKey, account.Signature, m.logger) |
There was a problem hiding this comment.
Error handling is missing after creating a new client. If CreateEncClient fails, the error should be handled appropriately, possibly by returning the error to the caller.
| // create a new client | ||
| encClient, err := wecommon.CreateEncClient(m.hostRPCBinAddr, account.AccountAddress, userPrivateKey, account.Signature, m.logger) | ||
| if err != nil { | ||
| m.logger.Error(fmt.Errorf("error creating new client, %w", err).Error()) |
There was a problem hiding this comment.
The error from client creation is logged but not handled, which could lead to an inconsistent state.
| } | ||
|
|
||
| // add a client to requested userAccountManager | ||
| userAccManager.AddClient(common.BytesToAddress(account.AccountAddress), encClient) |
There was a problem hiding this comment.
Potential addition of a nil encClient to userAccManager if CreateEncClient fails.
| signature[64] -= 27 | ||
|
|
||
| vk.Signature = signature | ||
|
|
||
| err := w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | ||
| if err != nil { | ||
| return fmt.Errorf("error saving user: %s", common.DefaultUser) |
There was a problem hiding this comment.
The subtraction of 27 from the signature's last byte is done to conform to the Ethereum signature format, but this operation modifies the original signature array directly, which could lead to unexpected behavior if the signature is used elsewhere after this method. Consider copying the signature before modifying it to ensure that the original signature remains intact.
- signature[64] -= 27
+ modifiedSignature := make([]byte, len(signature))
+ copy(modifiedSignature, signature)
+ modifiedSignature[64] -= 27Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| signature[64] -= 27 | |
| vk.Signature = signature | |
| err := w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | |
| if err != nil { | |
| return fmt.Errorf("error saving user: %s", common.DefaultUser) | |
| modifiedSignature := make([]byte, len(signature)) | |
| copy(modifiedSignature, signature) | |
| modifiedSignature[64] -= 27 | |
| vk.Signature = modifiedSignature | |
| err := w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | |
| if err != nil { | |
| return fmt.Errorf("error saving user: %s", common.DefaultUser) |
|
|
||
| err := w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | ||
| if err != nil { | ||
| return fmt.Errorf("error saving user: %s", common.DefaultUser) |
There was a problem hiding this comment.
The error message in the fmt.Errorf call should wrap the original error using %w to allow for error unwrapping by the caller.
- return fmt.Errorf("error saving user: %s", common.DefaultUser)
+ return fmt.Errorf("error saving user: %s - %w", common.DefaultUser, err)Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return fmt.Errorf("error saving user: %s", common.DefaultUser) | |
| return fmt.Errorf("error saving user: %s - %w", common.DefaultUser, err) |
|
|
||
| defaultAccountManager.AddClient(address, client) | ||
|
|
||
| err = w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | ||
| if err != nil { | ||
| return fmt.Errorf("error saving user: %s", common.DefaultUser) | ||
| } | ||
|
|
||
| err = w.storage.AddAccount([]byte(common.DefaultUser), vk.Account.Bytes(), vk.Signature) | ||
| if err != nil { | ||
| return fmt.Errorf("error saving account %s for user %s", vk.Account.Hex(), common.DefaultUser) |
There was a problem hiding this comment.
The error message in the fmt.Errorf call should wrap the original error using %w to allow for error unwrapping by the caller.
- return fmt.Errorf("error saving account %s for user %s", vk.Account.Hex(), common.DefaultUser)
+ return fmt.Errorf("error saving account %s for user %s - %w", vk.Account.Hex(), common.DefaultUser, err)Commitable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| defaultAccountManager.AddClient(address, client) | |
| err = w.storage.AddUser([]byte(common.DefaultUser), crypto.FromECDSA(vk.PrivateKey.ExportECDSA())) | |
| if err != nil { | |
| return fmt.Errorf("error saving user: %s", common.DefaultUser) | |
| } | |
| err = w.storage.AddAccount([]byte(common.DefaultUser), vk.Account.Bytes(), vk.Signature) | |
| if err != nil { | |
| return fmt.Errorf("error saving account %s for user %s", vk.Account.Hex(), common.DefaultUser) | |
| defaultAccountManager.AddClient(address, client) | |
| err = w.storage.AddAccount([]byte(common.DefaultUser), vk.Account.Bytes(), vk.Signature) | |
| if err != nil { | |
| return fmt.Errorf("error saving account %s for user %s - %w", vk.Account.Hex(), common.DefaultUser, err) |
The AddAccount method is called after the client is added to the defaultAccountManager, but if AddAccount fails, the client remains added. This could lead to an inconsistent state where the client is available but the account is not properly saved. Consider adding error handling to remove the client if saving the account fails.
zkokelj
temporarily deployed
to
sepolia-testnet
GitHub Actions
Inactive
Why this change is needed
This change is needed to fix recently found issue with large number connections to the host.
This is temporary hotfix and the way we do and maintain connection needs to be changed.
What changes were made as part of this PR
Previously we were creating clients for all existing users in the database at startup.
To avoid problems with that we now create all the clients for existing users and only when they are requested with:
GetUserAccountManagerPR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks