novasamatech / parity-signer

Air-gapped crypto wallet.
https://vault.novasama.io
GNU General Public License v3.0
558 stars 167 forks source link

Copy behavior inconsistently for Substrate and Ethereum accounts #363

Closed hanwencheng closed 5 years ago

hanwencheng commented 5 years ago

On account backup screen, we can copy the seed phrases by long pressing on it.

But this currently only works on Substrate accounts, for Ethereum accounts it would be an empty string.

Probably we should disable this copy feature by the release version. It would be safe if we force user to write the seed phrase down instead of copying it.

Related code here: https://github.com/paritytech/parity-signer/blob/master/src/screens/AccountBackup.js#L111

Screenshot 2019-09-06 at 17 09 55
Tbaut commented 5 years ago

Probably we should disable this copy feature by the release version. It would be safe if we force user to write the seed phrase down instead of copying it.

Agreed, I'd like to keep it in dev if possible as I tend to use this functionality now and then.

Tbaut commented 5 years ago

But this currently only works on Substrate accounts, for Ethereum accounts it would be an empty string.

What makes you think it's not possible for Ethereum? From the code it lgtm, just tested and I was able to copy an ethereum seed phrase. That would have been a bug.

hanwencheng commented 5 years ago

Thanks for the PR adding the development environment check!

Please check the screen record on Android devices.

https://drive.google.com/file/d/11Lh96_iMFQA4qFx0aCGDbSncul5cAmas/view?usp=sharing

There are two problems:

  1. The seed is not copied first time on Ethereum address. since in AccountBackup what we copied is:

    Clipboard.setString(`${seedPhrase}${derivationPath}`);

    But the seedPhrase and deviationPath is only available for Substrate account. So here we get empty string for Ethereum account.

  2. As we can see from the screen record the last time I changed to Ethereum account again, and here what I get from the clipboard is still the seed from the Substrate. So the every field in the state in AccountNew need to be reset every time when user click the a account with other address.

Tbaut commented 5 years ago

Right, didn't check yet but we're back to a bug now, that isn't a big deal any more since it only happens in dev mode.