Parse and construct function together (#328)

* fix: parse and construct together

* fix: tests, assert inputs valid

* fix: suri vlaidation

* fix: refactor derivation path and phrase

* fix: prevent account creation or recovery with invalid path

* Update src/screens/AccountRecover.js

Co-Authored-By: YJ <yjkimjunior@gmail.com>

* Update src/screens/AccountNew.js

Co-Authored-By: YJ <yjkimjunior@gmail.com>

src/components/AccountIconChooser.js

@@ -33,6 +33,7 @@ import { NetworkProtocols } from '../constants';
 import fonts from "../fonts";
 import { debounce } from '../util/debounce';
 import { brainWalletAddress, substrateAddress, words } from '../util/native';
+import {constructSURI} from '../util/suri'
 
 export default class AccountIconChooser extends React.PureComponent {
   constructor(props) {
@@ -48,7 +49,6 @@ export default class AccountIconChooser extends React.PureComponent {
 
     // clean previous selection
     onSelect({ newAddress: '', isBip39: false, newSeed: ''});
-
     try {
       const icons = await Promise.all(
         Array(4)
@@ -65,12 +65,19 @@ export default class AccountIconChooser extends React.PureComponent {
             if (protocol === NetworkProtocols.ETHEREUM) {
               Object.assign(result, await brainWalletAddress(result.seed));
             } else {
+              // Substrate
               try {
-                result.address = await substrateAddress(`${result.seed}${derivationPath}///${derivationPassword}`, prefix);
+                const suri = constructSURI({
+                  derivePath: derivationPath,
+                  password: derivationPassword,
+                  phrase: result.seed
+                });
+                
+                result.address = await substrateAddress(suri, prefix);
                 result.bip39 = true;
               } catch (e){
                 // invalid seed or derivation path
-                // console.error(e);
+                console.error(e);
               }
             }
             return result;

src/components/DerivationPathField.js

@@ -25,7 +25,7 @@ import {
 } from 'react-native';
 import Icon from 'react-native-vector-icons/MaterialIcons';
 
-import keyExtract from '../util/keyExtract'
+import {parseDerivationPath} from '../util/suri'
 import TextInput from './TextInput';
 
 export default function DerivationPathField(props) {
@@ -58,12 +58,24 @@ export default function DerivationPathField(props) {
       {showAdvancedField && 
         <TextInput
           onChangeText={(text) => {
-            const derivationPath = keyExtract(text);
-            onChange({
-              derivationPassword: derivationPath.password || '',
-              derivationPath: derivationPath.derivePath || ''
-            });
-            setIsValidPath(!!derivationPath.password || !!derivationPath.derivePath);
+            try {
+              const derivationPath = parseDerivationPath(text);
+
+              onChange({
+                derivationPassword: derivationPath.password || '',
+                derivationPath: derivationPath.derivePath || '',
+                isDerivationPathValid: true
+              });
+              setIsValidPath(true);
+            } catch (e) {
+              // wrong derivationPath
+              onChange({
+                derivationPassword: '',
+                derivationPath: '',
+                isDerivationPathValid: false
+              });
+              setIsValidPath(false);
+            }
           }}
           placeholder="optional derivation path"
           style={isValidPath ? ownStyles.validInput: ownStyles.invalidInput}

src/screens/AccountNew.js

@@ -17,7 +17,7 @@
 'use strict';
 
 import React from 'react';
-import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';
+import { StyleSheet, Text, View } from 'react-native';
 import { Subscribe } from 'unstated';
 
 import colors from '../colors';
@@ -32,6 +32,7 @@ import { NETWORK_LIST, NetworkProtocols } from '../constants';
 import fonts from '../fonts';
 import AccountsStore from '../stores/AccountsStore';
 import { empty, validateSeed } from '../util/account';
+import {constructSURI} from '../util/suri';
 
 export default class AccountNew extends React.Component {
   static navigationOptions = {
@@ -55,6 +56,7 @@ class AccountNewView extends React.Component {
     this.state = {
       derivationPassword: '',
       derivationPath: '',
+      isDerivationPathValid: true,
       selectedAccount: undefined,
       selectedNetwork: undefined,
     };
@@ -79,7 +81,7 @@ class AccountNewView extends React.Component {
 
   render() {
     const { accounts, navigation } = this.props;
-    const { derivationPassword, derivationPath, selectedAccount, selectedNetwork } = this.state;
+    const { derivationPassword, derivationPath, isDerivationPathValid, selectedAccount, selectedNetwork } = this.state;
     const {address, name, seed, validBip39Seed} = selectedAccount;
     const isSubstrate = selectedNetwork.protocol === NetworkProtocols.SUBSTRATE;
 
@@ -100,22 +102,38 @@ class AccountNewView extends React.Component {
               derivationPassword={derivationPassword}
               derivationPath={derivationPath}
               onSelect={({ newAddress, isBip39, newSeed }) => {
-                if (isSubstrate) {
-                  accounts.updateNew({ 
-                    address: newAddress,
-                    derivationPassword,
-                    derivationPath,
-                    seed: `${newSeed}${derivationPath}///${derivationPassword}`,
-                    seedPhrase: newSeed,
-                    validBip39Seed: isBip39
-                  });
+                if (newAddress && isBip39 && newSeed){
+                  if (isSubstrate) {
+                    try {
+                      const suri = constructSURI({
+                        derivePath: derivationPath,
+                        password: derivationPassword,
+                        phrase: newSeed
+                      });
+
+                      accounts.updateNew({ 
+                        address: newAddress,
+                        derivationPassword,
+                        derivationPath,
+                        seed: suri,
+                        seedPhrase: newSeed,
+                        validBip39Seed: isBip39
+                      });
+                    } catch (e) {
+                      console.error(e);
+                    }
+                  } else {
+                    // Ethereum account
+                    accounts.updateNew({
+                      address: newAddress,
+                      seed: newSeed,
+                      validBip39Seed: isBip39
+                    });
+                  }
                 } else {
-                  accounts.updateNew({
-                    address: newAddress,
-                    seed: newSeed,
-                    validBip39Seed: isBip39
-                });
-              }}}
+                  accounts.updateNew({ address: '', seed: '', validBip39Seed: false})
+                }
+              }}
               network={selectedNetwork}
               value={address && address}
             />
@@ -126,8 +144,8 @@ class AccountNewView extends React.Component {
               placeholder="Enter a new account name"
             />
             {isSubstrate && <DerivationPathField
-              onChange = { ({derivationPassword, derivationPath}) => {
-                this.setState({ derivationPath, derivationPassword });
+              onChange = { ({derivationPassword, derivationPath, isDerivationPathValid}) => {
+                this.setState({ derivationPath, derivationPassword, isDerivationPathValid });
               }}
               styles={styles}
           />}
@@ -139,7 +157,7 @@ class AccountNewView extends React.Component {
             <Button
               buttonStyles={styles.nextStep}
               title="Next Step"
-              disabled={!validateSeed(seed, validBip39Seed).valid}
+              disabled={!validateSeed(seed, validBip39Seed).valid || !isDerivationPathValid}
               onPress={() => {
                 validateSeed(seed, validBip39Seed).valid &&
                   navigation.navigate('AccountBackup', {

src/screens/AccountRecover.js

@@ -43,6 +43,7 @@ import AccountsStore from '../stores/AccountsStore';
 import { empty, validateSeed } from '../util/account';
 import { debounce } from '../util/debounce';
 import { brainWalletAddress, substrateAddress } from '../util/native';
+import {constructSURI} from '../util/suri';
 
 export default class AccountRecover extends React.Component {
   static navigationOptions = {
@@ -66,6 +67,7 @@ class AccountRecoverView extends React.Component {
     this.state = {
       derivationPassword: '',
       derivationPath: '',
+      isDerivationPathValid: true,
       seedPhrase: '',
       selectedAccount: undefined,
       selectedNetwork: undefined,
@@ -85,10 +87,22 @@ class AccountRecoverView extends React.Component {
     }
   }
 
+  clearNewAccount = function () {
+    const { accounts } = this.props;
+
+    accounts.updateNew({ address:'', derivationPath:'', derivationPassword:'', seed:'', seedPhrase:'', validBip39Seed: false })
+  }
+
   addressGeneration = (seedPhrase, derivationPath = '', derivationPassword = '') => {
     const { accounts } = this.props;
     const { selectedNetwork:{protocol, prefix} } = this.state;
 
+    if (!seedPhrase){
+      this.clearNewAccount();
+      
+      return;
+    }
+
     if (protocol === NetworkProtocols.ETHEREUM){
       brainWalletAddress(seedPhrase)
         .then(({ address, bip39 }) =>
@@ -96,15 +110,27 @@ class AccountRecoverView extends React.Component {
         )
         .catch(console.error);
     } else {
-      const suri = `${seedPhrase}${derivationPath}///${derivationPassword}`
-      substrateAddress(suri, prefix)
-        .then((address) => {
-          accounts.updateNew({ address, derivationPath, derivationPassword, seed: suri, seedPhrase, validBip39Seed: true })
-        })
-        .catch(
-          //invalid phrase
-          accounts.updateNew({ address:'', derivationPath:'', derivationPassword:'', seed:'', seedPhrase:'', validBip39Seed: false })
-        );
+      // Substrate
+      try {
+        const suri = constructSURI({
+          derivePath: derivationPath,
+          password: derivationPassword,
+          phrase: seedPhrase
+        });
+
+        substrateAddress(suri, prefix)
+          .then((address) => {
+            accounts.updateNew({ address, derivationPath, derivationPassword, seed: suri, seedPhrase, validBip39Seed: true })
+          })
+          .catch(() => {
+            //invalid phrase
+            this.clearNewAccount();
+          });
+      } catch (e) {
+        // invalid phrase or derivation path
+        this.clearNewAccount();
+      }
+      
     }
   };
 
@@ -129,7 +155,7 @@ class AccountRecoverView extends React.Component {
 
   render() {
     const { accounts, navigation } = this.props;
-    const { derivationPassword, derivationPath, selectedAccount, selectedNetwork} = this.state;
+    const { derivationPassword, derivationPath, isDerivationPathValid, selectedAccount, selectedNetwork} = this.state;
     const {address, name, networkKey, seedPhrase, validBip39Seed} = selectedAccount;
     const isSubstrate = selectedNetwork.protocol === NetworkProtocols.SUBSTRATE;
 
@@ -171,9 +197,9 @@ class AccountRecoverView extends React.Component {
             value={this.state.seedPhrase}
           />
           {isSubstrate && <DerivationPathField
-            onChange = { ({derivationPassword, derivationPath}) => {
+            onChange = { ({derivationPassword, derivationPath, isDerivationPathValid}) => {
               this.debouncedAddressGeneration(seedPhrase, derivationPath, derivationPassword);
-              this.setState({ derivationPath, derivationPassword });
+              this.setState({ derivationPath, derivationPassword, isDerivationPathValid });
             }}
             styles={styles}
           />}
@@ -186,7 +212,7 @@ class AccountRecoverView extends React.Component {
           />
           <Button
             buttonStyles={{ marginBottom: 40 }}
-            disabled={isSubstrate && !address}
+            disabled={isSubstrate && (!address || !isDerivationPathValid)}
             title="Next Step"
             onPress={() => {
               const validation = validateSeed(

src/stores/AccountsStore.js

@@ -20,7 +20,7 @@ import { Container } from 'unstated';
 
 import { accountId, empty } from '../util/account';
 import { loadAccounts, saveAccount } from '../util/db';
-import keyExtract from '../util/keyExtract'
+import {parseSURI} from '../util/suri'
 import { decryptData, encryptData } from '../util/native';
 
 
@@ -144,7 +144,7 @@ export default class AccountsStore extends Container {
     try {
       account.seed = await decryptData(account.encryptedSeed, pin);
 
-      const {phrase, derivePath, password} = keyExtract(account.seed)
+      const {phrase, derivePath, password} = parseSURI(account.seed)
 
       account.seedPhrase = phrase || '';
       account.derivationPath = derivePath || '';

src/util/keyExtract.js

-
-
-/**
- * @description Extracts the phrase, path and password from a SURI format for specifying secret keys `<secret>/<soft-key>//<hard-key>///<password>` (the `///password` may be omitted, and `/<soft-key>` and `//<hard-key>` maybe repeated and mixed).
- */
-
-export default function keyExtract (suri) {
-  const RE_CAPTURE = /^(\w+( \w+)*)?((\/\/?[^/]+)*)(\/\/\/(.*))?$/;
-  const matches = suri.match(RE_CAPTURE);
-
-  let phrase, derivePath, password = '';
-
-  if (matches) {
-    [, phrase = '', , derivePath = '', , , password = ''] = matches;
-  }
-
-
-  return {
-    phrase,
-    derivePath,
-    password
-  };
-}
\ No newline at end of file

src/util/suri.js

+/**
+ * @typedef {Object} SURIObject
+ * @property {string} phrase - The valid bip39 seed phrase
+ * @property {string} derivePath - The derivation path consisting in `/soft` and or `//hard`, can be repeated and interchanges
+ * @property {string} password - The optionnal password password without the `///`
+ */
+
+ /**
+ * @typedef {Object} DerivationPathObject
+ * @property {string} derivePath - The derivation path consisting in `/soft` and or `//hard`, can be repeated and interchanges
+ * @property {string} password - The optionnal password password without the `///`
+ */
+
+/**
+ * @description Extract the phrase, path and password from a SURI format for specifying secret keys `<secret>/<soft-key>//<hard-key>///<password>` (the `///password` may be omitted, and `/<soft-key>` and `//<hard-key>` maybe repeated and mixed).
+ * @param {string} suri The SURI to be parsed
+ * @returns {SURIObject}
+ */
+
+export function parseSURI (suri) {
+  const RE_CAPTURE = /^(\w+(?: \w+)*)?(.*)$/;
+  const matches = suri.match(RE_CAPTURE);
+  let phrase, derivationPath = '';
+  const ERROR = 'Invalid SURI input.';
+  
+  if (matches) {
+    [_, phrase, derivationPath = ''] = matches;
+    try {
+      parsedDerivationPath = parseDerivationPath(derivationPath)
+    } catch {
+      throw new Error(ERROR);
+    }
+  } else {
+    throw new Error(ERROR);
+  }
+
+  if(!phrase) {
+    throw new Error('SURI must contain a phrase.')
+  }
+
+  return {
+    phrase,
+    derivePath: parsedDerivationPath.derivePath || '',
+    password: parsedDerivationPath.password || ''
+  };
+}
+
+/**
+ * @description Extract the path and password from a SURI format for specifying secret keys `/<soft-key>//<hard-key>///<password>` (the `///password` may be omitted, and `/<soft-key>` and `//<hard-key>` maybe repeated and mixed).
+ * @param {string} suri The SURI to be parsed
+ * @returns {DerivationPathObject}
+ */
+
+export function parseDerivationPath (input) {
+  const RE_CAPTURE = /^((?:\/\/?[^/]+)*)(?:\/\/\/(.*))?$/;
+  const matches = input.match(RE_CAPTURE);
+  let derivePath, password;
+
+  if (matches) {
+    [_,derivePath = '', password = ''] = matches;
+  } else {
+    throw new Error('Invalid derivation path input.');
+  }
+
+  return {
+    derivePath,
+    password
+  };
+}
+
+/**
+ * @description Return a SURI format from a bip39 phrase, a derivePath, e.g `//hard/soft` and a password.
+ * @param {SURIObject} SURIObject
+ * @returns {string}
+ */
+
+export function constructSURI ({ derivePath = '', password = '', phrase }) {
+
+  if(!phrase) {
+    throw new Error('Cannot construct an SURI from emtpy phrase.');
+  }
+  
+  return `${phrase}${derivePath}///${password}`;
+}

src/util/suri.spec.js

+// Copyright 2015-2019 Parity Technologies (UK) Ltd.
+// This file is part of Parity.
+
+// Parity is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Parity is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Parity.  If not, see <http://www.gnu.org/licenses/>.
+
+'use strict';
+
+import { constructSURI, parseDerivationPath, parseSURI } from './suri';
+
+describe('derivation path', () => {
+  describe('parsing', () => {
+    it('should properly parse and return a derivation path object from a string containing soft, hard and password', () => {
+        const parsedDerivationPath = parseDerivationPath('/soft/soft//hard///mypassword');
+    
+        expect(parsedDerivationPath).toBeDefined();
+        expect(parsedDerivationPath.derivePath).toBe('/soft/soft//hard');
+        expect(parsedDerivationPath.password).toBe('mypassword');
+      });
+
+    it('should throw if the string is not a valid derivation path', () => {
+        const malformed = 'wrong/bla';
+        
+        expect(() => parseDerivationPath(malformed)).toThrowError('Invalid derivation path input.');
+    });
+
+    it('should accept a password alone', () => {
+      const passwordAlone = '///mypassword';
+      const parsedDerivationPath = parseDerivationPath(passwordAlone);
+    
+      expect(parsedDerivationPath).toBeDefined();
+      expect(parsedDerivationPath.password).toBe('mypassword');
+    })
+  });
+});
+
+describe('suri', () => {
+  describe('parsing', () => {
+    it('should properly parse and return an SURI object from a string', () => {
+        const suri = parseSURI('six nine great ball dog over moon light//hard/soft///mypassword');
+    
+        expect(suri).toBeDefined();
+        expect(suri.phrase).toBe('six nine great ball dog over moon light');
+        expect(suri.derivePath).toBe('//hard/soft');
+        expect(suri.password).toBe('mypassword');
+      });
+
+    it('should throw if the string is not a valid suri', () => {
+        const malformed = '1!,#(&(/)!_c.';
+        
+        expect(() => parseSURI(malformed)).toThrowError('Invalid SURI input.');
+    });
+
+    it('should throw if phrase was empty', () => {
+      const missingPhrase = '//hard/soft///password';
+
+      expect(() => parseSURI(missingPhrase)).toThrowError('SURI must contain a phrase.');
+    })
+  });
+
+  describe('constructing', () => {
+    it('should properly construct SURI string from object', () => {
+        const suriObject = {
+            derivePath: '//hard/soft',
+            phrase: 'six nine great ball dog over moon light',
+            password: 'mypassword'
+        };
+  
+        const suri = constructSURI(suriObject);
+  
+        expect(suri).toBeDefined();
+        expect(suri).toBe('six nine great ball dog over moon light//hard/soft///mypassword');
+    });
+
+    it('should throw if the suri object is not valid', () => {
+        const empty = {}
+
+        const malformed = {
+          phrase: null
+        }
+
+        expect(() => constructSURI(empty)).toThrow('Cannot construct an SURI from emtpy phrase.');
+        expect(() => constructSURI(malformed)).toThrow('Cannot construct an SURI from emtpy phrase.');
+    });
+  });
+});