Add client recovery methods #486

blakecduncan · 2023-01-25T14:51:02Z

What is this PR doing?

Adding the logic used recover an instant wallet to quill to the bls client module. There's probably room to add some more recovery functionality. This is just the logic that is currently used to recover from an Instant wallet to Quill.

How can these changes be manually tested?

Added tests to the contracts tests to make sure it works. Also, you could try recovering an instant wallet.

Does this PR resolve or contribute to any issues?

#220
#442

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)

# Conflicts: # contracts/clients/package.json # extension/package.json # extension/yarn.lock

blakecduncan · 2023-01-27T09:17:06Z

contracts/clients/src/signer/getRandomBlsPrivateKey.ts

+import { mcl } from "@thehubbleproject/bls";
+
+export default (): string => {
+  return `0x${mcl.randFr().serializeToHexStr()}`;


I originally put this here because we know that mcl has been initialised inside the signer directory. However, logically I'm thinking now that I should pull this out of the signer. Curious to hear your thoughts. Right now if you want to call this you have to create a wallet wrapper first like so. You probably shouldn't have to create a wallet to get a new PK.

const wallet = await this.BlsWalletWrapper(pk); const newPk = wallet.getRandomBlsPrivateKey();

Agree creating a new wallet to get a random private key is not optimal, unsure on the best place to put this. But could it be a static method somewhere? Similar to how we have static methods for an address on BlsWalletWrapper.

Yeah the more that I think about it, I'm gonna move it.

JohnGuilding

Looks good!

All comments are minor/non-blocking so approving

JohnGuilding · 2023-01-27T10:02:40Z

contracts/clients/src/BlsWalletWrapper.ts

+    return this.blsWalletSigner.getRandomBlsPrivateKey();
+  }
+
+  async getBundleSetRecoveryHash(


~~To check my understanding, this method sets the recovery hash and then returns 10000 that bundle?~~

Was originally thrown off by the naming here but not convinced there's anything better.

getSetRecoveryHashBundle/getRecoverWalletBundle reads more like a sentence but not a fan of "get" and "set" next to each other for the first method.

createSetRecoveryHashBundle/createRecoverWalletBundle reads better than "get" prefix to me, but prefer the "get" prefix semantically

signSetRecoveryHashOperation/signRecoverWalletOperation think this reads well but using bundle instead of operation makes more sense to me

setRecoveryHash/recoverWallet is misleading as the method is only creating the bundle and not executing it

Happy with to go with original (or one of 4 options if you think any make more sense) as I've spent far too long thinking about this lol

Yeah I spent some time thinking about this. I ended up going with what I had cause I figured getBundle{FUNCTION_NAME} could be a good naming pattern.

I think your first one might be the most readable though so I might switch to that.

JohnGuilding · 2023-01-27T10:15:05Z

contracts/clients/src/BlsWalletWrapper.ts

+    try {
+      return ethers.utils.formatBytes32String(salt);
+    } catch (e) {
+      throw new Error("Error convirting salt string to bytes 32 string.");


Making an assumption here, but is this error handling to ensure we have a more helpful error message when an invalid salt is passed in?

This is more of a question than a suggestion, but if so, would it be helpful to also surface the ethers error too as part of the error message?

Also nitpick but while I'm here, noticed a tiny typo in the error message :)

Yeah, this is a good shout. After thinking about it, this private method is unnecessary and we can just go with the default error

JohnGuilding · 2023-01-27T10:23:01Z

contracts/clients/src/signer/getRandomBlsPrivateKey.ts

+import { mcl } from "@thehubbleproject/bls";
+
+export default (): string => {
+  return `0x${mcl.randFr().serializeToHexStr()}`;


Agree creating a new wallet to get a random private key is not optimal, unsure on the best place to put this. But could it be a static method somewhere? Similar to how we have static methods for an address on BlsWalletWrapper.

JohnGuilding · 2023-01-27T10:33:17Z

extension/source/helpers/randFr.ts

-import * as mcl from 'mcl-wasm';
-import { hexlify, randomBytes } from 'ethers/lib/utils';
-
-export default async function randFr(): Promise<mcl.Fr> {


Are there any differences between this function, and the randFr function used in getRandomBlsPrivateKey?

No they are meant to do the same thing. That randFr function comes from the hubble library. I added this file a while back by copying the logic from the hubble project

gotcha cheers

JohnGuilding · 2023-01-27T10:51:51Z

contracts/clients/src/BlsWalletWrapper.ts

+    salt: string,
+    recoverWalletAddress: string,
+  ): Promise<Bundle> {
+    const saltBytes32String = BlsWalletWrapper.saltToBytes32String(salt);


What was the reason for changing references of this from saltHash/recoverySaltHash to saltBytes32String?

Don't have a strong opinion, but wondering if saltHash would read better than having the type in the name?

No particular reason. I think in a lot of my code I called it saltBytes32String to copy the function name and Kautuk called it saltHash in other places. I might switch to saltHash

JohnGuilding · 2023-01-27T11:00:18Z

contracts/test/recovery-test.ts

+    expect(await vg.hashFromWallet(wallet4.address)).to.eql(newHash);
+    expect(await vg.walletFromHash(newHash)).to.eql(wallet4.address);


General recovery question - my understanding of this scenario is that wallet 3 should not be able to send transactions after being recovered to wallet 4, is that correct?

If so, do you see value in a test that checks wallet 3 cannot send transactions after being recovered? That might be covered by existing assertions/not be desirable, curious on your thoughts here.

So in this situation, wallet 3 is actually the trusted wallet for wallet 4. I've been using the term trusted wallet recently because I think that makes more sense then what I've been calling it. In this situation, we can assume that it was actually wallet 4 that lost it's private key and needs to recover to a new one. Wallet 3 is the trusted wallet that can call the recover wallet function to swap the PK for wallet 4.

After the recovery is done, wallet 4's old private key should no longer be able to send txs. But the new private key that now maps to wallet 4's public key should be able to send transactions for that hash. That's why here I check that the public key hash for the new private key is the same as the hash that maps to wallet 4's address. If that's the case, the new private key can send txs for that address.

With regard to the test. We already have a decent amount of recovery tests in here which is why I just wanted to add a test to check that the bundle created from the new functions works. I just looked though and it doesn't look like we have a test that explicitly checks that the old private key doesn't work anymore. So I think that would be a valuable test.

I could add an extra line to this test to check that the old private key no longer maps to any public key hash. Which would mean it can't send any txs. When a recovery happens we just zero out the hash that the private key maps to in the mapping stored in the verification gateway. So that private key can't do anything anymore.

Yeah think I was confused about how this was working. I've got a good grasp now, thanks for clearing that up!

Trusted wallet as a term also makes sense. Wondering if renaming blsWallet3 to something like trustedBlsWallet would improve readability?

Makes sense in regards to additional test case.

blakecduncan · 2023-01-27T14:49:59Z

contracts/clients/src/BlsWalletWrapper.ts

@@ -120,6 +127,15 @@ export default class BlsWalletWrapper {
    return this.ExpectedAddress(verificationGateway, pubKeyHash);
  }

+  static async getRandomBlsPrivateKey(): Promise<string> {
+    await mcl.init(mcl.BN_SNARK1);


Hey @jacque006, I was curious to get your thoughts on this. I want the getRandomBlsPrivateKey function to be static but I need to make sure .init has been called on mcl. Looks like the only way we can guarantee that right now is if I put this code in the signer (I originally did that and then pulled it out because I didn't think it makes sense). Do you think it's an issue if I import the mcl library in this file?

I would try to keep any logic like this in the signer dir, and I would use @thehubbleproject/bls over a direct mcl-wasm import.

You should be able to do something like this:

./contracts/clients/signers/getRandomPrivateKey.ts

import { mcl } from "@thehubbleproject/bls"; export default function getRandomPrivateKey(): Promise<string> { await mcl.init(); return `0x${mcl.randFr().serializeToHexStr()}`; }

Then add as an export to ./contracts/clients/signers/index.ts and consume here.

You might run into issues running mcl.init twice. If that happens will have to try another way.

This worked for me. Thanks, @jacque006 !

blakecduncan added 2 commits January 25, 2023 12:11

Adding recovery methods

d173501

Returning a bundle

f9036b3

blakecduncan marked this pull request as draft January 25, 2023 14:51

github-actions bot added the clients label Jan 25, 2023

blakecduncan added 3 commits January 25, 2023 15:59

Update set recovery has to return a bundle

b0de915

Fix recovery hash function

414b527

Add new recovery tests

2f51ce8

github-actions bot added the contracts Smart contract related label Jan 26, 2023

blakecduncan added 2 commits January 26, 2023 15:16

Update function naming

f1af837

Update the extension to use the new code

8fa0ee2

github-actions bot added the extension Browser extension related label Jan 26, 2023

blakecduncan added 6 commits January 26, 2023 17:45

Remove logic to generate pk

b106715

Merge branch 'main' into addClientRecoveryMethods

d8a33a9

# Conflicts: # contracts/clients/package.json # extension/package.json # extension/yarn.lock

Update client module version

741dfe5

Remove extra change

8471bd8

Naming updates

4322d96

Fix experimental package version

449f931

github-actions bot added aggregator Aggregator backend related aggregator-proxy Aggregator proxy related labels Jan 26, 2023

Finally fixed it in extension

0821654

blakecduncan commented Jan 27, 2023

View reviewed changes

blakecduncan marked this pull request as ready for review January 27, 2023 09:19

JohnGuilding approved these changes Jan 27, 2023

View reviewed changes

PR comments

77f1638

blakecduncan commented Jan 27, 2023

View reviewed changes

Moving mcl logic back inside the signer directory

ff4f216

blakecduncan merged commit 6af93bd into main Feb 1, 2023

blakecduncan deleted the addClientRecoveryMethods branch February 1, 2023 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add client recovery methods #486

Add client recovery methods #486

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		expect(await vg.hashFromWallet(wallet4.address)).to.eql(newHash);
		expect(await vg.walletFromHash(newHash)).to.eql(wallet4.address);

Add client recovery methods #486

Add client recovery methods #486

Uh oh!

Conversation

Uh oh!

What is this PR doing?

How can these changes be manually tested?

Does this PR resolve or contribute to any issues?

Checklist

Guidelines

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!

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!

Uh oh!