Switch ./clients/deps/hubble-bls to @thehubbleproject/bls #92

jacque006 · 2022-01-12T01:21:17Z

What is this PR doing?

Switching ./contracts/clients/deps/hubble-bls to the @thehubbleproject/bls npm package.

How can these changes be manually tested?

clients: yarn test
contracts: yarn hardhat test
aggregator: ./programs/premerge.ts
extension: run/smoke test extension

Does this PR resolve or contribute to any issues?

#9

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)

TODO

Fix broken tests in clients
Fix crash in aggregator startup

contracts/clients/package.json

jacque006 · 2022-01-12T01:38:45Z

Two major issues blocking this now:

Most of the existing test cases fail due to new hex values being generated

cd ./contracts/clients && yarn build && yarn test

  index
    1) signs and verifies transaction
    2) aggregates transactions
    ✔ can aggregate transactions which already have multiple subTransactions (131ms)
    3) generates expected publicKeyStr


  1 passing (500ms)
  3 failing

  1) index
       signs and verifies transaction:

      AssertionError: expected [ Array(2) ] to deeply equal [ Array(2) ]
      + expected - actual

       [
      -  "0x0f8af80a400b731f4f2ddcd29816f296cca75e34816d466512a703631de3bb69"
      -  "0x023d76b485531a8dbc087b2d6f25563ad7f6d81d25f5f123186d0ec26da5e2d0"
      +  "0x0058b38298f3c486223de7c61f461ff3b47530d2619a383e49b298a56249e4fb"
      +  "0x0bf8beb03979073e57656af0a5bab043fe2d6bf4cbbf600f6a7190ce95fcf69c"
       ]
      
      at Context.<anonymous> (test/index.test.ts:59:38)

  2) index
       aggregates transactions:

      AssertionError: expected [ Array(2) ] to deeply equal [ Array(2) ]
      + expected - actual

       [
      -  "0x0008678ea56953fdca1b007b2685d3ed164b11de015f0a87ee844860c8e6cf30"
      -  "0x2bc51003125b2da84a01e639c3c2be270a9b93ed82498bffbead65c6f07df708"
      +  "0x091727df1b9834b31111c1d5c1e15989350de678232e46898c1c3c788e3c26ab"
      +  "0x1f69e3373d329564e875a1401c0b7a4a6f7ab3e9cf93ef73b59a1feb3286e693"
       ]
      
      at Context.<anonymous> (test/index.test.ts:105:39)

  3) index
       generates expected publicKeyStr:

      AssertionError: expected '0x027c3c0483be2722a29a0229bef64b2d8c1f8d4e954b0203d01ce342608b6eb8060c1136ac3aef9ba4b2a0272920fba5528f6e97b376c511bc746e47414a0d0411a697990758be620b0f09d3ad5bebe359964b74a29b89bdeb60671d243997fa2075298b12a51948b7a40dc69bdc91698ed95e5d2a69d04845af64aaf2eb1537' to equal '0x0127eaaf599e0e5997ebff004ae96b61afef4c35d9d67277ebd32a08bc42e2eb01dc98a8d2e5ac73933935dbb2888c6719914e86f14de30bf441b951867e27cd2f29d73e617ee30597f79b3fe567a11eb3bea1a2625ccb3fcb9635edf1d2f42927fac8b975dd299618e7f48753fdfa492f11f701445cdd7c3ca98bf5c386ec65'
      + expected - actual

      -0x027c3c0483be2722a29a0229bef64b2d8c1f8d4e954b0203d01ce342608b6eb8060c1136ac3aef9ba4b2a0272920fba5528f6e97b376c511bc746e47414a0d0411a697990758be620b0f09d3ad5bebe359964b74a29b89bdeb60671d243997fa2075298b12a51948b7a40dc69bdc91698ed95e5d2a69d04845af64aaf2eb1537
      +0x0127eaaf599e0e5997ebff004ae96b61afef4c35d9d67277ebd32a08bc42e2eb01dc98a8d2e5ac73933935dbb2888c6719914e86f14de30bf441b951867e27cd2f29d73e617ee30597f79b3fe567a11eb3bea1a2625ccb3fcb9635edf1d2f42927fac8b975dd299618e7f48753fdfa492f11f701445cdd7c3ca98bf5c386ec65
      
      at Context.<anonymous> (test/index.test.ts:171:52)

I am unsure when/what version of the hubble-bls repo was copied to ./contracts/clients/deps/hubble-bls to see what may be causing this difference.

