fix: clear account state when changing network #367

hanwencheng · 2019-09-09T06:50:34Z

close #363 .

Clear the state when choosing the new network. (Which may cause more bugs if user switch network between Substrate and Ethereum).
Copy the seed when creating an Ethereum account in DEV mode.

Fixed behaviour here:

Tbaut

The reason I couldn't see this bug is because in my tests I never tried to copy the seed at account creation, I only did it using the "See recovery phrase" menu.

Rather than having yet another if (isSubstrate) I would actually advise to remove the if here and handle Ethereum accounts just like Substrate accounts. In the near future both will have derivation paths. If things are set correctly, the derivation path and password will be set to empty string for Ethereum so that you don't have to change anything else. No need to empty anything with the networkChooser either. I think it's better to isolate each component and the Network chooser should not have to deal with side effect happening elsewhere.

pmespresso · 2019-09-09T08:25:43Z

src/screens/AccountBackup.js

@@ -28,6 +28,7 @@ import Button from '../components/Button';
 import TouchableItem from '../components/TouchableItem';
 import DerivationPasswordVerify from '../components/DerivationPasswordVerify';
 import AccountsStore from '../stores/AccountsStore';
+import {NETWORK_LIST, NetworkProtocols} from "../constants";


single quote

pmespresso · 2019-09-09T08:25:47Z

src/screens/AccountNetworkChooser.js

@@ -24,6 +24,7 @@ import fonts from "../fonts";
 import TouchableItem from '../components/TouchableItem';
 import { NETWORK_LIST } from '../constants';
 import AccountsStore from '../stores/AccountsStore';
+import { empty } from "../util/account";


single quote

pmespresso · 2019-09-09T13:17:51Z

@Tbaut sorry but I'm quite confused by what you mean above,

It seems this PR is just making the clippy copy the appropriate seed for the network that is chosen, and that's it.

Rather than having yet another if (isSubstrate) I would actually advise to remove the if here and handle Ethereum accounts just like Substrate accounts.

how? seed is separate from seedPhrase and derivationPath so they have to be treated separately.

If things are set correctly, the derivation path and password will be set to empty string for Ethereum so that you don't have to change anything else

FWICT that was the source of the bug being fixed here, where long pressing the seed for Ethereum would copy an empty string.

No need to empty anything with the networkChooser either

this seems to have been the other bug, where the wrong seed for the wrong network would be copied...

I think it's better to isolate each component and the Network chooser should not have to deal with side effect happening elsewhere.

? what is this referring to?

sorry if my quetsions don't make sense, but like i said, i'm quite confused here

Tbaut · 2019-09-09T13:58:45Z

This PR does solve the problem, I just think there's a better way to do so.
I propose to have the same behavior between Ethereum and Substrate accounts. So I propose to have a seedPhrase for Ethereum accounts as well so that we don't have to add conditions for Ethereum or Substrate. The DerivationPath and DerivationPassword` will likely be empty for Ethereum (until we implement the feature for v4)

how? seed is separate from seedPhrase and derivationPath so they have to be treated separately.

Sure I agree.

this seems to have been the other bug, where the wrong seed for the wrong network would be copied...

This is a side effect that doesn't happen with my suggestion.

I think it's better to isolate each component and the Network chooser should not have to deal with side effect happening elsewhere.

I don't see why we would need to totally empty an account if we change the network. This logic should not need to be there as it's a fix for something else. If we want to reuse this component for e.g #158 this logic would be in the way.

Happy to do a PR to show you what I mean. I just think there's a more elegant way to fix this bug.

Tbaut

Ok giving up on my "better solution". I still believe in it but there were other things breaking. So this solution is cool.

hanwencheng · 2019-09-09T19:47:51Z

Thanks for the Q&A session, which make things clear.

In the near future both will have derivation paths.

If that is happened (and also seedPhrase) I think it is would make sense to use a uniform data structure for Substrate and Ethreum accounts.

Another question, after I check the difference of:

seed: ${phrase}${derivePath}///${password} , see code
Copied seed clipboard Here: ${seedPhrase}${derivationPath} , see code

The only difference is whether password is included, is there a special reason that we do not include password in the account backup for Substrate account?

Tbaut · 2019-09-09T20:07:39Z

The only difference is whether password is included, is there a special reason that we do not include password in the account backup for Substrate account?

The idea is that the password should remain secret, so we give a way to test it, without ever revealing it. A malicious actor with the seed phrase can't get access to the fund (most malware would probably stop here), they would need not only derivation path but also the password on top of that.

seed: ${phrase}${derivePath}///${password} , see code

This is one of the things that broke with my suggestion, because brainwalletSign (here) doesn't support derivation path (and isn't happy with those "///" at the end of most seed phrases). Even after making this changes the signing was still broken, not sure where it comes from. Here's the suggestion I had: https://github.com/paritytech/parity-signer/compare/tbaut-suggestion?expand=1

fix: clear account state when change network

a0aeb1d

hanwencheng added the A0-pleasereview label Sep 9, 2019

hanwencheng requested review from pmespresso and Tbaut September 9, 2019 06:50

hanwencheng changed the title ~~fix: clear account state when change network~~ fix: clear account state when changing network Sep 9, 2019

Tbaut suggested changes Sep 9, 2019

View reviewed changes

pmespresso reviewed Sep 9, 2019

View reviewed changes

Tbaut approved these changes Sep 9, 2019

View reviewed changes

fix: code reformat

539e8ad

Tbaut merged commit ea0fcb0 into master Sep 9, 2019

Tbaut deleted the hanwen-choose-network branch September 9, 2019 20:55

Tbaut added A8-looksgood and removed A0-pleasereview labels Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clear account state when changing network #367

fix: clear account state when changing network #367

hanwencheng commented Sep 9, 2019

Tbaut left a comment •

edited

Loading

pmespresso Sep 9, 2019

pmespresso Sep 9, 2019

pmespresso commented Sep 9, 2019

Tbaut commented Sep 9, 2019 •

edited

Loading

Tbaut left a comment

hanwencheng commented Sep 9, 2019 •

edited

Loading

Tbaut commented Sep 9, 2019 •

edited

Loading

fix: clear account state when changing network #367

fix: clear account state when changing network #367

Conversation

hanwencheng commented Sep 9, 2019

Tbaut left a comment • edited Loading

Choose a reason for hiding this comment

pmespresso Sep 9, 2019

Choose a reason for hiding this comment

pmespresso Sep 9, 2019

Choose a reason for hiding this comment

pmespresso commented Sep 9, 2019

Tbaut commented Sep 9, 2019 • edited Loading

Tbaut left a comment

Choose a reason for hiding this comment

hanwencheng commented Sep 9, 2019 • edited Loading

Tbaut commented Sep 9, 2019 • edited Loading

Tbaut left a comment •

edited

Loading

Tbaut commented Sep 9, 2019 •

edited

Loading

hanwencheng commented Sep 9, 2019 •

edited

Loading

Tbaut commented Sep 9, 2019 •

edited

Loading