Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Do not allow + show an error if user imports a Parity Signer account for a chainId other than the current chain #360

Open
ltfschoen opened this issue Jan 15, 2019 · 7 comments
Assignees
Milestone

Comments

@ltfschoen
Copy link
Contributor

@axelchalon mentioned:

@parity/qr-signer returns us the address and chainId when scanning the account, so this can be done. However I think it might be easier to simply show the Signer account in the accounts list if we're connected to the right chain (we might want to show a warning at one point if we import a Signer account for a chainId other than the chain we're currently connected to -- otherwise the user might be surprised not to see their imported signer account show up in the accounts list after completing the import).

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 15, 2019

Is this issue for what Axel proposes and the warning? Because I'm not in favor of what the title says (that's different) 🙄

@ltfschoen ltfschoen changed the title Show network associated with an account similar to Parity Signer colour-coding Show warning if user imports a Parity Signer account for a chainId other than the chain we are currently connected to Jan 15, 2019
@ltfschoen
Copy link
Contributor Author

Is this issue for what Axel proposes and the warning? Because I'm not in favor of what the title says (that's different) 🙄

Yes, sorry, fixed now. It appears as though Axel has already added functionality to only show the accounts associated with a chainId that matches the chain currently in use in PR #336

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 15, 2019

I think we should simply not allow the user to import an account from the wrong chain. So "Show warning error if user imports a Parity.."

@Tbaut Tbaut added this to the 0.3-beta Mainnet milestone Jan 15, 2019
@ltfschoen
Copy link
Contributor Author

I think we should simply not allow the user to import an account from the wrong chain. So "Show warning error if user imports a Parity.."

For non-Parity Signer accounts, if a Fether user had a lot of accounts that were associated with different chains on their computer, and then they wanted to change to a different computer. If it were me I'd just copy the whole ~/Library/Application\ Support/io.parity.ethereum/keys/ directory across. Then if I started Fether without a separate Parity Ethereum instance running it would create a Parity Ethereum node using the default Kovan chain, and only the Kovan accounts would appear in the UI.
But if I used the Fether UI I'd have to backup each account individually to a JSON file and I'd put them in folders corresponding to what network they were associated with. And then assuming we had support for multiple chains in Fether, I'd have to start Fether for the first chain and then import each account that I had for it, and then repeat for the second chain, third chain, etc (so if I had 10 accounts on 3 chains I'd have to backup 30 times and import 30 times, as well as switching to a different chain a couple of times.

In this situation wouldn't it be ideal to have a "Bulk recover from JSON Keyfile" option, which just allows the user to select multiple JSON files, and then in the UI shows the filename and prompts the user to enter the associated password and continues to the next one?

For Parity Signer accounts, if a user had previously imported a lot of different Parity Signer accounts from different chains into Fether (say 30 accounts, with 10 on each chain with 3 chains), but then their computer and mobile phone got stolen but they still had their Recovery Words in safe keeping. They could use the "Recover Account" option on Parity Signer to recover each account by entering each set of recovery words respectively for each account to keep their private keys on the device. And then import into Fether each account using Fether's "Recover from Parity Signer" option using the QR codes. Again they'd have to start the first chain, import 10 accounts, then start the second chain, import 10 accounts, etc.

Could we add a "Bulk recover from Parity Signer" option that only allows importing of Parity Signer accounts associated with the currently used chain, but automates the process with less clicking?

It'd already have been quite an arduous process having to enter the sequence of recovery words 30 times to recover all the accounts in Parity Signer.

Alternatively, could we add an option in Parity Signer that "groups" the accounts list by their chainId (currently they are just sorted by name)? So when a user is importing them they can go through each of them sequentially.
If the first group is Kovan, then they can start their Kovan node, and import each one, then start a node for the second group, etc.

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 15, 2019

30 accounts feels very much like an edge case (and not related to this issue is it?). Let's consider it if the needs appears. I wouldn't create an issue at this point tbh.

@ltfschoen
Copy link
Contributor Author

30 accounts feels very much like an edge case (and not related to this issue is it?). Let's consider it if the needs appears. I wouldn't create an issue at this point tbh.

Agreed 30 accounts is an edge case, and would only be applicable to this issue if we had a "Bulk recover from Parity Signer" feature (which we don't have), because in that case the user may be "advanced" and just wants to import them quickly without any "warnings", but only needs to be warned/reminded once at the start of the process.

@ltfschoen ltfschoen changed the title Show warning if user imports a Parity Signer account for a chainId other than the chain we are currently connected to Show error if user imports a Parity Signer account for a chainId other than the chain we are currently connected to Jan 16, 2019
@Tbaut Tbaut changed the title Show error if user imports a Parity Signer account for a chainId other than the chain we are currently connected to Do not allow + show an error if user imports a Parity Signer account for a chainId other than the current chain Mar 8, 2019
@ltfschoen
Copy link
Contributor Author

ltfschoen commented Mar 31, 2019

I'm able to look into this now that #466 is resolved

@ltfschoen ltfschoen self-assigned this Mar 31, 2019
ltfschoen added a commit that referenced this issue Apr 1, 2019
@Tbaut Tbaut modified the milestones: 0.3-beta Mainnet, 0.4 Apr 11, 2019
Tbaut pushed a commit that referenced this issue Apr 15, 2019
…ount matching current chain. ETC support (#483)

* feat: Relates to #360. Only allow import from Parity Signer chain account matching current chain. ETC support

* review-fix: Refer to non-Parity chain names in the UI. Add console.error

* review-fix: Do not need to chcek health status before calling chainId RPC of light.js on pages accessed through navigation

* review-fix: Rename function name that matches current chain id with imported chain id of address

* review-fix: Remove unnecessary function

* review-fix: Rename function to accountAlreadyExists

* review-fix: Remove FIXME. See #483 (comment)

* review-fix: Refactor to use util functions isEtcChainId, chainIdToString, isNotErc20TokenAddress

* fix: Fix typo in comment

* review-fix: Change wording of parity phrase comment

* review-fix: Do not clear isImport as not account related

* fix: Clear error so error when recover from seed phrase not still shown if then click to recover from QR code

* fix: Rename so signerChainId correctly destructured and not undefined

* review-fix: Remove async/await from clear

* fix: Avoid mapping signer chain id to chain name since too much maintenance with Parity Ethereum

* review-fix: Remove await from createAccountStore

* tests: Add colour to fether-react tests

* refactor: No need to parseInt on the signerChainId

* refactor: Use isNotErc20TokenAddress

* refactor: Use isNotErc20TokenAddress again

* refactor: Add isErc20TokenAddress util so more readable

* fix: Replace valueOf with .eq. Fix so obtain BN from props

* refactor: Combine into single if statement when checking if valid Eth/Etc address

* refactor: Update utils without unnecessary return block
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants