-
Notifications
You must be signed in to change notification settings - Fork 38
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo in the struct field name. - hostRPCBinAddr string
+ hostRPCBindAddr string Commitable suggestion
Suggested change
|
||||||
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, | ||||||
} | ||||||
} | ||||||
|
@@ -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 | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error handling is missing after creating a new client. If |
||||||
if err != nil { | ||||||
m.logger.Error(fmt.Errorf("error creating new client, %w", err).Error()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential addition of a nil |
||||||
addressesWithClients = append(addressesWithClients, addressHexString) | ||||||
} | ||||||
|
||||||
return userAccManager, nil | ||||||
} | ||||||
|
||||||
// DeleteUserAccountManager removes the UserAccountManager associated with the given userID. | ||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The - userAccountManager := NewUserAccountManager(unauthedClient, log.New(), nil, "ws://test")
+ userAccountManager := NewUserAccountManager(unauthedClient, log.New(), databaseStorage, "ws://test") Commitable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||||||||||||||||||||||
|
@@ -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.") | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
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!