Wallet recovery #342

blakecduncan · 2022-09-20T01:37:45Z

Merging is blocked by #367

What is this PR doing?

Adding an addRecoveryWallet method to the RPC
UI changes to allow the user to recover

Video demo of recovery
https://www.loom.com/share/34e84b23cd8647058dc92afc43a9ed28

How can these changes be manually tested?

You can test by recovering from an instant wallet to the Quill extension (as seen in the demo video above)
This won't currently work on Goerli because there is a bug fix in progress. If you want to test it locally you need to use [email protected]

Does this PR resolve or contribute to any issues?

#281

Checklist

I have manually tested these changes
Post a link to the PR in the group chat

Guidelines

If your PR is not ready, mark it as a draft
The resolve conversation button is for reviewers, not authors
- (But add a 'done' comment or similar)

…copy that over

extension/source/background/KeyringController.ts

…in the keyring

extension/source/background/KeyringController.ts

* added UI for entering recovery hash * added ui for salt * completed workflow for wallet recovery * lint: fix * updated hex conversion of salt * using temp wallet for recovery * replace address after recovery

JohnGuilding

Tested locally and works well. Was able to recover a wallet successfully from the instant wallet into Quill.

I'm not too confident reviewing the changes in the KeyringController.ts as it's quite a lot of new stuff for me, so would be good to get a second opinion on that specific file :)

extension/source/Home/Wallet/Wallets/Recovery/RecoverWalletModal.tsx

JohnGuilding · 2022-10-26T18:11:25Z

extension/source/Home/Wallet/Wallets/Recovery/RecoverWalletModal.tsx

+const RecoverWalletModal = () => {
+  const [modalIsOpen, setIsOpen] = useState(false);
+  const [pageIndex, setPageIndex] = useState(0);
+  const [walletPrivateKey, setWalletPrivateKey] = useState<string>('');


Do we need to explicitly add the type here? Can see that we're letting typescript infer the type for the above useState's

My personal preference is to have explicit typing everywhere. Copied the above 2 lines from somewhere else in the codebase so missed adding type to it.

I may lean towards letting typescript infer the type but don't have a strong opinion either way, so happy with either option as long as we're being consistent :)

@kautukkundan Are you sure that's your preference in this case where TypeScript is correctly inferring the type? Would you also add <boolean> and <number> to the previous lines?

@voltrevo Fine with me as is in this particular case.

The problem starts happening when using complex types. eg - let's say a new Address type is defined as a string that always starts with "0x". In this case initializing with default '' would also work as it'll infer type string, but may break code later when things get complex. This had been the case earlier when using sol.G1 and sol.G2 types in the HubbleBLS library.

So looking at broader picture and anticipating future I prefer being explicit even with simpler types to maintain consistency, If that makes sense.

extension/source/Home/Wallet/Wallets/Recovery/StepOneInfo.tsx

JohnGuilding · 2022-10-26T18:18:58Z

extension/source/Home/Wallet/Wallets/Recovery/StepOneInfo.tsx

+        <br />
+        <div className="text-[10pt] text-grey-700 leading-loose font-bold mt-2">
+          Do not close this modal until you have completed all the steps that
+          follows else you will lose access to your original keys!


Thoughts on:

"...steps that follow, else you..."

JohnGuilding · 2022-10-26T18:21:11Z

extension/source/Home/Wallet/Wallets/Recovery/StepThreeRecover.tsx

