Merge branch 'watchonly-hide2' into staging-watchonly

Author: benma

backend/accounts.go

@@ -37,15 +37,6 @@ import (
 	"github.com/ethereum/go-ethereum/params"
 )
 
-// Set the `watch` setting on new accounts to this default value.
-// For now we keep it unset, and have users opt-in to watching specific accounts.
-//
-// We set it to `nil` not `false` so that we reserve the possibility to default all accounts to
-// watch-only for accounts where the user hasn't made an active decision (e.g. turn on watch-only
-// for all accounts of a keystore if the user did not activate/deactivate watch-only manually on any
-// of them).
-var defaultWatch *bool = nil
-
 // hardenedKeystart is the BIP44 offset to make a keypath element hardened.
 const hardenedKeystart uint32 = hdkeychain.HardenedKeyStart
 
@@ -488,16 +479,28 @@ func (backend *Backend) RenameAccount(accountCode accountsTypes.Code, name strin
 	return nil
 }
 
+// copyBool makes a copy, so that multiple values do not share the same reference. This avoids
+// potential future bugs if someone modified a flag like `*account.Watch = X`,
+// accidentally changing the value for many accounts that share the same reference.
+func copyBool(b *bool) *bool {
+	if b == nil {
+		return nil
+	}
+	cpy := *b
+	return &cpy
+}
+
 // AccountSetWatch sets the account's persisted watch flag to `watch`. Set to `true` if the account
 // should be loaded even if its keystore is not connected.
 // If `watch` is set to `false`, the account is unloaded and the frontend notified.
