Ethereum Group PCD #290

djma · 2023-07-04T04:07:37Z

Subsumes #287

apps/consumer-client/pages/examples/add-pcd.tsx

djma · 2023-07-04T04:09:49Z

apps/consumer-client/pages/examples/add-pcd.tsx

+/**
+ * The offset that the hex representation of the public key starts at, without the 0x prefix and without the 04 prefix. That's what the zk-circuit expects.
+ * https://github.com/indutny/elliptic/issues/86
+ * https://dev.to/q9/finally-understanding-ethereum-accounts-1kpe
+ */
+const hexPubkeyOffset = 2 + 2;
+
+function AddEthGroupPCDButton() {


A good chunk of code here is duplicated in EthereumGroupPCD.spec.ts. I tried to refactor it and put this example code in EthereumGroupPCD.ts but ran into problems in EthereumGroupPCD.init(). I'm leaving it like this for your review.

apps/consumer-client/pages/examples/add-pcd.tsx

apps/passport-client/components/screens/ProveScreen/GenericProveSection.tsx

djma · 2023-07-04T04:10:42Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+const isNode =
+  typeof process !== "undefined" &&
+  process.versions != null &&
+  process.versions.node != null;


Not sure if this is kosher

I recommend passing a parameter into the init function via EthereumGroupPCDInitArgs called something like proverConfigOverride, which you set only in the browser, so you don't have to do this isNode checking in the PCD package.

djma · 2023-07-04T04:10:52Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+} else {
+  addrMembershipConfig = {
+    circuit:
+      "https://storage.googleapis.com/personae-proving-keys/membership/addr_membership.circuit",
+    witnessGenWasm:
+      "https://storage.googleapis.com/personae-proving-keys/membership/addr_membership.wasm",
+  };
+
+  pubkeyMembershipConfig = {
+    circuit:
+      "https://storage.googleapis.com/personae-proving-keys/membership/pubkey_membership.circuit",
+    witnessGenWasm:
+      "https://storage.googleapis.com/personae-proving-keys/membership/pubkey_membership.wasm",
+  };


Warning from spartan-ecdsa:

Spartan-ecdsa default config warning: We recommend using defaultPubkeyMembershipPConfig/defaultPubkeyMembershipVConfig only for testing purposes. Please host and specify the circuit and witnessGenWasm files on your own server for sovereign control. Download files: https://github.com/personaelabs/spartan-ecdsa/blob/main/packages/lib/README.md#circuit-downloads

We can host these on our server here to avoid this warning

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

ichub · 2023-07-06T14:11:04Z

README.md

+- [`@pcd/ethereum-ownership-pcd`](packages/ethereum-ownership-pcd): a pcd that can be used to claim that a Semaphore identity knows the private key of an ethereum address.
+- [`@pcd/ethereum-group-pcd`](packages/ethereum-group-pcd): a pcd that can be used to claim that a Semaphore identity knows the private key of an ethereum address or public key that is part of a merkle group of addresses or public keys, without revealing the specific one.


apps/consumer-client/pages/examples/add-pcd.tsx

ichub · 2023-07-06T14:21:27Z

apps/consumer-client/pages/examples/add-pcd.tsx

+          value: undefined,
+          userProvided: true,
+          description:
+            "The Semaphore Identity which you are signing the message.",


Suggested change

"The Semaphore Identity which you are signing the message.",

"The Semaphore Identity with which you are signing the message.",

the message

on first read, it's confusing to understand what 'the message' is. I'd recommend changing the description to something like

"The Semaphore Identity which you are proving owns the given Ethereum address."

ichub · 2023-07-06T15:33:52Z

packages/ethereum-group-pcd/test/EthereumGroupPCD.spec.ts

+    },
+    groupType: {
+      argumentType: ArgumentTypeName.String,
+      value: GroupType.PUBLICKEY,


you should probably also have a happy path for the address group type

ichub · 2023-07-06T15:35:26Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+}
+
+export enum GroupType {
+  PUBLICKEY = "publickey",


PUBLIC_KEY = "public_key"

packages/ethereum-group-pcd/test/EthereumGroupPCD.spec.ts

ichub · 2023-07-06T15:37:57Z

apps/consumer-client/pages/examples/add-pcd.tsx

@@ -1,18 +1,26 @@
+import { EthereumGroupPCDPackage, GroupType } from "@pcd/ethereum-group-pcd";


I'm not sure how this works on your computer, but I needed to add a dependency on this package in the package.json of consumer-client

ichub · 2023-07-06T15:38:53Z

apps/passport-client/src/pcdPackages.ts

you should make an equivalent update to apps/passport-server/src/services/provingService.ts

in startProvingService? what about all the other pcds?

apps/passport-client/src/pcdPackages.ts

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

ichub · 2023-07-17T09:07:01Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+  addrProver = new MembershipProver(addrMembershipConfig);
+  pubkeyProver = new MembershipProver(pubkeyMembershipConfig);
+  addrVerifier = new MembershipVerifier(addrMembershipConfig);
+  pubkeyVerifier = new MembershipVerifier(pubkeyMembershipConfig);
+  await Promise.all([
+    addrProver.initWasm(),
+    pubkeyProver.initWasm(),
+    addrVerifier.initWasm(),
+    pubkeyVerifier.initWasm(),
+  ]);


ichub · 2023-07-17T09:07:54Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+ *
+ * https://dev.to/q9/finally-understanding-ethereum-accounts-1kpe
+ */
+export function getRawPubKeyBuffer(pubKey: string): Buffer {


ichub · 2023-07-17T09:08:38Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+  }
+
+  if (
+    [GroupType.ADDRESS, GroupType.PUBLICKEY].includes(args.groupType as any)


Suggested change

[GroupType.ADDRESS, GroupType.PUBLICKEY].includes(args.groupType as any)

![GroupType.ADDRESS, GroupType.PUBLICKEY].includes(args.groupType as any)

you should run all your code :P

ah.. it was a double error. Missing the ! and missing the .value, which led to tests passing.

ichub · 2023-07-17T09:10:13Z

packages/ethereum-group-pcd/src/EthereumGroupPCD.ts

+    )
+  );
+  parsed.claim.publicInput = publicInput;
+  return parsed;


