Add watchonly setting to accounts & handle watchonly account loading/unloading #2327
Conversation
Regression from 18aea1d, which started loading the persisted accounts for all accounts, not just ETH. For ERC20 token accounts there is no corresponding persisted account, so the token would fail to be shown in the frontend. We can just use `account.Config().Config` instead, which is loaded from the persisted config, and which has meaningful data for tokens - e.g. the same signing config as the parent ETH account.
| // 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 { |
There was a problem hiding this comment.
not unit tested yet, i plan on adding one at some point
| // watchonly results in the account *and* the tokens remaining loaded. | ||
| if acct := backend.accounts.lookup(accountCode); acct != nil { | ||
| if acct.Config().Config.CoinCode == coinpkg.CodeETH { | ||
| for _, erc20TokenCode := range acct.Config().Config.ActiveTokens { |
There was a problem hiding this comment.
@jadzeidan fyi the erc20-as-their-own-accounts model is really quite bad for the code 🙈 special cases everywhere. it would be very nice (from the code point of view) if we could bundle the tokens into the parent ETH account. could also reduce the confusion about that the fees for an erc20 token tx are coming from a "different" account.
|
@Beerosagos PTAL :) |
doing it rn :) |
| @@ -203,7 +203,7 @@ class Sidebar extends Component<Props> { | |||
| { | |||
| accountsByKeystore.map(keystore => (<React.Fragment key={keystore.keystore.rootFingerprint}> | |||
| <div className={style.sidebarHeaderContainer}> | |||
| <span className={style.sidebarHeader} hidden={!keystores?.length}> | |||
| <span className={style.sidebarHeader} hidden={!keystore.accounts.length}> | |||
| {t('sidebar.accounts')} - {keystore.keystore.name} | |||
There was a problem hiding this comment.
Just a nit: I find the ACCOUNT - <keystore name> notation a bit confusing, as it seems to suggest that is the name of an account. I'd prefer <keystore name> - ACCOUNT or even removing the ACCOUNT part completely (maybe replacing it with the root fingerprint? smth like <keystore name> [fingerprint].
Anyway, this can be discussed and possibly changed in a future PR as well.
cc @jadzeidan
There was a problem hiding this comment.
For clarity, this is the title of the group header above the accounts (it says ACCOUNTS, not ACCOUNT).
Anyway, I agree the "ACCOUNTS" part could be dropped.
About the fingeprint: we had discussed that fingeprints are too technical/noisy to show by default. that discussion was in relation to disambiguate keystores from the same wallet but different passphrases, but decided that users can just rename the individual accounts.
There was a problem hiding this comment.
Ah then it is just in the Italian translation, I guess. Plural english words are often transformed into their singular version: in Italian we have 'ACCOUNT' instead of accounts.
This will determine if an account will be displayed at launch even without the keystore being connected.
benma
force-pushed
the
watchonly-toggle
branch
from
d180798
to
3e0cef0
Compare
|
@Beerosagos thanks for the nice review! I addressed all comments, PTAL |
When disconnecting a keystore, previously all accounts were removed, because we had no watch-only accounts. When registering, all keystore accounts were (re-)loaded, because they were the only ones to be shown. With watch-only, we don't want to reinit accounts that are being watched. This allows a user to connect a keystore e.g. in the middle of an action inside an account, like displaying an address, without being disrupted. The force flag is needed to reinitialize also watch-only accounts if e.g. the activeTokens field changes for an ETH account. Ideally we can always dynamically fix the loaded accounts to match the persisted config, but that is for the future.
benma
force-pushed
the
watchonly-toggle
branch
from
3e0cef0
to
bfcbfeb
Compare
Enabling the watchonly setting on an account leaves the account loaded when the keystore is removed, and the account is loaded at app launch too.
Watch-only is opt-in per account for now - in the future we might switch to enabling watch-only by default.
Not done in this PR: handling the case when the user performs an action on a watchonly account that requires the keystore, like verifying an address or transaction. This will follow in another PR.