fix deriving on bad paths (#3257)

* fix deriving on bad paths * add more tests * allow recovery of bad paths * move blockchain into wallet descriptor type * revert changes to onboarding provider:w * remove blockchain param from use signed wallet * stash
coral-xyz · Mar 10, 2023 · edf1509 · edf1509 · vercel · Mar 10, 2023
1 parent d3e8457
commit edf1509
Show file tree

Hide file tree

Showing 20 changed files with 96 additions and 108 deletions.
diff --git a/packages/app-extension/src/components/Onboarding/pages/Finish.tsx b/packages/app-extension/src/components/Onboarding/pages/Finish.tsx
@@ -43,7 +43,7 @@ export const Finish = ({ isAddingAccount }: { isAddingAccount?: boolean }) => {
       }
       setLoading(false);
     })();
-  }, [onboardingData, isAddingAccount]);
+  }, [background, isAddingAccount, onboardingData, maybeCreateUser]);
 
   return !loading ? (
     <SetupComplete

diff --git a/packages/app-extension/src/components/Onboarding/pages/HardwareDefaultWallet.tsx b/packages/app-extension/src/components/Onboarding/pages/HardwareDefaultWallet.tsx
@@ -44,7 +44,7 @@ export const HardwareDefaultWallet = ({
       }[blockchain];
       setLedgerWallet(ledgerWallet);
     })();
-  }, [blockchain]);
+  }, [blockchain, transport]);
 
   // This is effectively the same as UI_RPC_METHOD_FIND_WALLET_DESCRIPTOR
   useEffect(() => {
@@ -92,6 +92,7 @@ export const HardwareDefaultWallet = ({
         const publicKey = publicKeys[0];
         const derivationPath = recoveryPaths[0];
         onNext({
+          blockchain,
           derivationPath,
           publicKey,
         });
@@ -100,7 +101,7 @@ export const HardwareDefaultWallet = ({
         setAccountIndex(accountIndex + 1);
       }
     })();
-  }, [ledgerWallet, accountIndex]);
+  }, [accountIndex, background, blockchain, ledgerWallet, onError, onNext]);
 
   return <Loading />;
 };
diff --git a/packages/app-extension/src/components/Onboarding/pages/HardwareDeriveWallet.tsx b/packages/app-extension/src/components/Onboarding/pages/HardwareDeriveWallet.tsx
@@ -37,7 +37,7 @@ export const HardwareDeriveWallet = ({
       }[blockchain];
       setLedgerWallet(ledgerWallet);
     })();
-  }, [blockchain]);
+  }, [blockchain, transport]);
 
   useEffect(() => {
     (async () => {
@@ -71,11 +71,12 @@ export const HardwareDeriveWallet = ({
       // path if unusable
 
       onNext({
+        blockchain,
         derivationPath,
         publicKey,
       });
     })();
-  }, [ledgerWallet]);
+  }, [background, blockchain, ledgerWallet, onError, onNext]);
 
   return <Loading />;
 };