One issue with this parse function is that it's not invoking the constructor of EthereumGroupPCD, but creating an object that conforms to its type signature. Not a huge deal right now but feels kind of wrong. Checking the way I do this in other packages and it seems that this is a pervasive error. Here's an issue to fix at some point later. #308

ichub · 2023-07-17T09:13:44Z

packages/ethereum-group-pcd/test/EthereumGroupPCD.spec.ts

+    newEthGroupPCD.claim.publicInput.circuitPubInput.Uy =
+      newEthGroupPCD.claim.publicInput.circuitPubInput.Uy - BigInt(1);
+
+    assert(await EthereumGroupPCDPackage.verify(newEthGroupPCD));


ichub · 2023-07-17T09:15:05Z

packages/ethereum-group-pcd/test/EthereumGroupPCD.spec.ts

+    assert(await EthereumGroupPCDPackage.verify(ethGroupPCD));
+  });
+
+  it("should work with address (slow ~40s)", async function () {


ichub · 2023-07-18T12:30:09Z

I'm getting the following error when I run the code on my computer and open up the passport app:

ichub

lgtm! last thing left is to resolve the yarn.lock conflicts

Onboarding local dev fixes Fix passport-server:dev concurrently concurrently Ethereum Group PCD Add test more test tests progress Ethereum Address Ownership example in add-pcd.tsx small fix switch to merkleproof eth group pcd in add-pcd smol fix small rename Fix ethereum ownership tests and ethereum group ownership tests remove useNativeBigInt address PR comments address pr comments

djma commented Jul 4, 2023

View reviewed changes

djma force-pushed the djma/ethgrouppcd branch from 086eda9 to 1989665 Compare July 4, 2023 04:21

ichub reviewed Jul 6, 2023

View reviewed changes

djma requested a review from ichub July 13, 2023 23:23

ichub reviewed Jul 17, 2023

View reviewed changes

apps/passport-client/src/pcdPackages.ts Show resolved Hide resolved

ichub reviewed Jul 17, 2023

View reviewed changes

djma force-pushed the djma/ethgrouppcd branch from 7b15d63 to cc70924 Compare July 17, 2023 21:36

djma requested a review from ichub July 17, 2023 21:36

ichub approved these changes Jul 25, 2023

View reviewed changes

djma added 3 commits July 26, 2023 10:57

Fix __dirname is not defined

1fe7960

fix yarn dev:node

65b88e2

djma force-pushed the djma/ethgrouppcd branch from 419114b to 65b88e2 Compare July 26, 2023 17:57

ichub merged commit 959b0dd into proofcarryingdata:main Jul 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ethereum Group PCD #290

Ethereum Group PCD #290

djma commented Jul 4, 2023 •

edited

Loading

djma Jul 4, 2023

djma Jul 4, 2023

ichub Jul 6, 2023

djma Jul 4, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

ichub Jul 6, 2023

djma Jul 13, 2023

ichub Jul 17, 2023

ichub Jul 17, 2023

ichub Jul 17, 2023

ichub Jul 17, 2023

djma Jul 17, 2023

ichub Jul 17, 2023

ichub Jul 17, 2023

ichub Jul 17, 2023

ichub commented Jul 18, 2023

ichub left a comment

		- [`@pcd/ethereum-ownership-pcd`](packages/ethereum-ownership-pcd): a pcd that can be used to claim that a Semaphore identity knows the private key of an ethereum address.
		- [`@pcd/ethereum-group-pcd`](packages/ethereum-group-pcd): a pcd that can be used to claim that a Semaphore identity knows the private key of an ethereum address or public key that is part of a merkle group of addresses or public keys, without revealing the specific one.

	"The Semaphore Identity which you are signing the message.",
	"The Semaphore Identity with which you are signing the message.",

		@@ -1,18 +1,26 @@
		import { EthereumGroupPCDPackage, GroupType } from "@pcd/ethereum-group-pcd";

	[GroupType.ADDRESS, GroupType.PUBLICKEY].includes(args.groupType as any)
	![GroupType.ADDRESS, GroupType.PUBLICKEY].includes(args.groupType as any)

Ethereum Group PCD #290

Ethereum Group PCD #290

Conversation

djma commented Jul 4, 2023 • 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

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

Choose a reason for hiding this comment

ichub commented Jul 18, 2023

ichub left a comment

Choose a reason for hiding this comment

djma commented Jul 4, 2023 •

edited

Loading