Wallet local storage with spending password #110

dmernies-iohk · 2019-12-11T18:14:26Z

No description provided.

jpcapurro-atixlabs

Keep in mind this is not a spending password , but a wallet lock instead. I'm behind this approach since it gives us pretty reasonable privacy/security, while being much easier to implement (it doesn't have to be plugged into every use of the keys), but it's important that we're all aware of the differences.

Please check that it's necessary to save the password's hash. If that were the case, I think proper use of password salt would make rainbow table attacks (which would be pretty difficult, since passwords would not be stored in a central location) a bit more difficult.

jpcapurro-atixlabs · 2019-12-23T19:26:14Z

examples/wallet/app/actions/account.js

-export function setAccount(privateKey: string): Thunk<SetKeysAction> {
+export function setAccount(
+  privateKey: string,
+  spendingPassword: ?string


It's confusing to have a variable named the same as a type, and have it be of a different type 🙃

jpcapurro-atixlabs · 2019-12-23T19:52:13Z

examples/wallet/app/actions/account.js

+    const accountKeys = readAccountKeysFromDEN(spendingPassword);
+    if (accountKeys) {
+      const spendingPasswordKeys = {
+        walletId: 'wallet01',


why is this necessary?

It is not necessary. I added it in case I could create multiple wallets as in other wallets, for example Daedalus.

jpcapurro-atixlabs · 2019-12-23T19:58:23Z

examples/wallet/app/actions/account.js

+          .then(() => dispatch(push(routes.WALLET)))
+          .catch(error => {
+            console.log(error);
+            dispatch(push(routes.INPUT_KEYS));


what do you think about adding (as documentation) a state diagram showing what triggers redirects and to where? do you think it would be overkill? should it be apparent in the code and we're making it hard to follow?

I think it's a good idea since community developers will understand the source code more easily. I will add it

jpcapurro-atixlabs · 2019-12-23T20:23:22Z

examples/wallet/app/actions/account.js

+            dispatch(push(routes.INPUT_KEYS));
+          });
+      })
+      .catch(error => {


from what I understand, if there's an error updating the transactions, getting node settings or updating the account state, we'd redirect to the walet page.

But if there's an error redirecting to the wallet, then we'll redirect to the input keys screen.

And if there's an error in ^, then we log the error.

I don't think this is intended.

jpcapurro-atixlabs · 2019-12-23T20:30:10Z

examples/wallet/app/actions/account.js

-      return getAccountFromSeed(seed).then(
-        curry(initializeKeysAndRedirect)(dispatch)
+      return getAccountFromSeed(seed).then(keys =>
+        curry(initializeKeysAndRedirect)(dispatch, keys, spendingPassword)


If it's always used like this, consider swapping the last two parameters and doing:

.then(curry(initializeKeysAndRedirect)(dispatch, password))

If not, you could curry with a placeholder:

.then(curry(initializeKeysAndRedirect)(dispatch, _,password))

Please see the documentation to see what the _ should be, without importing the whole library.

Creating a function inline makes currying redundant.

I think it is more readable to use all the parameters. Will remain incurry (initializeKeysAndRedirect) (dispatch, keys, unlockWalletPassword)

jpcapurro-atixlabs · 2019-12-26T17:18:14Z

examples/wallet/app/components/RestoreWalletFromPrivateKey.js

+// FIXME: this has no error handling, neither while parsing the address
+// nor when fetching the balance.
+export default ({ setAccount }: Props) => {
+  const handleSubmitCreateSpending = function handleSubmitCreateSpending(


this name is confusing

The function was renamed to handleSubmit

jpcapurro-atixlabs · 2019-12-26T17:19:22Z

examples/wallet/app/components/RestoreWalletFromPrivateKey.js

+  );
+  const [hiddenSpendingPassword, setHiddenSpendingPassword] = useState(false);
+
+  const [


I would'nt bother creating a new state variable for something which can be easily computed from existing ones.

this issue persists

hiddenSpendingPassword and setHiddenSpendingPassword does not exists

jpcapurro-atixlabs · 2019-12-26T17:21:10Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+      }
+    }
+    if (isValidMnemonic(newMnemonicPhrase)) {
+      return Promise.all([


don't use Promise.all for a single promise

Promise.all is deleted

jpcapurro-atixlabs · 2019-12-26T17:21:35Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+    }
+    if (isValidMnemonic(newMnemonicPhrase)) {
+      return Promise.all([
+        setAccountFromMnemonic(newMnemonicPhrase, newMnemonicPassword, '')


same comment about optional parameters applies here

jpcapurro-atixlabs · 2019-12-26T17:22:32Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+
+  const [newMnemonicPassword, setNewMnemonicPassword] = useState('');
+
+  const checkValidSpendingPassword = function checkValidSpendingPassword(


this is duplicated code from the other restoreWallet component

jpcapurro-atixlabs · 2020-01-13T13:52:57Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+            onChange={event => setNewMnemonicPhrase(event.target.value)}
+            onBlur={() => checkIsValidMnemonicPhrase()}
+          />
+          <Form.Label className="text-danger" hidden={isMnemonicValid}>


use Form.Control.Feedback for this.
Also, research the props of that component to see if you can style it by passing a prop (some have a plaintext prop)

If that's not possible, consider using , which is a more idiomatic way to apply styles to inline text, without implying a semantic distinction ( is used for emphasis)

span is used instead em

jpcapurro-atixlabs

There was a clear improvement, but I think we could have a component to input a password and its confirmation, because it's implemented differently in both screens where it's required.

Also, there are some informative labels which could be implemented better:

First of all, labels should be the, well, label, of an input. React-bootstrap accomplishes this with its Form.Group component, which should have an input (Form.Control) and corresponding label (Form.Label).

React-bootstrap has an extra Form.Control.Feedback which behaves automagically (you only have to tell it if it's for negative or positive feedback, and it will be rendered depending on the value of the isInvalid/isValid prop for the related input, respectively), which you should use for form feedback.

Regarding informative fields, they should not be red, since this implies they are errors, which they're not. And making them s implies they have some emphasis and displays them as italics.

jpcapurro-atixlabs · 2020-01-13T13:54:12Z

examples/wallet/app/components/RestoreWalletFromPrivateKey.js

+              isInvalid={!isValidPassword}
+              onChange={event => setPassword(event.target.value)}
+            />
+            <Form.Label º hidden={isValidPassword}>


why the º ?

I think there was an accident using keyboard. Corrected

jpcapurro-atixlabs · 2020-01-13T13:54:56Z

examples/wallet/app/components/RestoreWalletFromPrivateKey.js

+              onChange={event => setConfirmPassword(event.target.value)}
+              className="mt-3"
+            />
+            <Form.Label


The same comment about Form.Control.Feedback applies here.

I will use it on all input validation.

jpcapurro-atixlabs · 2020-01-13T18:29:18Z

examples/wallet/app/actions/account.js

+  return function setKeysWithUnlockWalletPasswordThunk(dispatch) {
+    const accountKeys = readEncryptedAccountInfo(unlockWalletPassword);
+    if (accountKeys) {
+      return getAccountFromPrivateKey(accountKeys.privateKey).then(keys =>


It's not necessary to generate the AccountKeys object again, since the AccountKeys object is stored already.

this wasn't addressed yet

jpcapurro-atixlabs · 2020-01-13T18:38:35Z

examples/wallet/app/actions/account.js

+        dispatch(updateNodeSettings()),
+        dispatch(updateAccountState())
+      ])
+        .then(() => dispatch(push(routes.WALLET)))


.then(() => dispatch(push(routes.WALLET))) .catch(error => { console.error(error); dispatch(push(routes.WALLET)); });

could be written as

.catch(console.error); .then(() => dispatch(push(routes.WALLET)))

which IMO is more idiomatic since you'd want to log the error and continue with the chain regardless

also, this is repeated in the initializeKeysAndRedirect function

The suggestion was applied

jpcapurro-atixlabs · 2020-01-13T18:42:38Z

examples/wallet/app/actions/account.js

@@ -87,28 +104,37 @@ export function updateAccountTransactionsAndState(): Thunk<SetKeysAction> {
 const initializeKeysAndRedirect = (
  dispatch: Dispatch,
  keys: AccountKeys,
-  saveAccount?: boolean = true
+  unlockWalletPassword: ?string = ''


According to the form, this should not be optional

now mandatory

jpcapurro-atixlabs · 2020-01-13T19:13:24Z

examples/wallet/app/utils/storage.js

+  keys: AccountKeys
+): void {
+  const plainTextAccountInfo = JSON.stringify(keys);
+  const spedingPwd: string = unlockWalletPassword || '';


the password should not be optional. If we got here without a valid password, we should throw

Now not optional

jpcapurro-atixlabs · 2020-01-13T19:15:15Z

examples/wallet/app/utils/storage.js

-    };
+// eslint-disable-next-line flowtype/space-after-type-colon
+export function readEncryptedAccountInfo(
+  unlockWalletPassword: ?string


this shouldn't be optional.

Not optional

jpcapurro-atixlabs · 2020-01-13T19:50:06Z

examples/wallet/app/reducers/types.js

@@ -37,6 +37,10 @@ export type AccountKeys = {
  identifier: Identifier
 };

+export type UnlockWalletPassword = {


this type is only used for a definition in the account actions, it could be defined inline there.

Deleted. Not a necessary type

jpcapurro-atixlabs · 2020-01-13T19:55:13Z

examples/wallet/app/actions/account.js

 export const SET_KEYS = 'SET_KEYS';
-export const PRIVATE_KEY_ERROR = 'privateKeyError';
-export const ACCOUNT_STATE_ERROR = 'accountStateError';
+export const SET_UNLOCK_WALLET_PASSWORD = 'SET_UNLOCK_WALLET_PASSWORD';


No reducer uses this

jpcapurro-atixlabs · 2020-01-13T19:58:24Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+    }
+    setArePasswordAndConfirmationEqual(true);
+
+    if (!/^[0-9a-zA-Z]{8,}$/.test(password)) {


just check the lenght, this makes all non-latin-script passwords invalid.

This validation is deleted. Now there is a new component used to validate password only with lenght

…does not exists.

…mponents

…assword

…the functions to verify the keys to unlock the wallet with the encrypted record

dmernies-iohk

Corrections implemented

jpcapurro-atixlabs · 2020-01-22T15:16:47Z

examples/wallet/app/actions/account.js

+  return function setKeysWithUnlockWalletPasswordThunk(dispatch) {
+    const accountKeys = readEncryptedAccountInfo(unlockWalletPassword);
+    if (accountKeys) {
+      return getAccountFromPrivateKey(accountKeys.privateKey).then(keys =>


this wasn't addressed yet

jpcapurro-atixlabs · 2020-01-22T15:18:53Z

examples/wallet/app/actions/account.js

  keys: AccountKeys,
-  saveAccount?: boolean = true
+  unlockWalletPassword: string = ''


this hints the parameter is optional when it's not

jpcapurro-atixlabs · 2020-01-22T15:37:16Z

examples/wallet/app/actions/account.js

-      return getAccountFromSeed(seed).then(
-        curry(initializeKeysAndRedirect)(dispatch)
+      return getAccountFromSeed(seed).then(keys =>
+        curry(initializeKeysAndRedirect)(dispatch, keys, unlockWalletPassword)


Either go with:

.then(keys => initializeKeysAndRedirect(dispatch, keys, unlockWalletPassword) );

or:

.then( curry(initializeKeysAndRedirect)(dispatch, curry.placeholder, unlockWalletPassword) );

or change the function signature to receive the keys last, and do:

.then( curry(initializeKeysAndRedirect)(dispatch, unlockWalletPassword) );

It's confusing to curry a function and execute it right away. It might lead a reader to infer the function receives even more parameters and'll be executed later on.

jpcapurro-atixlabs · 2020-01-22T15:48:17Z

examples/wallet/app/components/CreateUnlockWalletPassword.js

+  const [isValidPassword, setIsValidPassword] = useState(true);
+
+  return (
+    <Form.Group>


Why nest the form groups? there should be one group per input AFAIK: https://react-bootstrap.netlify.com/components/forms/

jpcapurro-atixlabs · 2020-01-22T18:20:03Z

examples/wallet/app/components/CreateUnlockWalletPassword.js

+  setValidCreateUnlockWalletPassword: PropTypes.func
+};
+export default function CreateUnlockWalletPassword({
+  setValidCreateUnlockWalletPassword


on the called component, the function to be called on a change or other event should be called on{event}, to follow a react naming convention present in, for example, the <input> element with its onChange prop

here, it should be called onValidPassword or something like that.

jpcapurro-atixlabs · 2020-01-22T18:29:11Z

examples/wallet/app/components/CreateUnlockWalletPassword.js

+  ] = useState(true);
+
+  const [password, setPassword] = useState('');
+  const [confirmPassword, setConfirmPassword] = useState('');


all this variables and hooks could be typed via flow

jpcapurro-atixlabs · 2020-01-22T18:55:49Z

examples/wallet/app/containers/CreateUnlockWalletPassword.js

@@ -0,0 +1,5 @@
+// @flow


this file is now innecessary

jpcapurro-atixlabs · 2020-01-22T19:05:11Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+    setIsMnemonicValid(false);
+  };
+
+  const setValidCreateUnlockWalletPassword = function setValidCreateUnlockWalletPassword(


due to the aforementioned naming convention, this should probably be called validPasswordHandler or somethin like that.

jpcapurro-atixlabs · 2020-01-22T19:16:51Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+  };
+
+  const setValidCreateUnlockWalletPassword = function setValidCreateUnlockWalletPassword(
+    unlockPwd: string,


you could export the type of the callback in the called component and import it wherever it's used, to avoid having to type the handlers manuallly every time

jpcapurro-atixlabs · 2020-01-22T19:20:07Z

examples/wallet/app/components/RestoreWalletFromMnemonic.js

+  const handleSubmitMnemonic = function handleSubmitMnemonic(event) {
+    event.preventDefault();
+    if (isValidMnemonic(newMnemonicPhrase)) {
+      // TODO: ADD IF SENTENCE TO CHECK IF PASSWORD AND CONFIRM IS OK


I don't understand this. Shouldn't this if be enough?

dmernies-iohk force-pushed the wallet-spending-password branch 6 times, most recently from 52c1e29 to 3095c6c Compare December 18, 2019 17:37

dmernies-iohk force-pushed the wallet-spending-password branch from 3095c6c to a001d53 Compare December 18, 2019 19:13

dmernies-iohk marked this pull request as ready for review December 18, 2019 20:19

dmernies-iohk force-pushed the wallet-spending-password branch from dfb5e1f to 85eb7e7 Compare December 19, 2019 15:21

jpcapurro-atixlabs suggested changes Dec 26, 2019

View reviewed changes

dmernies-iohk force-pushed the wallet-spending-password branch 13 times, most recently from 4b2269c to d63b730 Compare January 8, 2020 17:35

dmernies-iohk force-pushed the wallet-spending-password branch 4 times, most recently from e741a49 to c8ae5ed Compare January 10, 2020 18:14

jpcapurro-atixlabs reviewed Jan 13, 2020

View reviewed changes

dmernies-iohk force-pushed the wallet-spending-password branch from c8ae5ed to a7360a0 Compare January 13, 2020 14:17

jpcapurro-atixlabs suggested changes Jan 13, 2020

View reviewed changes

dmernies-iohk force-pushed the wallet-spending-password branch 3 times, most recently from 35d376f to 07131cd Compare January 21, 2020 19:21

dmernies-iohk and others added 24 commits January 21, 2020 16:25

Corrections by code review

06b76d9

decrypt utils are added to save encrypted spending password and data

a547967

Create spending password screen is added and displayed when password …

d76a264

…does not exists.

RestoreWalletFromPrivateKey component is created to improve visual co…

17ab19a

…mponents

A new component to revoer BIP39 phrases is added

e42bc53

Saving encrypted wallet function is added and ivoked with hardcoded p…

decedd5

…assword

speding password input on restore key is added

b94f937

Unlock wallet screen is added to unlock with spending password

689b791

Unlocking wallet function are renamed

892d543

Cleaning unused code from routes and nodeConection

5c020db

Corrections by code review

0112de9

Hash is removed from the key that is saved from in localStorate. Now …

53d6a6f

…the functions to verify the keys to unlock the wallet with the encrypted record

UnlockWalletPassword is used instead of SpendingPassword

8c9a28f

fake salt is removed

f4e53a9

walletId is removed

797fabb

Code Review improvements

9815315

UnlockWalletPassword is mandatory now

4cecbb8

A new component to create a new unlock wallet is added

853a5e8

Hide spending password error is deleted

0342b9e

corrections by code review

ecc2841

Form.Control.Feeback is added instead of instead of artisan validations

75a13f1

 tag is used insetead of and Form.Feedback is added

1761414

Validation of private key is added

86215c3

Corrections by code review

86d49cc

dmernies-iohk commented Jan 21, 2020

View reviewed changes

dmernies-iohk force-pushed the wallet-spending-password branch from 07131cd to 86d49cc Compare January 21, 2020 19:30

jpcapurro-atixlabs suggested changes Jan 22, 2020

View reviewed changes


		const [newMnemonicPassword, setNewMnemonicPassword] = useState('');

		const checkValidSpendingPassword = function checkValidSpendingPassword(

Wallet local storage with spending password #110

Are you sure you want to change the base?

Wallet local storage with spending password #110

Uh oh!

Conversation

dmernies-iohk commented Dec 11, 2019

Uh oh!

jpcapurro-atixlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpcapurro-atixlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment