-
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 all commits
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 | ||
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,62 @@ 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 | ||
// this should never happen, but is useful for tests | ||
if m.storage == nil { | ||
m.logger.Error("storage is nil in UserAccountManager") | ||
return userAccManager, nil | ||
} | ||
return nil, fmt.Errorf("UserAccountManager doesn't exist for user: %s", userID) | ||
|
||
databaseAccounts, err := m.storage.GetAccounts(userIDbytes) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
userPrivateKey, err := m.storage.GetUserPrivateKey(userIDbytes) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
for _, account := range databaseAccounts { | ||
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 +110,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 | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -144,6 +144,11 @@ func (w *WalletExtension) SubmitViewingKey(address gethcommon.Address, signature | |||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
144
to
+150
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 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] -= 27 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. The error message in the - return fmt.Errorf("error saving user: %s", common.DefaultUser)
+ return fmt.Errorf("error saving user: %s - %w", common.DefaultUser, err) Commitable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
// create an encrypted RPC client with the signed VK and register it with the enclave | ||||||||||||||||||||||||||||||||||||||
// todo (@ziga) - Create the clients lazily, to reduce connections to the host. | ||||||||||||||||||||||||||||||||||||||
client, err := rpc.NewEncNetworkClient(w.hostAddr, vk, w.logger) | ||||||||||||||||||||||||||||||||||||||
|
@@ -157,11 +162,6 @@ func (w *WalletExtension) SubmitViewingKey(address gethcommon.Address, signature | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
162
to
167
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 message in the - 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
Suggested change
The |
||||||||||||||||||||||||||||||||||||||
|
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.
Typo in the struct field name.
Commitable suggestion