diff --git a/packages/app-extension/src/components/Onboarding/pages/HardwareOnboard.tsx b/packages/app-extension/src/components/Onboarding/pages/HardwareOnboard.tsx
@@ -52,8 +52,9 @@ export function useHardwareOnboardSteps({
   // Flow for onboarding a hardware wallet.
   //
   const steps = [
-    <ConnectHardwareWelcome onNext={nextStep} />,
+    <ConnectHardwareWelcome key="ConnectHardwareWelcome" onNext={nextStep} />,
     <ConnectHardwareSearching
+      key="ConnectHardwareSearching"
       blockchain={blockchain}
       onNext={(transport) => {
         setTransport(transport);
@@ -141,6 +142,7 @@ export function useHardwareOnboardSteps({
       ? [
           // Sign the found wallet descriptor for API submit
         <HardwareSign
+          key="HardwareSign"
           blockchain={blockchain}
           walletDescriptor={walletDescriptor}
           message={

diff --git a/packages/app-extension/src/components/Onboarding/pages/HardwareSearchWallet.tsx b/packages/app-extension/src/components/Onboarding/pages/HardwareSearchWallet.tsx
@@ -58,13 +58,13 @@ export const HardwareSearchWallet = ({
           }
         }
         if (bs58.encode(ledgerAddress) === publicKey) {
-          onNext({ derivationPath, publicKey });
+          onNext({ blockchain, derivationPath, publicKey });
           return;
         }
       }
       setError(true);
     })();
-  }, [publicKey]);
+  }, [blockchain, publicKey, onError, onNext, transport]);
 
   if (!error) {
     return <Loading />;

diff --git a/packages/app-extension/src/components/Onboarding/pages/MnemonicSearch.tsx b/packages/app-extension/src/components/Onboarding/pages/MnemonicSearch.tsx
@@ -47,6 +47,7 @@ export const MnemonicSearch = ({
           const index = publicKeys.findIndex((p: string) => p === publicKey);
           if (index !== -1) {
             walletDescriptors.push({
+              blockchain,
               derivationPath: recoveryPaths[index],
               publicKey,
             });
@@ -59,7 +60,7 @@ export const MnemonicSearch = ({
         setError(true);
       }
     })();
-  }, [serverPublicKeys, mnemonic]);
+  }, [background, serverPublicKeys, mnemonic, onNext]);
 
   if (!error) {
     return <Loading />;

diff --git a/packages/app-extension/src/components/Onboarding/pages/OnboardAccount.tsx b/packages/app-extension/src/components/Onboarding/pages/OnboardAccount.tsx
@@ -4,15 +4,8 @@ import type {
   SignedWalletDescriptor,
   WalletDescriptor,
 } from "@coral-xyz/common";
-import {
-  getCreateMessage,
-  UI_RPC_METHOD_KEYRING_STORE_KEEP_ALIVE,
-} from "@coral-xyz/common";
-import {
-  useBackgroundClient,
-  useOnboarding,
-  useSignMessageForWallet,
-} from "@coral-xyz/recoil";
+import { getCreateMessage } from "@coral-xyz/common";
+import { useOnboarding, useSignMessageForWallet } from "@coral-xyz/recoil";
 
 import { useSteps } from "../../../hooks/useSteps";
 import { CreatePassword } from "../../common/Account/CreatePassword";
@@ -59,14 +52,15 @@ export const OnboardAccount = ({
     signedWalletDescriptors,
     selectedBlockchains,
   } = onboardingData;
+
   const signMessageForWallet = useSignMessageForWallet(mnemonic);
 
   useEffect(() => {
     // Reset blockchain keyrings on certain changes that invalidate the addresses
     setOnboardingData({
       signedWalletDescriptors: [],
     });
-  }, [action, keyringType, mnemonic]);
+  }, [action, keyringType, mnemonic, setOnboardingData]);
 
   const steps = [
     <InviteCodeForm

diff --git a/...app-extension/src/components/Unlocked/Settings/AddConnectWallet/ConnectHardware/index.tsx b/...app-extension/src/components/Unlocked/Settings/AddConnectWallet/ConnectHardware/index.tsx
@@ -41,7 +41,7 @@ export function ConnectHardware({
         UI_RPC_METHOD_BLOCKCHAIN_KEYRINGS_ADD;
     await background.request({
       method,
-      params: [blockchain, signedWalletDescriptor],
+      params: [signedWalletDescriptor],
     });
   };
 

diff --git a/packages/app-extension/src/components/Unlocked/Settings/AddConnectWallet/CreateMenu.tsx b/packages/app-extension/src/components/Unlocked/Settings/AddConnectWallet/CreateMenu.tsx
@@ -69,15 +69,15 @@ export function CreateMenu({ blockchain }: { blockchain: Blockchain }) {
         );
       }
     })();
-  }, [blockchain]);
+  }, [background, blockchain]);
 
   useEffect(() => {
     const prevTitle = nav.title;
     nav.setOptions({ headerTitle: "" });
     return () => {
       nav.setOptions({ headerTitle: prevTitle });
     };
-  }, [nav.setOptions]);
+  }, [nav]);
 
   useEffect(() => {
     (async () => {
@@ -87,7 +87,7 @@ export function CreateMenu({ blockchain }: { blockchain: Blockchain }) {
       });
       setKeyringExists(blockchainKeyrings.includes(blockchain));
     })();
-  }, [blockchain]);
+  }, [background, blockchain]);
 
   const createNewWithPhrase = async () => {
     // Mnemonic based keyring. This is the simple case because we don't
@@ -120,7 +120,7 @@ export function CreateMenu({ blockchain }: { blockchain: Blockchain }) {
       });
       await background.request({
         method: UI_RPC_METHOD_BLOCKCHAIN_KEYRINGS_ADD,
-        params: [blockchain, { ...walletDescriptor, signature }],
+        params: [{ ...walletDescriptor, signature }],
       });
       newPublicKey = walletDescriptor.publicKey;
       // Keyring now exists, toggle to other options

diff --git a/packages/app-extension/src/components/Unlocked/Settings/AddConnectWallet/ImportMnemonic.tsx b/packages/app-extension/src/components/Unlocked/Settings/AddConnectWallet/ImportMnemonic.tsx
@@ -62,7 +62,7 @@ export function ImportMnemonic({
     return () => {
       nav.setOptions({ headerTitle: prevTitle });
     };
-  }, [theme]);
+  }, [nav, theme]);
 
   // TODO replace the left nav button to go to the previous step if step > 0
 