-func (backend *Backend) AccountSetWatch(accountCode accountsTypes.Code, watch bool) error {
+func (backend *Backend) AccountSetWatch(filter func(*config.Account) bool, watch *bool) error {
 	err := backend.config.ModifyAccountsConfig(func(accountsConfig *config.AccountsConfig) error {
-		acct := accountsConfig.Lookup(accountCode)
-		if acct == nil {
-			return errp.Newf("Could not find account %s", accountCode)
+		for _, acct := range accountsConfig.Accounts {
+			if !filter(acct) {
+				continue
+			}
+			acct.Watch = copyBool(watch)
 		}
-		acct.Watch = &watch
 		return nil
 	})
 	if err != nil {
@@ -510,18 +513,18 @@ func (backend *Backend) AccountSetWatch(accountCode accountsTypes.Code, watch bo
 	//
 	// This ensures that removing a keystore after setting an ETH account including tokens to
 	// watchonly results in the account *and* the tokens remaining loaded.
-	if acct := backend.accounts.lookup(accountCode); acct != nil {
+	for _, acct := range backend.accounts {
 		if acct.Config().Config.CoinCode == coinpkg.CodeETH {
 			for _, erc20TokenCode := range acct.Config().Config.ActiveTokens {
-				erc20AccountCode := Erc20AccountCode(accountCode, erc20TokenCode)
+				erc20AccountCode := Erc20AccountCode(acct.Config().Config.Code, erc20TokenCode)
 				if tokenAcct := backend.accounts.lookup(erc20AccountCode); tokenAcct != nil {
-					tokenAcct.Config().Config.Watch = &watch
+					tokenAcct.Config().Config.Watch = copyBool(watch)
 				}
 			}
 		}
 	}
 
-	if !watch {
+	if watch == nil || !*watch {
 		backend.initAccounts(false)
 		backend.emitAccountsStatusChanged()
 	}
@@ -753,6 +756,12 @@ func (backend *Backend) persistBTCAccountConfig(
 		return err
 	}
 
+	var accountWatch *bool
+	if backend.config.AppConfig().Backend.Watchonly {
+		t := true
+		accountWatch = &t
+	}
+
 	var signingConfigurations signing.Configurations
 	for _, cfg := range supportedConfigs {
 		extendedPublicKey, err := keystore.ExtendedPublicKey(coin, cfg.keypath)
@@ -774,7 +783,7 @@ func (backend *Backend) persistBTCAccountConfig(
 	if keystore.SupportsUnifiedAccounts() {
 		return backend.persistAccount(config.Account{
 			HiddenBecauseUnused:   hiddenBecauseUnused,
-			Watch:                 defaultWatch,
+			Watch:                 accountWatch,
 			CoinCode:              coin.Code(),
 			Name:                  name,
 			Code:                  code,
@@ -794,7 +803,7 @@ func (backend *Backend) persistBTCAccountConfig(
 
 		err := backend.persistAccount(config.Account{
 			HiddenBecauseUnused:   hiddenBecauseUnused,
-			Watch:                 defaultWatch,
+			Watch:                 copyBool(accountWatch),
 			CoinCode:              coin.Code(),
 			Name:                  suffixedName,
 			Code:                  splitAccountCode(code, cfg.ScriptType()),
@@ -849,9 +858,15 @@ func (backend *Backend) persistETHAccountConfig(
 		),
 	}
 
+	var accountWatch *bool
+	if backend.config.AppConfig().Backend.Watchonly {
+		t := true
+		accountWatch = &t
+	}
+
 	return backend.persistAccount(config.Account{
 		HiddenBecauseUnused:   hiddenBecauseUnused,
-		Watch:                 defaultWatch,
+		Watch:                 accountWatch,
 		CoinCode:              coin.Code(),
 		Name:                  name,
 		Code:                  code,
@@ -864,7 +879,7 @@ func (backend *Backend) persistETHAccountConfig(
 func (backend *Backend) initPersistedAccounts() {
 	// Only load accounts which belong to connected keystores or for which watchonly is enabled.
 	keystoreConnectedOrWatch := func(account *config.Account) bool {
-		if account.IsWatch() {
+		if account.IsWatch(backend.config.AppConfig().Backend.Watchonly) {
 			return true
 		}
 
@@ -900,7 +915,7 @@ outer:
 		// Watch-only accounts are loaded regardless, and if later e.g. a BitBox02 BTC-only is
 		// inserted with the same seed as a Multi, we will need to catch that mismatch when the
 		// keystore will be used to e.g. display an Ethereum address etc.
-		if backend.keystore != nil && !account.IsWatch() {
+		if backend.keystore != nil && !account.IsWatch(backend.config.AppConfig().Backend.Watchonly) {
 			switch coin.(type) {
 			case *btc.Coin:
 				for _, cfg := range account.SigningConfigurations {
@@ -1033,6 +1048,26 @@ func (backend *Backend) maybeAddP2TR(keystore keystore.Keystore, accounts []*con
 // were created (persisted) before the introduction of taproot support.
 func (backend *Backend) updatePersistedAccounts(
 	keystore keystore.Keystore, accounts []*config.Account) error {
+
+	// setWatch, if the global Watchonly flag is enabled, sets the `Watch`
+	// flag to `true`, turning this account into a watch-only account.
+	setWatch := func() error {
+		if !backend.config.AppConfig().Backend.Watchonly {
+			return nil
+		}
+		for _, account := range accounts {
+			if account.Watch == nil {
+				t := true
+				account.Watch = &t
+			}
+		}
+		return nil
+	}
+
+	if err := setWatch(); err != nil {
+		return err
+	}
+
 	return backend.maybeAddP2TR(keystore, accounts)
 }
 
@@ -1069,7 +1104,18 @@ func (backend *Backend) uninitAccounts(force bool) {
 	keep := []accounts.Interface{}
 	for _, account := range backend.accounts {
 		account := account
-		if !force && account.Config().Config.IsWatch() {
+
+		belongsToKeystore := false
+		if backend.keystore != nil {
+			fingerprint, err := backend.keystore.RootFingerprint()
+			if err != nil {
+				backend.log.WithError(err).Error("could not retrieve keystore fingerprint")
+			} else {
+				belongsToKeystore = account.Config().Config.SigningConfigurations.ContainsRootFingerprint(fingerprint)
+			}
+		}
+
+		if !force && (belongsToKeystore || account.Config().Config.IsWatch(backend.config.AppConfig().Backend.Watchonly)) {
 			// Do not uninit/remove account that is being watched.
 			keep = append(keep, account)
 			continue

backend/accounts_test.go

@@ -1046,18 +1046,12 @@ func TestAccountSupported(t *testing.T) {
 	b := newBackend(t, testnetDisabled, regtestDisabled)
 	defer b.Close()
 
+	require.NoError(t, b.SetWatchonly(true))
+
 	// Registering a new keystore persists a set of initial default accounts.
 	b.registerKeystore(bb02Multi)
 	require.Len(t, b.Accounts(), 3)
 	require.Len(t, b.Config().AccountsConfig().Accounts, 3)
-	// Mark all as watch-only.
-	require.NoError(t, b.config.ModifyAccountsConfig(func(cfg *config.AccountsConfig) error {
-		for _, acct := range cfg.Accounts {
-			f := true
-			acct.Watch = &f
-		}
-		return nil
-	}))
 
 	b.DeregisterKeystore()
 	// Registering a Bitcoin-only like keystore loads also the altcoins that were persisted
@@ -1066,15 +1060,10 @@ func TestAccountSupported(t *testing.T) {
 	require.Len(t, b.Accounts(), 3)
 	require.Len(t, b.Config().AccountsConfig().Accounts, 3)
 
-	b.DeregisterKeystore()
 	// If watch-only is disabled, then these will not be loaded if not supported by the keystore.
-	require.NoError(t, b.config.ModifyAccountsConfig(func(cfg *config.AccountsConfig) error {
-		for _, acct := range cfg.Accounts {
-			f := false
-			acct.Watch = &f
-		}
-		return nil
-	}))
+	require.NoError(t, b.SetWatchonly(false))
+	b.DeregisterKeystore()
+
 	// Registering a Bitcoin-only like keystore loads only the Bitcoin account, even though altcoins
 	// were persisted previously.
 	b.registerKeystore(bb02BtcOnly)

backend/backend.go

@@ -789,3 +789,39 @@ func (backend *Backend) GetAccountFromCode(code string) (accounts.Interface, err
 func (backend *Backend) CancelConnectKeystore() {
 	backend.connectKeystore.cancel(errUserAbort)
 }
+
+// SetWatchonly sets the app config's watchonly flag to `watchonly`.
+// When enabling watchonly, all currently loaded accounts are turned into watchonly accounts.
+// When disabling watchonly, all persisted accounts's watchonly status is reset.
+func (backend *Backend) SetWatchonly(watchonly bool) error {
+	err := backend.config.ModifyAppConfig(func(config *config.AppConfig) error {
+		config.Backend.Watchonly = watchonly
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	if !watchonly {
+		// When disabling watchonly, we reset the Watch flag for each account, so that when the user
+		// enables watchonly again, it does not show all accounts again - they first need to be
+		// loaded via their keystore.
+		return backend.AccountSetWatch(
+			func(*config.Account) bool {
+				// Apply to each account.
+				return true
+			},
+			nil,
+		)
+	}
+	// When enabling watchonly, we turn the currently loaded accounts into watch-only accounts.
+	t := true
+	return backend.AccountSetWatch(
+		func(account *config.Account) bool {
+			// Apply to each currently loaded account.
+			defer backend.accountsAndKeystoreLock.RLock()()
+			return backend.accounts.lookup(account.Code) != nil
+		},
+		&t,
+	)
+}

backend/backend_test.go

@@ -120,13 +120,8 @@ func TestRegisterKeystore(t *testing.T) {
 	// Deregistering the keystore leaves the loaded accounts (watchonly), and leaves the persisted
 	// accounts and keystores.
 	// Mark accounts as watch-only.
-	require.NoError(t, b.config.ModifyAccountsConfig(func(cfg *config.AccountsConfig) error {
-		for _, acct := range cfg.Accounts {
-			f := true
-			acct.Watch = &f
-		}
-		return nil
-	}))
+	require.NoError(t, b.SetWatchonly(true))
+
 	b.DeregisterKeystore()
 	require.Len(t, b.Accounts(), 3)
 	require.Len(t, b.Config().AccountsConfig().Accounts, 3)
@@ -156,14 +151,10 @@ func TestRegisterKeystore(t *testing.T) {
 	require.Equal(t, rootFingerprint2, []byte(b.Config().AccountsConfig().Keystores[1].RootFingerprint))
 
 	b.DeregisterKeystore()
-	// Enable watch-only for all but two accounts, one of each keystore. Now, all watch-only
-	// accounts plus the non-watch only accounts of the connected keystore will be loaded.
-	require.NoError(t, b.config.ModifyAccountsConfig(func(cfg *config.AccountsConfig) error {
-		for _, acct := range cfg.Accounts {
-			t := true
-			acct.Watch = &t
-		}
 
+	// Disable watch-only for two accounts, one of each keystore. Now, all accounts plus the
+	// non-watch only accounts of the connected keystore will be loaded.
+	require.NoError(t, b.config.ModifyAccountsConfig(func(cfg *config.AccountsConfig) error {
 		f := false
 		cfg.Lookup("v0-55555555-btc-0").Watch = &f
 		cfg.Lookup("v0-66666666-ltc-0").Watch = &f

backend/config/accounts.go

@@ -42,7 +42,9 @@ type Account struct {
 	// If false, the account is only displayed if the keystore is connected. If true, it is loaded
 	// and displayed when the app launches.
 	//
-	// If nil, it is considered false.
+	// If nil, it is considered false.  The reason for this is that we don't want to suddenly show
+	// all persisted accounts when the Watchonly setting is enabled - only accounts that are loaded
+	// when Watchonly is enabled should do this.
 	Watch                 *bool                  `json:"watch"`
 	CoinCode              coin.Code              `json:"coinCode"`
 	Name                  string                 `json:"name"`
@@ -73,9 +75,10 @@ func (acct *Account) SetTokenActive(tokenCode string, active bool) error {
 	return nil
 }
 
-// IsWatchOnly returns true if the `Watch` setting is set to true.
-func (acct *Account) IsWatch() bool {
-	return acct.Watch != nil && *acct.Watch
+// IsWatch returns true if the `Watch` setting is set to true and `defaultWatchonly` is true. For
+// `defaultWatchonly`, you should provide the global watchonly setting from the backend config.
+func (acct *Account) IsWatch(defaultWatchonly bool) bool {
+	return defaultWatchonly && acct.Watch != nil && *acct.Watch
 }
 
 // Keystore holds information related to keystores such as the BitBox02.

backend/config/config.go

@@ -100,6 +100,13 @@ type Backend struct {
 
 	// BtcUnit is the unit used to represent Bitcoin amounts. See `coin.BtcUnit` for details.
 	BtcUnit coin.BtcUnit `json:"btcUnit"`
+
+	// Watchonly determines if accounts should be loaded even if their keystore is not conneced.
+	// If false, accounts are only loaded if their keystore is connected.
+	// If true, they are loaded and displayed when the app launches.
+	// Individual accounts can be exempt from being loaded even if this flag is true by setting the account's
+	// `Watch` flag to false.
+	Watchonly bool `json:"watchonly"`
 }
 
 // DeprecatedCoinActive returns the Active setting for a coin by code.  This call is should not be
@@ -325,6 +332,16 @@ func (config *Config) SetAppConfig(appConfig AppConfig) error {
 	return config.save(config.appConfigFilename, config.appConfig)
 }
 
+// ModifyAppConfig calls f with the current config, allowing f to make any changes, and
+// persists the result if f returns nil error.  It propagates the f's error as is.
+func (config *Config) ModifyAppConfig(f func(*AppConfig) error) error {
+	defer config.appConfigLock.Lock()()
+	if err := f(&config.appConfig); err != nil {
+		return err
+	}
+	return config.save(config.appConfigFilename, config.appConfig)
+}
+
 // AccountsConfig returns the accounts config.
 func (config *Config) AccountsConfig() AccountsConfig {
 	defer config.accountsConfigLock.RLock()()