Aggregator fails to start due to missing global document

cd ./aggregatgor && ./programs/aggregator.ts

error: Uncaught (in promise) ReferenceError: document is not defined
    at https://cdn.esm.sh/v62/[email protected]/deno/mcl-wasm.js:2:2798
    at Object.EQ.A.init (https://cdn.esm.sh/v62/[email protected]/deno/mcl-wasm.js:2:532289)
    at Object.Pe [as init] (https://cdn.esm.sh/v62/@thehubbleproject/[email protected]/deno/bls.js:2:3702)
    at Function.new (https://cdn.esm.sh/v62/@thehubbleproject/[email protected]/deno/bls.js:2:8567)
    at b5 (https://cdn.esm.sh/v62/[email protected]/deno/bls-wallet-clients.js:2:5578)
    at Function.create (file:///home/jacque006/workspaces/bls-wallet/aggregator/src/app/EthereumService.ts:60:35)
    at async app (file:///home/jacque006/workspaces/bls-wallet/aggregator/src/app/app.ts:30:27)
    at async file:///home/jacque006/workspaces/bls-wallet/aggregator/programs/aggregator.ts:6:1

@voltrevo Any ideas or help you could provide would be appreciated.

contracts/clients/package.json

contracts/clients/test/index.test.ts

jacque006 · 2022-01-22T02:28:07Z

aggregator/src/helpers/Rng.ts

 function byteToHex(byte: number): string {
  return byte.toString(16).padStart(2, "0");
 }

 function hash(data: string) {
  const byteValues = Array.from(new TextEncoder().encode(data));
-  const hex = `0x${byteValues.map(byteToHex).join("")}`;
-
-  return keccak256(hex);


This was causing our favorite err _wrapInput error in tests when generating a random private key. Note that the way it is setup now closely mirrors how the extension generates its private keys.

But... that's not a hash now. The whole idea of Rng is to provide reproducible random numbers for testing and keccak256 is the core of that. (Which could be documented better, that's on me.)

I'm surprised that this fixes the issue. I suppose it's by chance? Unless there's something more to it that I don't understand.

It seems like the issue is that hubble-bls has requirements (valid or otherwise) about what a private key can be. A hex string of the right length isn't enough. I think we need to address that directly.

thehubbleproject/hubble-bls#27 , investigating.

Fixed with @thehubbleproject/[email protected], reverted this change back to original code.

jacque006 · 2022-01-22T02:29:32Z

contracts/clients/package.json

@@ -17,15 +17,15 @@
    "publish-experimental-dry-run": "node scripts/showVersion.js >.version && npm version $(node scripts/showBaseVersion.js)-$(git rev-parse HEAD | head -c7) --allow-same-version && npm publish --tag experimental --dry-run && npm version $(cat .version) && rm .version"
  },
  "dependencies": {
-    "ethers": "^5.5.2",
-    "mcl-wasm": "0.7.6",
-    "typescript": "^4.3.5"


typescript should only be needed as a dependency to run type checking and transpilation, moved to dev deps.

-This +typescript

👍... for a minute there I was like... why is ethers a dev dependency? 🤯

jacque006 · 2022-01-22T02:31:10Z

contracts/clients/src/signer/defaultDomain.ts

@@ -1,4 +1,3 @@
-import { arrayify } from "@ethersproject/bytes";
-import { keccak256 } from "@ethersproject/keccak256";


We may end up wanting to move these back to more specific imports depending on what we decide in #109

jacque006 · 2022-01-22T02:33:02Z

contracts/clients/test/index.test.ts

-      "0x0058b38298f3c486223de7c61f461ff3b47530d2619a383e49b298a56249e4fb",
-      "0x0bf8beb03979073e57656af0a5bab043fe2d6bf4cbbf600f6a7190ce95fcf69c",
+      "0x0f8af80a400b731f4f2ddcd29816f296cca75e34816d466512a703631de3bb69",
+      "0x023d76b485531a8dbc087b2d6f25563ad7f6d81d25f5f123186d0ec26da5e2d0",


[email protected] generates new hexes in these tests. They are still valid in contract and aggregator tests.

Weird! But ok :).

jacque006 · 2022-01-22T02:34:02Z

contracts/test/recovery-test.ts

+    const newKey: PublicKey = [
+      BigNumber.from(1),
+      BigNumber.from(2),
+      BigNumber.from(3),
+      BigNumber.from(4),
+    ];


New Typescript version caught this. Is there a better way we could to this?

New Typescript version caught this.

I don't think that's what happened :/. The issue is that the type of .map works on arrays, not tuples, so using .map converts your tuple into an array, so the assignment absolutely shouldn't work. This hasn't changed, I checked back to 3.3.3 and it still 'correctly' rejects constructions like this:

https://www.typescriptlang.org/play?ts=3.3.3#code/C4TwDgpgBACg0lAvFA2gOwK4FsBGEBOANFJrgcaXkSdlQLoDcAUEwGYZoDGwAlgPZoowPgDlaBABQBnYPgBcUGfh5oA5gEoFlAlADeTKIaj4IwDPkFiy+abPXMAvi04CZUMAGsF8JKgDkAIx+xH4ATMFQfgDMEX4ALH50AHRYAIZgEsJWVPZMQA

I'm curious how this happened. Any ideas?

...the presence of CI would have been useful here :(

Anyway, in terms of a better way, I don't think you can. I mean, I think it's possible to write a map function that is tuple-aware, but I don't think it's worth doing here.

Right, we could do ...map<PublicKey>(...) or something similar, but better to be explicit.

I attempted to update your example to more closely match a BigNumber example but still get the same error. Unsure what changed to now get that error.

+1 Getting some CI processes in place, can prioritize after current round of work.

jacque006 · 2022-01-22T02:40:17Z

aggregator/programs/premerge.ts

@@ -1,4 +1,4 @@
-#!/usr/bin/env -S deno run --unstable --allow-run --allow-read --allow-write
+#!/usr/bin/env -S deno run --unstable --allow-run --allow-read --allow-write --allow-env


--allow-env now appears to be required for most scripts. Unsure what change did this, looks to be Deno std lib related.

jacque006 · 2022-01-22T02:42:59Z

aggregator/deps.ts

@@ -40,17 +40,17 @@ export type {
  PublicKey,
  Signature,
  VerificationGateway,
-} from "https://esm.sh/[email protected].3";
+} from "https://esm.sh/[email protected].4-05c23aa";


With the rebase workflow I prefer to use to consolidate commits, this will never match the merged commit unless I append it afterwards.

@voltrevo You've had some confusion in the past based on this, thoughts?

Mmm. I understand the issue you're referring to but not the preference you're indicating above.

I think it makes sense to republish with your final changes anyway (why use a commit that represents an incomplete PR?), but let's have a chat to straighten this out.

Just realized you can't use the final commit if you're trying to use that commit id to update the version being used. Yeah let's chat.

@voltrevo I tried our discussed process of pinning experimental version in last commit, let me know how it looks.

voltrevo

A few things here.

I also tested this with the extension (const useNewUI = false; in Popup.tsx) and I'm getting the err _wrapInput issue. Need to fix that before this can go ahead.

On the bright side, I did manually switch to a valid private key value and was able to confirm that the transfers do still work even though we're generating different signatures. Wild.

voltrevo · 2022-01-24T04:47:47Z

aggregator/src/helpers/Rng.ts

 function byteToHex(byte: number): string {
  return byte.toString(16).padStart(2, "0");
 }

 function hash(data: string) {
  const byteValues = Array.from(new TextEncoder().encode(data));
-  const hex = `0x${byteValues.map(byteToHex).join("")}`;
-
-  return keccak256(hex);


But... that's not a hash now. The whole idea of Rng is to provide reproducible random numbers for testing and keccak256 is the core of that. (Which could be documented better, that's on me.)

I'm surprised that this fixes the issue. I suppose it's by chance? Unless there's something more to it that I don't understand.

It seems like the issue is that hubble-bls has requirements (valid or otherwise) about what a private key can be. A hex string of the right length isn't enough. I think we need to address that directly.

voltrevo · 2022-01-24T05:05:06Z

contracts/clients/package.json

@@ -17,15 +17,15 @@
    "publish-experimental-dry-run": "node scripts/showVersion.js >.version && npm version $(node scripts/showBaseVersion.js)-$(git rev-parse HEAD | head -c7) --allow-same-version && npm publish --tag experimental --dry-run && npm version $(cat .version) && rm .version"
  },
  "dependencies": {
-    "ethers": "^5.5.2",
-    "mcl-wasm": "0.7.6",
-    "typescript": "^4.3.5"


-This +typescript

👍... for a minute there I was like... why is ethers a dev dependency? 🤯

voltrevo · 2022-01-24T05:08:08Z

contracts/clients/test/index.test.ts

-      "0x0058b38298f3c486223de7c61f461ff3b47530d2619a383e49b298a56249e4fb",
-      "0x0bf8beb03979073e57656af0a5bab043fe2d6bf4cbbf600f6a7190ce95fcf69c",
+      "0x0f8af80a400b731f4f2ddcd29816f296cca75e34816d466512a703631de3bb69",
+      "0x023d76b485531a8dbc087b2d6f25563ad7f6d81d25f5f123186d0ec26da5e2d0",


Weird! But ok :).

voltrevo · 2022-01-24T05:27:06Z

contracts/test/recovery-test.ts

+    const newKey: PublicKey = [
+      BigNumber.from(1),
+      BigNumber.from(2),
+      BigNumber.from(3),
+      BigNumber.from(4),
+    ];


New Typescript version caught this.

I don't think that's what happened :/. The issue is that the type of .map works on arrays, not tuples, so using .map converts your tuple into an array, so the assignment absolutely shouldn't work. This hasn't changed, I checked back to 3.3.3 and it still 'correctly' rejects constructions like this:

https://www.typescriptlang.org/play?ts=3.3.3#code/C4TwDgpgBACg0lAvFA2gOwK4FsBGEBOANFJrgcaXkSdlQLoDcAUEwGYZoDGwAlgPZoowPgDlaBABQBnYPgBcUGfh5oA5gEoFlAlADeTKIaj4IwDPkFiy+abPXMAvi04CZUMAGsF8JKgDkAIx+xH4ATMFQfgDMEX4ALH50AHRYAIZgEsJWVPZMQA

I'm curious how this happened. Any ideas?

...the presence of CI would have been useful here :(

Anyway, in terms of a better way, I don't think you can. I mean, I think it's possible to write a map function that is tuple-aware, but I don't think it's worth doing here.

jacque006 · 2022-01-24T23:40:39Z

@voltrevo I assumed the extension would Just Work ™️ given the other changes, good catch. Will triage and fix.

Switch ./clients/deps/hubble-bls to @thehubbleproject/bls. Clean up import paths in clients. Update hex values in clients tests with new mcl-wasm values. Add and expose signMessage function to wallet. Update clients package in aggregator and fix private key generation in tests. Update contract tests. Update all ethers.js versions to v5.5.4.

Pin experimental [email protected] to aggregator & extension.

jacque006 · 2022-03-15T22:04:27Z

contracts/clients/src/BlsWalletWrapper.ts

+    const [proxyAdminAddress, blsWalletLogicAddress] = await Promise.all([
+      verificationGateway.walletProxyAdmin(),
+      verificationGateway.blsWalletLogic(),
+    ]);


jacque006 · 2022-03-15T22:05:10Z

contracts/clients/src/BlsWalletWrapper.ts

+  /** Sign a message */
+  signMessage(message: string): Signature {
+    return this.blsWalletSigner.signMessage(message, this.privateKey);
+  }


This seemed appropriate to expose, as someone may need to use it during the wallet recovery/upgrade process.

jacque006 · 2022-03-15T22:08:24Z

contracts/package.json

@@ -38,7 +36,7 @@
    "eslint-plugin-prettier": "^3.4.0",
    "eslint-plugin-promise": "^5.1.0",
    "ethereum-waffle": "^3.0.0",
-    "ethers": "^5.5.1",
+    "ethers": "5.5.4",


[email protected] makes everything very sad, explicitly pinned 5.5.4 until someone can investigate upgrade path, likely in #160

jacque006 · 2022-03-15T22:10:47Z

@voltrevo Needed changes made, fully retested. Ready for your re-review.

For the extension, I made sure the old UI path properly generated a key but was unable to confirm the txn send as that path is now tied with extension dApp work.

jacque006 requested a review from voltrevo January 12, 2022 01:21

github-actions bot added aggregator Aggregator backend related clients labels Jan 12, 2022

jacque006 commented Jan 12, 2022

View reviewed changes

contracts/clients/package.json Outdated Show resolved Hide resolved

jacque006 commented Jan 12, 2022

View reviewed changes

contracts/clients/package.json Outdated Show resolved Hide resolved

jacque006 force-pushed the update-hubble-bls branch from 532c32b to 9050be1 Compare January 19, 2022 04:47

github-actions bot added the contracts Smart contract related label Jan 19, 2022

jacque006 commented Jan 19, 2022

View reviewed changes

contracts/clients/package.json Outdated Show resolved Hide resolved

jacque006 commented Jan 19, 2022

View reviewed changes

contracts/clients/test/index.test.ts Show resolved Hide resolved

jacque006 force-pushed the update-hubble-bls branch from d697f5f to 1900704 Compare January 22, 2022 02:20

jacque006 commented Jan 22, 2022

View reviewed changes

jacque006 changed the title ~~Switch clients/deps/hubble-bls to @thehubbleproject/bls~~ Switch ./clients/deps/hubble-bls to @thehubbleproject/bls Jan 22, 2022

jacque006 commented Jan 22, 2022

View reviewed changes

jacque006 force-pushed the update-hubble-bls branch from 1900704 to 3785fec Compare January 22, 2022 02:41

jacque006 commented Jan 22, 2022

View reviewed changes

jacque006 marked this pull request as ready for review January 22, 2022 02:43

jacque006 requested a review from jzaki January 22, 2022 02:43

voltrevo suggested changes Jan 24, 2022

View reviewed changes

jacque006 marked this pull request as draft February 24, 2022 22:03

jacque006 force-pushed the update-hubble-bls branch 2 times, most recently from 4fdb312 to 2f1ef6c Compare March 15, 2022 01:21

github-actions bot added the extension Browser extension related label Mar 15, 2022

jacque006 force-pushed the update-hubble-bls branch from 24481fe to 47f2af8 Compare March 15, 2022 21:33

jacque006 force-pushed the update-hubble-bls branch from 47f2af8 to f2bad77 Compare March 15, 2022 21:51

Pin [email protected]

82cc9c7

Pin experimental [email protected] to aggregator & extension.

jacque006 commented Mar 15, 2022

View reviewed changes

jacque006 marked this pull request as ready for review March 15, 2022 22:08

voltrevo approved these changes Mar 22, 2022

View reviewed changes

voltrevo merged commit 7dbf86b into main Mar 22, 2022

voltrevo deleted the update-hubble-bls branch March 22, 2022 23:16

jacque006 mentioned this pull request Apr 7, 2022

Try to use hubble-bls directly instead of using copy #9

Closed

voltrevo mentioned this pull request Jun 15, 2022

Cells #221

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch ./clients/deps/hubble-bls to @thehubbleproject/bls #92

Switch ./clients/deps/hubble-bls to @thehubbleproject/bls #92

jacque006 commented Jan 12, 2022 •

edited

Loading

jacque006 commented Jan 12, 2022

jacque006 Jan 22, 2022

voltrevo Jan 24, 2022

jacque006 Jan 25, 2022

jacque006 Mar 15, 2022

jacque006 Jan 22, 2022 •

edited

Loading

voltrevo Jan 24, 2022

jacque006 Jan 22, 2022

jacque006 Jan 22, 2022

voltrevo Jan 24, 2022

jacque006 Jan 22, 2022

voltrevo Jan 24, 2022

jacque006 Jan 24, 2022

jacque006 Jan 22, 2022

jacque006 Jan 22, 2022

voltrevo Jan 24, 2022 •

edited

Loading

voltrevo Jan 24, 2022

jacque006 Mar 15, 2022

voltrevo left a comment

voltrevo Jan 24, 2022

voltrevo Jan 24, 2022

voltrevo Jan 24, 2022

voltrevo Jan 24, 2022

jacque006 commented Jan 24, 2022

jacque006 Mar 15, 2022

jacque006 Mar 15, 2022

jacque006 Mar 15, 2022

jacque006 commented Mar 15, 2022

		@@ -1,4 +1,3 @@
		import { arrayify } from "@ethersproject/bytes";
		import { keccak256 } from "@ethersproject/keccak256";

		@@ -1,4 +1,4 @@
		#!/usr/bin/env -S deno run --unstable --allow-run --allow-read --allow-write
		#!/usr/bin/env -S deno run --unstable --allow-run --allow-read --allow-write --allow-env

Switch ./clients/deps/hubble-bls to @thehubbleproject/bls #92

Switch ./clients/deps/hubble-bls to @thehubbleproject/bls #92

Conversation

jacque006 commented Jan 12, 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

TODO

jacque006 commented Jan 12, 2022

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 Jan 22, 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

voltrevo Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voltrevo 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

jacque006 commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacque006 commented Mar 15, 2022

jacque006 commented Jan 12, 2022 •

edited

Loading

jacque006 Jan 22, 2022 •

edited

Loading

voltrevo Jan 24, 2022 •

edited

Loading