@@ -74,13 +74,13 @@ export function ImportMnemonic({
         // import the path
         pk = await background.request({
           method: UI_RPC_METHOD_KEYRING_IMPORT_WALLET,
-          params: [blockchain, signedWalletDescriptor],
+          params: [signedWalletDescriptor],
         });
       } else {
         // Blockchain keyring doesn't exist, init
         pk = await background.request({
           method: UI_RPC_METHOD_BLOCKCHAIN_KEYRINGS_ADD,
-          params: [blockchain, signedWalletDescriptor],
+          params: [signedWalletDescriptor],
         });
       }
     } else {
@@ -106,6 +106,7 @@ export function ImportMnemonic({
     ...(inputMnemonic
       ? [
         <MnemonicInput
+          key="MnemonicInput"
           buttonLabel="Next"
           onNext={(mnemonic) => {
               setMnemonic(mnemonic);
@@ -115,6 +116,7 @@ export function ImportMnemonic({
           // Must prompt for a name if using an input mnemonic, because we can't
           // easily generate one
         <InputName
+          key="InputName"
           onNext={(name) => {
               setName(name);
               nextStep();
@@ -123,7 +125,8 @@ export function ImportMnemonic({
         ]
       : []),
     <ImportWallets
-      blockchain={blockchain!}
+      key="ImportWallets"
+      blockchain={blockchain}
       mnemonic={mnemonic}
       recovery={publicKey}
       allowMultiple={false}

diff --git a/packages/app-extension/src/components/common/Account/ImportWallets.tsx b/packages/app-extension/src/components/common/Account/ImportWallets.tsx
@@ -210,7 +210,7 @@ export function ImportWallets({
         // If the query failed assume all are valid
       }
     })();
-  }, [walletDescriptors]);
+  }, [background, blockchain, walletDescriptors]);
 
   //
   // Load a list of accounts and their associated balances
@@ -239,6 +239,7 @@ export function ImportWallets({
       .then(async (publicKeys: string[]) => {
         setWalletDescriptors(
           derivationPaths.map((derivationPath, i) => ({
+            blockchain,
             publicKey: publicKeys[i],
             derivationPath,
           }))
@@ -388,9 +389,10 @@ export function ImportWallets({
     if (currentIndex === -1) {
       // Not selected, add it
       const walletDescriptor = {
+        blockchain,
         derivationPath,
         publicKey,
-      } as WalletDescriptor;
+      };
       // Adding the account
       if (allowMultiple) {
         newCheckedWalletDescriptors.push(walletDescriptor);
@@ -448,8 +450,8 @@ export function ImportWallets({
             select
             disabled={ledgerLocked}
           >
-            {derivationPathOptions.map((o, index) => (
-              <MenuItem value={o.label} key={index}>
+            {derivationPathOptions.map((o) => (
+              <MenuItem value={o.label} key={o.label}>
                 {o.label}
               </MenuItem>
             ))}

diff --git a/packages/background/src/backend/core.ts b/packages/background/src/backend/core.ts
@@ -1180,13 +1180,10 @@ export class Backend {
     return SUCCESS_RESPONSE;
   }
 
-  async ledgerImport(
-    blockchain: Blockchain,
-    signedWalletDescriptor: SignedWalletDescriptor
-  ) {
+  async ledgerImport(signedWalletDescriptor: SignedWalletDescriptor) {
     const { signature, ...walletDescriptor } = signedWalletDescriptor;
-    const { publicKey } = walletDescriptor;
-    await this.keyringStore.ledgerImport(blockchain, walletDescriptor);
+    const { blockchain, publicKey } = walletDescriptor;
+    await this.keyringStore.ledgerImport(walletDescriptor);
     try {
       await this.userAccountPublicKeyCreate(blockchain, publicKey, signature);
     } catch (error) {
@@ -1259,7 +1256,7 @@ export class Backend {
               publicKey: publicKeys[index],
               derivationPath: recoveryPaths[index],
             };
-            await this.blockchainKeyringsAdd(blockchain, {
+            await this.blockchainKeyringsAdd({
               ...walletDescriptor,
               signature: "",
             });
@@ -1549,6 +1546,7 @@ export class Backend {
       const publicKey = publicKeys[0];
       const derivationPath = recoveryPaths[0];
       return {
+        blockchain,
         derivationPath,
         publicKey,
       };
@@ -1681,15 +1679,13 @@ export class Backend {
    * Add a new blockchain keyring to the keyring store (i.e. initialize it).
    */
   async blockchainKeyringsAdd(
-    blockchain: Blockchain,
     signedWalletDescriptor: SignedWalletDescriptor
   ): Promise<string> {
     await this.keyringStore.blockchainKeyringAdd(
-      blockchain,
       signedWalletDescriptor as WalletDescriptor
     );
 
-    const { signature, publicKey } = signedWalletDescriptor;
+    const { blockchain, signature, publicKey } = signedWalletDescriptor;
 
     // Add the new public key to the API
     try {