Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove creating all clients at startup #1662

Merged
merged 2 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions tools/walletextension/accountmanager/account_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,18 @@ func NewAccountManager(unauthedClient rpc.Client, logger gethlog.Logger) *Accoun
}
}

// GetAllAddressesWithClients returns a list of addresses which already have clients (are in accountClients map)
func (m *AccountManager) GetAllAddressesWithClients() []string {
m.accountsMutex.RLock()
defer m.accountsMutex.RUnlock()

addresses := make([]string, 0, len(m.accountClients))
for address := range m.accountClients {
addresses = append(addresses, address.Hex())
}
return addresses
}

// AddClient adds a client to the list of clients, keyed by account address.
func (m *AccountManager) AddClient(address gethcommon.Address, client *rpc.EncRPCClient) {
m.accountsMutex.Lock()
Expand Down
21 changes: 3 additions & 18 deletions tools/walletextension/container/walletextension_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"net/http"
"os"

"github.com/ethereum/go-ethereum/common"
"github.com/ten-protocol/go-ten/go/common/log"
"github.com/ten-protocol/go-ten/go/common/stopcontrol"
"github.com/ten-protocol/go-ten/go/rpc"
Expand Down Expand Up @@ -40,13 +39,12 @@ func NewWalletExtensionContainerFromConfig(config config.Config, logger gethlog.
logger.Crit("unable to create temporary client for request ", log.ErrKey, err)
}

userAccountManager := useraccountmanager.NewUserAccountManager(unAuthedClient, logger)

// 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
Copy link

@coderabbitai coderabbitai bot Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 loading

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.

Suggested change
// todo (@ziga) - implement lazy loading for clients to reduce number of connections and speed up loading

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zkokelj Thank you for the update. If there's anything else that needs attention, please let me know!

Expand All @@ -62,21 +60,8 @@ func NewWalletExtensionContainerFromConfig(config config.Config, logger gethlog.

// iterate over users create accountManagers and add all accounts to them per user
for _, user := range allUsers {
currentUserAccountManager := userAccountManager.AddAndReturnAccountManager(hex.EncodeToString(user.UserID))

accounts, err := databaseStorage.GetAccounts(user.UserID)
if err != nil {
logger.Error(fmt.Errorf("error getting accounts for user: %s, %w", hex.EncodeToString(user.UserID), err).Error())
}
for _, account := range accounts {
encClient, err := wecommon.CreateEncClient(hostRPCBindAddr, account.AccountAddress, user.PrivateKey, account.Signature, logger)
if err != nil {
logger.Error(fmt.Errorf("error creating new client, %w", err).Error())
}

// add client to current userAccountManager
currentUserAccountManager.AddClient(common.BytesToAddress(account.AccountAddress), encClient)
}
userAccountManager.AddAndReturnAccountManager(hex.EncodeToString(user.UserID))
logger.Info(fmt.Sprintf("account manager added for user: %s", hex.EncodeToString(user.UserID)))
}

// captures version in the env vars
Expand Down
73 changes: 69 additions & 4 deletions tools/walletextension/useraccountmanager/user_account_manager.go
Original file line number Diff line number Diff line change
@@ -1,23 +1,31 @@
package useraccountmanager

import (
"encoding/hex"
"fmt"

"github.com/ethereum/go-ethereum/common"
gethlog "github.com/ethereum/go-ethereum/log"
"github.com/ten-protocol/go-ten/go/rpc"
"github.com/ten-protocol/go-ten/tools/walletextension/accountmanager"
wecommon "github.com/ten-protocol/go-ten/tools/walletextension/common"
"github.com/ten-protocol/go-ten/tools/walletextension/storage"
)

type UserAccountManager struct {
userAccountManager map[string]*accountmanager.AccountManager
unauthenticatedClient rpc.Client
storage storage.Storage
hostRPCBinAddr string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the struct field name.

- hostRPCBinAddr        string
+ hostRPCBindAddr       string

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.

Suggested change
hostRPCBinAddr string
hostRPCBindAddr string

logger gethlog.Logger
}

func NewUserAccountManager(unauthenticatedClient rpc.Client, logger gethlog.Logger) UserAccountManager {
func NewUserAccountManager(unauthenticatedClient rpc.Client, logger gethlog.Logger, storage storage.Storage, hostRPCBindAddr string) UserAccountManager {
return UserAccountManager{
userAccountManager: make(map[string]*accountmanager.AccountManager),
unauthenticatedClient: unauthenticatedClient,
storage: storage,
hostRPCBinAddr: hostRPCBindAddr,
logger: logger,
}
}
Expand All @@ -34,14 +42,61 @@ func (m *UserAccountManager) AddAndReturnAccountManager(userID string) *accountm
}

// GetUserAccountManager retrieves the UserAccountManager associated with the given userID.
// It returns the UserAccountManager and nil error if one exists.
// it returns the UserAccountManager and nil error if one exists.
// before returning it checks the database and creates all missing clients for that userID
// (we are not loading all of them at startup to limit the number of established connections)
// If a UserAccountManager does not exist for the userID, it returns nil and an error.
func (m *UserAccountManager) GetUserAccountManager(userID string) (*accountmanager.AccountManager, error) {
userAccManager, exists := m.userAccountManager[userID]
if exists {
if !exists {
return nil, fmt.Errorf("UserAccountManager doesn't exist for user: %s", userID)
}

// we have userAccountManager as expected.
// now we need to create all clients that don't exist there yet
addressesWithClients := userAccManager.GetAllAddressesWithClients()

// get all addresses for current userID
userIDbytes, err := hex.DecodeString(userID)
if err != nil {
return nil, err
}

// 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never happen - but if it's used for tests then it's fine we just need a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment

return nil, fmt.Errorf("UserAccountManager doesn't exist for user: %s", userID)

databseAccounts, err := m.storage.GetAccounts(userIDbytes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

databseAccounts -> databaseAccounts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if err != nil {
return nil, err
}

userPrivateKey, err := m.storage.GetUserPrivateKey(userIDbytes)
if err != nil {
return nil, err
}

for _, account := range databseAccounts {
addressHexString := common.BytesToAddress(account.AccountAddress).Hex()
// check if a client for the current address already exists (and skip it if it does)
if addressAlreadyExists(addressHexString, addressesWithClients) {
continue
}

// create a new client
encClient, err := wecommon.CreateEncClient(m.hostRPCBinAddr, account.AccountAddress, userPrivateKey, account.Signature, m.logger)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
m.logger.Error(fmt.Errorf("error creating new client, %w", err).Error())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential addition of a nil encClient to userAccManager if CreateEncClient fails.

addressesWithClients = append(addressesWithClients, addressHexString)
}

return userAccManager, nil
}

// DeleteUserAccountManager removes the UserAccountManager associated with the given userID.
Expand All @@ -54,3 +109,13 @@ func (m *UserAccountManager) DeleteUserAccountManager(userID string) error {
delete(m.userAccountManager, userID)
return nil
}

// addressAlreadyExists is a helper function to check if an address is already present in a list of existing addresses
func addressAlreadyExists(str string, list []string) bool {
for _, v := range list {
if v == str {
return true
}
}
return false
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import (

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)
Comment on lines 10 to 17
Copy link

@coderabbitai coderabbitai bot Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not testing restart. Not including those two parameters does not have an effect on current test.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zkokelj Understood, the parameters will be omitted as they are not relevant to the current test scope. Thank you for the clarification.

Expand All @@ -21,7 +21,6 @@ func TestAddingAndGettingUserAccountManagers(t *testing.T) {
}
// We should get error if we try to get Account manager for User2
_, err = userAccountManager.GetUserAccountManager(userID2)

if err == nil {
t.Fatal("expecting error when trying to get AccountManager for user that doesn't exist.")
}
Expand Down Expand Up @@ -51,7 +50,7 @@ func TestAddingAndGettingUserAccountManagers(t *testing.T) {

func TestDeletingUserAccountManagers(t *testing.T) {
unauthedClient, _ := rpc.NewNetworkClient("ws://test")
userAccountManager := NewUserAccountManager(unauthedClient, log.New())
userAccountManager := NewUserAccountManager(unauthedClient, log.New(), nil, "")
userID := "user1"

// Add an account manager for the user
Expand Down
Loading