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.

Sign up for GitHub

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

api - Refactor removal / replacement of deprecated and obsolete funcs #4976

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
+89 −91
Open

api - Refactor removal / replacement of deprecated and obsolete funcs #4976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0da32fd
updated backend StatusBackend interface to match usage
Samyoul Mar 21, 2024
699d9cf
backend_test : Resolved unhandled errors, import collisions and upgra…
Samyoul Mar 22, 2024
64ba08e
geth_backend : resolved import collisions
Samyoul Mar 22, 2024
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
7 changes: 3 additions & 4 deletions api/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package api
import (
signercore "github.com/ethereum/go-ethereum/signer/core/apitypes"

"github.com/status-im/status-go/account"
"github.com/status-im/status-go/eth-node/types"
"github.com/status-im/status-go/multiaccounts"
"github.com/status-im/status-go/multiaccounts/accounts"
Expand All @@ -15,22 +16,20 @@ import (

// StatusBackend defines the contract for the Status.im service
type StatusBackend interface {
// IsNodeRunning() bool // NOTE: Only used in tests
StartNode(config *params.NodeConfig) error // NOTE: Only used in canary
StartNodeWithKey(acc multiaccounts.Account, password string, keyHex string, conf *params.NodeConfig) error
StartNodeWithAccount(acc multiaccounts.Account, password string, conf *params.NodeConfig) error
StartNodeWithAccountAndInitialConfig(account multiaccounts.Account, password string, settings settings.Settings, conf *params.NodeConfig, subaccs []*accounts.Account) error
StopNode() error
// RestartNode() error // NOTE: Only used in tests

GetNodeConfig() (*params.NodeConfig, error)
UpdateRootDataDir(datadir string)

// SelectAccount(loginParams account.LoginParams) error
SelectAccount(loginParams account.LoginParams) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering... if this function calls func (m *DefaultManager) SelectAccount(loginParams LoginParams) error and it expects loginParams.Password to be provided, won't that return an error in case of a keycard migrated profile?

Copy link
Member Author

@Samyoul Samyoul Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting questions @saledjenic. I don't think that this change will break any existing flows.

This change only reintroduces the function signature to the StatusBackend interface. The implementation details of this function (whether loginParams.Password is expected etc) are a concern for the implementing struct.

However existing code, unaltered by this PR, that implements this function signature already uses the loginParams.Password property.

func (m *DefaultManager) SelectAccount(loginParams LoginParams) error {
	m.mu.Lock()
	defer m.mu.Unlock()

	m.accountsGenerator.Reset()

	selectedChatAccount, err := m.unlockExtendedKey(loginParams.ChatAddress.String(), loginParams.Password)
	if err != nil {
		return err
	}
	m.watchAddresses = loginParams.WatchAddresses
	m.mainAccountAddress = loginParams.MainAccount
	m.selectedChatAccount = selectedChatAccount
	return nil
}

This code is called via the LoginAccount API method. So perhaps a different API method is called when logging in via Keycard, because the failure would already be an issue without the changes in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Samyoul yes, I see what you've changed, no problem with your change at all, I just used the opportunity to ask does that somehow affect the keycard users, nothing else, sorry about confusion. :)

OpenAccounts() error
GetAccounts() ([]multiaccounts.Account, error)
LocalPairingStarted() error
// SaveAccount(account multiaccounts.Account) error
SaveAccount(account multiaccounts.Account) error
SaveAccountAndStartNodeWithKey(acc multiaccounts.Account, password string, settings settings.Settings, conf *params.NodeConfig, subaccs []*accounts.Account, keyHex string) error
Recover(rpcParams personal.RecoverParams) (types.Address, error)
Logout() error
Expand Down
81 changes: 43 additions & 38 deletions api/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
"io/ioutil"
"math/rand"
"os"
"path"
Expand Down Expand Up @@ -72,7 +71,7 @@ func setupTestWalletDB() (*sql.DB, func() error, error) {
}

func setupTestMultiDB() (*multiaccounts.Database, func() error, error) {
tmpfile, err := ioutil.TempFile("", "tests")
tmpfile, err := os.CreateTemp("", "tests")
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -112,6 +111,12 @@ func setupGethStatusBackend() (*GethStatusBackend, func() error, func() error, f
return backend, stop1, stop2, stop3, err
}

func handleError(t *testing.T, err error) {
if err != nil {
t.Logf("deferred function error: '%s'", err)
}
}

func TestBackendStartNodeConcurrently(t *testing.T) {
utils.Init()

Expand Down Expand Up @@ -617,7 +622,7 @@ func TestBackendGetVerifiedAccount(t *testing.T) {
Name: "private key keypair",
Type: accounts.KeypairTypeKey,
Accounts: []*accounts.Account{
&accounts.Account{
{
Address: address,
KeyUID: keyUID,
},
Expand Down Expand Up @@ -647,7 +652,7 @@ func TestBackendGetVerifiedAccount(t *testing.T) {
Name: "profile keypair",
Type: accounts.KeypairTypeProfile,
Accounts: []*accounts.Account{
&accounts.Account{
{
Address: types.HexToAddress(derivedInfo.Address),
KeyUID: walletInfo.KeyUID,
Type: accounts.AccountTypeGenerated,
Expand Down Expand Up @@ -675,15 +680,15 @@ func TestBackendGetVerifiedAccount(t *testing.T) {

db, err := accounts.NewDB(backend.appDB)
require.NoError(t, err)
defer db.Close()
defer handleError(t, db.Close())
_, err = backend.AccountManager().ImportAccount(pkey, password)
require.NoError(t, err)
require.NoError(t, db.SaveOrUpdateKeypair(&accounts.Keypair{
KeyUID: keyUID,
Name: "private key keypair",
Type: accounts.KeypairTypeKey,
Accounts: []*accounts.Account{
&accounts.Account{
{
Address: address,
KeyUID: keyUID,
},
Expand Down Expand Up @@ -711,7 +716,7 @@ func TestRuntimeLogLevelIsNotWrittenToDatabase(t *testing.T) {

tmpdir := t.TempDir()

json := `{
config := `{
"NetworkId": 3,
"DataDir": "` + tmpdir + `",
"KeyStoreDir": "` + tmpdir + `",
Expand All @@ -727,7 +732,7 @@ func TestRuntimeLogLevelIsNotWrittenToDatabase(t *testing.T) {
"LogLevel": "DEBUG"
}`

conf, err := params.NewConfigFromJSON(json)
conf, err := params.NewConfigFromJSON(config)
require.NoError(t, err)
require.Equal(t, "INFO", conf.RuntimeLogLevel)
keyhex := hex.EncodeToString(gethcrypto.FromECDSA(chatKey))
Expand All @@ -739,12 +744,12 @@ func TestRuntimeLogLevelIsNotWrittenToDatabase(t *testing.T) {

address := crypto.PubkeyToAddress(walletKey.PublicKey)

settings := testSettings
settings.KeyUID = keyUID
settings.Address = crypto.PubkeyToAddress(walletKey.PublicKey)
s := testSettings
s.KeyUID = keyUID
s.Address = crypto.PubkeyToAddress(walletKey.PublicKey)

chatPubKey := crypto.FromECDSAPub(&chatKey.PublicKey)
require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", settings, conf,
require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", s, conf,
[]*accounts.Account{
{Address: address, KeyUID: keyUID, Wallet: true},
{Address: crypto.PubkeyToAddress(chatKey.PublicKey), KeyUID: keyUID, Chat: true, PublicKey: chatPubKey}}, keyhex))
Expand Down Expand Up @@ -788,12 +793,12 @@ func TestLoginWithKey(t *testing.T) {

address := crypto.PubkeyToAddress(walletKey.PublicKey)

settings := testSettings
settings.KeyUID = keyUID
settings.Address = crypto.PubkeyToAddress(walletKey.PublicKey)
s := testSettings
s.KeyUID = keyUID
s.Address = crypto.PubkeyToAddress(walletKey.PublicKey)

chatPubKey := crypto.FromECDSAPub(&chatKey.PublicKey)
require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", settings, conf,
require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", s, conf,
[]*accounts.Account{
{Address: address, KeyUID: keyUID, Wallet: true},
{Address: crypto.PubkeyToAddress(chatKey.PublicKey), KeyUID: keyUID, Chat: true, PublicKey: chatPubKey}}, keyhex))
Expand Down Expand Up @@ -858,12 +863,12 @@ func TestLoginAccount(t *testing.T) {
require.NoError(t, b.Logout())
require.NoError(t, b.StopNode())

accounts, err := b.GetAccounts()
accs, err := b.GetAccounts()
require.NoError(t, err)
require.Len(t, accounts, 1)
require.Len(t, accs, 1)

loginAccountRequest := &requests.Login{
KeyUID: accounts[0].KeyUID,
KeyUID: accs[0].KeyUID,
Password: password,
WakuV2Nameserver: nameserver,
}
Expand Down Expand Up @@ -898,13 +903,13 @@ func TestVerifyDatabasePassword(t *testing.T) {

address := crypto.PubkeyToAddress(walletKey.PublicKey)

settings := testSettings
settings.KeyUID = keyUID
settings.Address = crypto.PubkeyToAddress(walletKey.PublicKey)
s := testSettings
s.KeyUID = keyUID
s.Address = crypto.PubkeyToAddress(walletKey.PublicKey)

chatPubKey := crypto.FromECDSAPub(&chatKey.PublicKey)

require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", settings, conf, []*accounts.Account{
require.NoError(t, b.SaveAccountAndStartNodeWithKey(main, "test-pass", s, conf, []*accounts.Account{
{Address: address, KeyUID: keyUID, Wallet: true},
{Address: crypto.PubkeyToAddress(chatKey.PublicKey), KeyUID: keyUID, Chat: true, PublicKey: chatPubKey}}, keyhex))
require.NoError(t, b.Logout())
Expand Down Expand Up @@ -977,14 +982,14 @@ func TestDeleteMultiaccount(t *testing.T) {
err = backend.SaveAccount(account)
require.NoError(t, err)

files, err := ioutil.ReadDir(rootDataDir)
files, err := os.ReadDir(rootDataDir)
require.NoError(t, err)
require.NotEqual(t, 3, len(files))

err = backend.DeleteMultiaccount(account.KeyUID, keyStoreDir)
require.NoError(t, err)

files, err = ioutil.ReadDir(rootDataDir)
files, err = os.ReadDir(rootDataDir)
require.NoError(t, err)
require.Equal(t, 3, len(files))
}
Expand Down Expand Up @@ -1136,13 +1141,13 @@ func TestConvertAccount(t *testing.T) {

err = backend.StartNodeWithAccountAndInitialConfig(account, password, *defaultSettings, nodeConfig, profileKeypair.Accounts)
require.NoError(t, err)
multiaccounts, err := backend.GetAccounts()
multiaccs, err := backend.GetAccounts()
require.NoError(t, err)
require.NotEmpty(t, multiaccounts[0].ColorHash)
require.NotEmpty(t, multiaccs[0].ColorHash)
serverMessenger := backend.Messenger()
require.NotNil(t, serverMessenger)

files, err := ioutil.ReadDir(rootDataDir)
files, err := os.ReadDir(rootDataDir)
require.NoError(t, err)
require.NotEqual(t, 3, len(files))

Expand Down Expand Up @@ -1239,19 +1244,19 @@ func TestConvertAccount(t *testing.T) {
}

func copyFile(srcFolder string, dstFolder string, fileName string, t *testing.T) {
data, err := ioutil.ReadFile(path.Join(srcFolder, fileName))
data, err := os.ReadFile(path.Join(srcFolder, fileName))
if err != nil {
t.Fail()
}

err = ioutil.WriteFile(path.Join(dstFolder, fileName), data, 0600)
err = os.WriteFile(path.Join(dstFolder, fileName), data, 0600)
if err != nil {
t.Fail()
}
}

func copyDir(srcFolder string, dstFolder string, t *testing.T) {
files, err := ioutil.ReadDir(srcFolder)
files, err := os.ReadDir(srcFolder)
require.NoError(t, err)
for _, file := range files {
if !file.IsDir() {
Expand Down Expand Up @@ -1282,18 +1287,18 @@ func login(t *testing.T, conf *params.NodeConfig) {

require.NoError(t, b.OpenAccounts())

accounts, err := b.GetAccounts()
accs, err := b.GetAccounts()
require.NoError(t, err)

require.Len(t, accounts, 1)
require.Equal(t, username, accounts[0].Name)
require.Equal(t, keyUID, accounts[0].KeyUID)
require.Len(t, accs, 1)
require.Equal(t, username, accs[0].Name)
require.Equal(t, keyUID, accs[0].KeyUID)

wg := sync.WaitGroup{}
wg.Add(1)
go func() {
defer wg.Done()
err := b.StartNodeWithAccount(accounts[0], passwd, conf)
err := b.StartNodeWithAccount(accs[0], passwd, conf)
require.NoError(t, err)
}()

Expand Down Expand Up @@ -1360,13 +1365,13 @@ func TestChangeDatabasePassword(t *testing.T) {
require.NoError(t, err)
appDb, err := sqlite.OpenDB(appDbPath, newPassword, account.KDFIterations)
require.NoError(t, err)
appDb.Close()
defer handleError(t, appDb.Close())

walletDbPath, err := backend.getWalletDBPath(account.KeyUID)
require.NoError(t, err)
walletDb, err := sqlite.OpenDB(walletDbPath, newPassword, account.KDFIterations)
require.NoError(t, err)
walletDb.Close()
defer handleError(t, walletDb.Close())

// Test that keystore can be decrypted with the new password
acc, key, err := backend.accountManager.AddressToDecryptedAccount(accountInfo.WalletAddress, newPassword)
Expand Down
Loading