+}> = ({ onComplete, walletPrivateKey }) => {
+  const { rpc } = useQuill();
+
+  const [salt, setSalt] = useState<string>('');


Similar point here and line below, do we want to explicitly state the type?

extension/source/Home/Wallet/Wallets/Recovery/StepThreeRecover.tsx

JohnGuilding · 2022-10-26T18:28:48Z

extension/source/Home/Wallet/Wallets/Recovery/StepTwoWalletCreation.tsx

+        </div>
+        <div className="mt-4 bg-grey-900 bg-opacity-25 rounded-md p-5">
+          {loading ? (
+            <div className="flex gap-4 text-[12pt] items-center ">


Tiny bit of whitespace :)

JohnGuilding · 2022-10-26T18:38:13Z

extension/source/background/KeyringController.ts

+     * of wallet being recovered
+     * @param recoverySaltHash Salt used to set the recovery
+     * hash on the wallet that is being recovered
+     * @param signerWalletPrivateKey the private key of the wallet that is used


Nitpicky but just asking for consistency's sake, should the word "the" after signerWalletPrivateKey be capitalised here? Or should the capitalised words after the param names above this be uncapitalised?

JohnGuilding · 2022-10-26T18:54:45Z

extension/source/background/KeyringController.ts

+    const addressSignature = await this.signWalletAddress(
+      recoveryWalletAddress,
+      newPrivateKey,
+    );


Might not be fully understanding here, my two questions are:

Is this creating a signature using a randomly generated private key and an already generated contract address?

If so, how is this linking an already created address to a randomly generated private key? My knowledge of ECDSA cryptography is that the address must be derived from the public key, which is derived from the private key. But I admit my knowledge may be lacking here as these are smart contract wallets and we're not using ECDSA? And I don't fully understand how they are instantiated.

@JohnGuilding good question

Yeah it is. It's the smart contract address of the 'instant wallet'. So the wallet we are recovering. Signed by the newly generated private key that will become the new private key for the address we are recovering.

I may not be the best at explaining this but this is my understanding. This signature is basically how the trusted wallet (the one that was set in the recovery hash) says that this private key now has permission to act on behalf of the instant wallet address. When the recovery happens this private key will create a new public key but instead of deterministically generating an address the public key will map to the existing instant wallet address. This signature is a way of verifying and setting this PK as the PK to the old address without revealing the private key.

Thanks for the answer, have a better understanding now. Will make sure to re-read your recovery article too

JohnGuilding · 2022-10-26T19:00:57Z

extension/source/background/KeyringController.ts

@@ -110,6 +115,19 @@ export default class KeyringController {
      return networkData.address;
    },

+    createTempAccount: async (_request) => {


Same question for addRecoveryWallet(), do we need to add unit tests for this new logic? I'm assuming as this is the extension which isn't meant to be production code, we are happy with not adding tests?

@JohnGuilding We generally don't have tests for the extension as they would be complicated to setup & manage. However, as extension gets more stable might make sense to add some e2e tests for more common paths. Would save us some regression testing as well.

jacque006

It might also be beneficial to move some of the functionality into the clients lib so that other wallets/dApps can take advantage of this functionality, similar to #220 . But I would be okay if that's follow up work.

extension/source/Home/Wallet/Wallets/Recovery/StepThreeRecover.tsx

jacque006 · 2022-10-27T03:26:26Z

extension/source/Home/Wallet/Wallets/Recovery/StepTwoWalletCreation.tsx

+    };
+
+    createWallet();
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Why are deps disabled here? Should just be rpc.

I sometimes don't understand these deps. Like it's not possible to change some deps without explicit reload which is gonna rerender anyway.

But yeah, let's add rpc here.

Useful comment on this issue with links for further reading from Dan Abramov
facebook/create-react-app#6880 (comment)

jacque006 · 2022-10-27T03:27:28Z

extension/source/Home/Wallet/Wallets/Recovery/StepTwoWalletCreation.tsx

+          ) : (
+            <div className="flex justify-between">
+              <div className="font-mono text-[11pt]">{walletAddress}</div>
+              {/* eslint-disable-next-line */}


What rule was disabled here? Better to disable a specific rule than all linting rules.

it wanted to also have a "keyboard input" for the copy button which didn't make much sense to me.

jacque006 · 2022-10-27T03:29:21Z

extension/source/background/KeyringController.ts

@@ -110,6 +115,19 @@ export default class KeyringController {
      return networkData.address;
    },

+    createTempAccount: async (_request) => {


@JohnGuilding We generally don't have tests for the extension as they would be complicated to setup & manage. However, as extension gets more stable might make sense to add some e2e tests for more common paths. Would save us some regression testing as well.

jacque006 · 2022-10-27T03:31:37Z

extension/source/background/KeyringController.ts

@@ -110,6 +115,19 @@ export default class KeyringController {
      return networkData.address;
    },

+    createTempAccount: async (_request) => {
+      const pKey = generateRandomHex(256);


Use a BN254 spaced key instead of just a random hex. See https://github.com/thehubbleproject/hubble-bls/blob/master/src/mcl.ts#L186 . We should expose that via the signer in the client library somewhere.

@jacque006 Ahh interesting we do generateRandomHex a couple of other places in the keyring controller which is where I copied this from. Should I also switch those?

Also, just curious, what is the benefit of BN254 over a random hex?

Just want to double check that this is the right way to call that function?

randFr().serializeToHexStr()

yeah makes sense

what is the benefit of BN254 over a random hex?

If I understand correctly, The random number generated might sometimes lie outside the range of the BN curve we are using. This function ensures it's in range.

@kautukkundan correct. If the key is out of range, we just hash it into a key that is in range. I have a bucklist item/change that will be more explicit about this in the future thehubbleproject/hubble-bls#30

jacque006 · 2022-10-27T03:33:13Z

extension/source/background/KeyringController.ts

+    const netCfg = getNetworkConfig(network, this.multiNetworkConfig);
+
+    // Create new private key for the wallet we are recovering to.
+    const newPrivateKey = generateRandomHex(256);


See above comment on BLS private key gen.

kautukkundan

LGTM 🚀

blakecduncan added 3 commits September 18, 2022 19:59

Add addRecoveryWallet function to the keyring controller

739a70f

Get wallet hash from the recovery address so user no longer needs to …

ff39b25

…copy that over

Update types for addRecoveryWallet method

e1ac6df

github-actions bot added the extension Browser extension related label Sep 20, 2022

blakecduncan marked this pull request as draft September 20, 2022 01:38

blakecduncan requested a review from kautukkundan September 20, 2022 01:38

blakecduncan commented Sep 20, 2022

View reviewed changes

extension/source/background/KeyringController.ts Outdated Show resolved Hide resolved

blakecduncan commented Sep 20, 2022

View reviewed changes

extension/source/background/KeyringController.ts Outdated Show resolved Hide resolved

blakecduncan added 3 commits September 22, 2022 09:59

Fixing the add recovery wallet function

d1eca08

Use aggregator url from currently selected network

4b09caa

Adding a param for the signers private key and saving the new wallet …

0b3eff0

…in the keyring

blakecduncan commented Sep 27, 2022

View reviewed changes

extension/source/background/KeyringController.ts Show resolved Hide resolved

blakecduncan commented Sep 27, 2022

View reviewed changes

extension/source/background/KeyringController.ts Show resolved Hide resolved

kautukkundan mentioned this pull request Oct 16, 2022

added UI for entering recovery hash #355

Merged

2 tasks

blakecduncan and others added 3 commits October 20, 2022 10:55

Merge branch 'main' into wallet-recovery

20b84df

added UI for entering recovery hash (#355)

dfea315

* added UI for entering recovery hash * added ui for salt * completed workflow for wallet recovery * lint: fix * updated hex conversion of salt * using temp wallet for recovery * replace address after recovery

Lint fix

2ff4efa

blakecduncan requested a review from JohnGuilding October 26, 2022 09:07

blakecduncan self-assigned this Oct 26, 2022

blakecduncan marked this pull request as ready for review October 26, 2022 09:08

blakecduncan requested review from voltrevo and jacque006 October 26, 2022 09:11

JohnGuilding reviewed Oct 26, 2022

View reviewed changes

jacque006 reviewed Oct 27, 2022

View reviewed changes

blakecduncan added 6 commits October 27, 2022 13:19

Adding randFr() for PK generation

b4f60c9

Add back button to recovery modal

20698d1

Missed some files to commit

2284c26

Fix address issues using randFr

fa2d29f

Fix randFr function by initing the mcl

c179693

Fix lint issue

9fba928

kautukkundan approved these changes Nov 3, 2022

View reviewed changes

blakecduncan added 2 commits November 16, 2022 15:44

Merge branch 'main' into wallet-recovery

1b0b469

Merge branch 'main' into wallet-recovery

85c8667

jacque006 merged commit 9078f61 into main Dec 15, 2022

jacque006 deleted the wallet-recovery branch December 15, 2022 20:58

jacque006 mentioned this pull request Dec 15, 2022

Add recovery to extension #281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet recovery #342

Wallet recovery #342

blakecduncan commented Sep 20, 2022 •

edited

Loading

JohnGuilding left a comment

JohnGuilding Oct 26, 2022

kautukkundan Oct 27, 2022

JohnGuilding Oct 27, 2022

voltrevo Oct 31, 2022

kautukkundan Nov 3, 2022 •

edited

Loading

JohnGuilding Oct 26, 2022

JohnGuilding Oct 26, 2022

JohnGuilding Oct 26, 2022

JohnGuilding Oct 26, 2022

JohnGuilding Oct 26, 2022

blakecduncan Oct 27, 2022

JohnGuilding Oct 27, 2022

JohnGuilding Oct 26, 2022

jacque006 Oct 27, 2022

jacque006 left a comment

jacque006 Oct 27, 2022

kautukkundan Oct 27, 2022

JohnGuilding Oct 27, 2022

jacque006 Oct 27, 2022

kautukkundan Oct 27, 2022

jacque006 Oct 27, 2022

jacque006 Oct 27, 2022 •

edited

Loading

blakecduncan Oct 27, 2022 •

edited

Loading

blakecduncan Oct 27, 2022 •

edited

Loading

kautukkundan Oct 27, 2022

jacque006 Oct 27, 2022

jacque006 Oct 27, 2022

kautukkundan left a comment

Wallet recovery #342

Wallet recovery #342

Conversation

blakecduncan commented Sep 20, 2022 • edited Loading

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

JohnGuilding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kautukkundan Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacque006 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacque006 Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

blakecduncan Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

blakecduncan Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kautukkundan left a comment

Choose a reason for hiding this comment

blakecduncan commented Sep 20, 2022 •

edited

Loading

kautukkundan Nov 3, 2022 •

edited

Loading

jacque006 Oct 27, 2022 •

edited

Loading

blakecduncan Oct 27, 2022 •

edited

Loading

blakecduncan Oct 27, 2022 •

edited

Loading