Feat/scaffold coordinator service api calls #11

JohnGuilding · 2025-05-14T21:17:51Z

What does this PR do:

Create functions that call all of the coordinator service endpoints required to finalise a poll
- merge
- generateProofs
- submit
Add required helper functions from coordinator service to get things building, such as getSigner, getAuthorizationHeader
Add zod for api request return types & runtime validation
Make a note of where files/logic is copied
- Also note where changes were made

Open questions

I'm not 100% familiar with the configuration required, I'm pretty sure some of what I've written will be wrong, so let me know if you can see any mistakes that should be fixed in this PR
Also not 100% how authentication should be handled, my thinking was to solve that in a follow up PR
Added minimum required utilities to get everything to build with the 3 finalise functions, but let me know if I missed any

Other notes

I wanted to keep this PR as small as possible, so didn't make it an e2e PR that could be tested. My thinking was that I could at least solve authentication and add any remaining utilities in a follow up PR. That said, please feel free to suggest any improvements here or other suggestions for next steps.

There is a fair amount of copied code in this PR, the new logic is mainly in coordinator.ts. Spoke with Nico and we discussed that we would look to potentially export a lot of the logic that the coordinator uses into an sdk to that devs can just import those instead of copying them over which is what I'm doing now. We agreed that seemed out of scope for now, but let me know if you wanted me to do that first - only issue is that is would be time consuming given we want to get the demo out soon.

JohnGuilding · 2025-05-14T21:35:07Z

.eslintrc.cjs

@@ -18,6 +18,7 @@ module.exports = {
    "@typescript-eslint/no-explicit-any": "off",
    "@typescript-eslint/no-unused-vars": ["warn", { ignoreRestSiblings: true }],
    "@typescript-eslint/prefer-nullish-coalescing": "warn",
+    "@typescript-eslint/no-duplicate-enum-values": "off",


I appeared to be getting a false positive here where this warning would throw during pre-commit hooks for the new enums, even though they don't seem to have duplicate values. Adding this to each file also did not work so I was a bit confused why it was getting thrown

So disabling the rule here worked whereas disabling it in the file did not

I'd not disable this eslint rule. Better to fix the issues related to it. It can't be false positive, something can be related to mixed default and defined values.

JohnGuilding · 2025-05-14T21:36:44Z

plugins/maciVoting/services/coordinator/config.ts

Didn't give this too much thought as wasn't sure about a lot of the values. Please let me know if you see anything that is definitely wrong

NicoSerranoP · 2025-05-14T21:49:32Z

Thank you John! It is looking good, I will add some comments 😊

NicoSerranoP

Thank you @JohnGuilding I left some comments, try reading them from the "Conversation" tab so they are in order (my idea was to leave the main file /coordinator/coordinator.ts to the end :)

plugins/maciVoting/services/coordinator/config.ts

plugins/maciVoting/services/coordinator/utils/auth.ts

plugins/maciVoting/services/coordinator/utils/types.ts

plugins/maciVoting/services/coordinator/coordinator.ts

0xmad

@JohnGuilding thanks, left some comments

0xmad · 2025-05-14T22:37:14Z

.eslintrc.cjs

@@ -18,6 +18,7 @@ module.exports = {
    "@typescript-eslint/no-explicit-any": "off",
    "@typescript-eslint/no-unused-vars": ["warn", { ignoreRestSiblings: true }],
    "@typescript-eslint/prefer-nullish-coalescing": "warn",
+    "@typescript-eslint/no-duplicate-enum-values": "off",


I'd not disable this eslint rule. Better to fix the issues related to it. It can't be false positive, something can be related to mixed default and defined values.

plugins/maciVoting/services/coordinator/config.ts

0xmad · 2025-05-14T22:38:15Z

plugins/maciVoting/services/coordinator/coordinator.ts

+
+type GenerateResponse = z.infer<typeof GenerateResponseSchema>;
+type SubmitResponse = z.infer<typeof SubmitResponseSchema>;
+type CoordinatorServiceResult<T, E = Error> = { success: true; data: T } | { success: false; error: E };


It should be an interface, not type

z.infer returns type not an interface. Might remove zod anyway as you can argue it is over-engineering

Do you guys have a style guide or anything that you refer to? Just curious to understand the patterns/preferences you follow so I know for the future

I don't think we have a specific guideline but we should build one 😊 . Most of it is guided by:

Clean code

Contributing to MACI

Instead of types we prefer using interfaces, it's just an agreement (not written somewhere yet).
So you can use it like this:

interface ICoordinatorServiceResult<T, E = Error> { success: boolean; data?: T; error?: E; }

Sure, switched to using an interface.

Curious to get your thoughts on the following:

export type CoordinatorServiceResult<T, E = Error> = { success: true; data: T } | { success: false; error: E }; const usingDiscriminatedUnion: CoordinatorServiceResult<boolean> = { success: true, error: new Error("I'm an error"), // This does not compile }; export interface ICoordinatorServiceResult<T, E = Error> { success: boolean; data?: T; error?: E; } const usingInterface: ICoordinatorServiceResult<boolean> = { success: true, error: new Error("I'm an error"), // This compiles };

The discriminated union type here provides a bit more flexibility and type safety in this example. For return types, I think this could be useful. Do you have a preference for this or would you rather stick with interfaces?

Actually, for this case you can keep the type but move inline types to interfaces:

interface ISuccessResult<T> { success: true; data: T; } interface IErrorResult<E = Error> { success: false; error: E; }

Or you can get rid of success property and check if there is data or error

0xmad · 2025-05-14T22:39:34Z

plugins/maciVoting/services/coordinator/coordinator.ts

+  let response: Response;
+  try {
+    response = await fetch(`${BASE_URL}/proof/merge`, {


Do not use try/catch like this. You can use it like fetch(...).catch((error) => ...)

Sure, done. Out of interest, why is there a preference for this way of doing things over try catch?

It's just a more functional approach instead of procedural. With catch method you don't need to have a mutation for response and keep in const, and it makes code cleaner because you can't return the value from catch method so you need to explicitly handle it outside which makes it more obvious when you read the code (not a good practice to have some logic handling in catch).

you don't need to have a mutation for response and keep in const

I like this benefit

makes code cleaner because you can't return the value from catch method so you need to explicitly handle it outside which makes it more obvious when you read the code (not a good practice to have some logic handling in catch).

Ok yeah I see how this can be cleaner

Thanks for the explanation

0xmad · 2025-05-14T22:40:42Z

plugins/maciVoting/services/coordinator/coordinator.ts

+export const merge = async (
+  maciContractAddress: Address,
+  pollId: number,
+  approval: string,
+  sessionKeyAddress: Address
+): Promise<CoordinatorServiceResult<boolean>> => {


Use one object instead of four separate params

These args have changed now but have done this where multiple args still exist

So this is the idea that if I want to add new argument, I just add a new property to the object, plus I don't need to worry about the order of the arguments. For instance first option looks better than second one:

merge({ maciContractAddress: "0x...", pollId: 0n, approval: "321...", sessionKeyAddress: "123..." }); merge("0x...", 0n, "321...", "123...");

Got it. Yeah I agree named arguments are cleaner - thanks for pointing this out!

0xmad · 2025-05-14T22:41:32Z

plugins/maciVoting/services/coordinator/coordinator.ts

+    startBlock: Number(blockNumber) - 100,
+    endBlock: Number(blockNumber) + 100,


Why are these values hardcoded?

I copied and pasted code from the coordinator service. Assume these are wrong, I guess I should consume them as arguments into the function?

These values are used to generate a MACI state that would later be used to generate a proof. In our context startBlock and endBlock would be variables. startBlock would be the proposal snapshot block and the end block will be the getFutureBlockNumberAtTimestamp(pollEndDate)

I suggest we send these values as arguments into the function generateProofs 😊

Added as arguments!

But why is 100 hardcoded?

@0xmad here is where I originally copied them from. I'm not sure why 100 was chosen but I assume they were deemed as acceptable e2e test values

I think it should be at least moved as a const or to config (.env) and I don't know why this +/- 100 blocks are on tests.

Would something like this work? Or would you want to move the 100 values themselves to config? Something else?

test("should generate proofs correctly", async () => { const blockNumber = await publicClient.getBlockNumber(); const startBlock = Number(blockNumber) - 100; const endBlock = Number(blockNumber) + 100; const encryptedCoordinatorPrivateKey = await encryptWithCoordinatorRSAPublicKey( coordinatorMACIKeypair.privateKey.serialize(), ); const response = await fetch(`${TEST_URL}/proof/generate`, { method: "POST", headers: { Authorization: encryptedHeader, "Content-Type": "application/json", }, body: JSON.stringify({ poll: Number(pollId), maciContractAddress: maciAddress, mode: testPollDeploymentConfig.mode, encryptedCoordinatorPrivateKey, startBlock, endBlock, blocksPerBatch: 20, chain: CHAIN, } as IGenerateArgs), });

0xmad · 2025-05-14T22:41:43Z

plugins/maciVoting/services/coordinator/coordinator.ts

+  let response: Response;
+  try {
+    response = await fetch(`${BASE_URL}/proof/generate`, {


Same here for try/catch

0xmad · 2025-05-14T22:42:04Z

plugins/maciVoting/services/coordinator/coordinator.ts

+export const submit = async (
+  pollId: number,
+  maciContractAddress: Address,
+  approval: string,
+  sessionKeyAddress: Address
+): Promise<CoordinatorServiceResult<SubmitResponse>> => {


Same here for one object param

0xmad · 2025-05-14T22:42:14Z

plugins/maciVoting/services/coordinator/coordinator.ts

+  approval: string,
+  sessionKeyAddress: Address
+): Promise<CoordinatorServiceResult<SubmitResponse>> => {
+  const args: ISubmitProofsArgs = {


No need to specify type here

0xmad · 2025-05-14T22:42:29Z

plugins/maciVoting/services/coordinator/coordinator.ts

+  let response: Response;
+  try {
+    response = await fetch(`${BASE_URL}/proof/submit`, {


Same here for try/catch

JohnGuilding · 2025-05-15T23:34:08Z

I'd not disable this eslint rule. Better to fix the issues related to it. It can't be false positive, something can be related to mixed default and defined values.

yeah good point, since I deleted the code based on Nico's comments I could remove this easily

JohnGuilding · 2025-05-15T23:35:51Z

@NicoSerranoP @0xmad addressed your comments or left a follow up question. let me know if I missed anything!

NicoSerranoP · 2025-05-16T05:44:04Z

plugins/maciVoting/contexts/CoordinatorContext.tsx

+
+// TODO: move to env
+const BASE_URL = "http://localhost:3000/v1";
+const MACI_CONTRACT_ADDRESS = "0x0000000000000000000000000000000000000000";


Instead of MACI_CONTRACT_ADDRESS lets use 🚀

import { PUB_MACI_ADDRESS } from "@/constants";

Cheers done

Please, use PUBLIC_MACI_ADDRESS name instead

Add schemas.ts Add errors.ts Add final TODOs Note copied code

The component with the lint error did not even use props, so removed the interface

0xmad · 2025-05-16T15:32:18Z

constants.ts

@@ -17,6 +17,9 @@ export const PUB_MACI_ADDRESS = (process.env.NEXT_PUBLIC_MACI_ADDRESS ?? "") as
 export const PUB_MACI_DEPLOYMENT_BLOCK = Number(process.env.NEXT_PUBLIC_MACI_DEPLOYMENT_BLOCK ?? 0);
 export const NEXT_PUBLIC_SECONDS_PER_BLOCK = Number(process.env.NEXT_PUBLIC_SECONDS_PER_BLOCK ?? 1); // ETH Mainnet block takes ~12s

+// MACI Coordinator service
+export const PUB_COORDINATOR_SERVICE_URL = process.env.NEXT_PUBLIC_COORDINATOR_SERVICE_URL ?? "";


Suggested change

export const PUB_COORDINATOR_SERVICE_URL = process.env.NEXT_PUBLIC_COORDINATOR_SERVICE_URL ?? "";

export const PUBLIC_COORDINATOR_SERVICE_URL = process.env.NEXT_PUBLIC_COORDINATOR_SERVICE_URL ?? "";

Let's do it for all the constants here

0xmad · 2025-05-16T15:35:20Z

plugins/maciVoting/contexts/CoordinatorContext.tsx

+      }).catch((error) => {
+        throw error instanceof Error ? error : new Error(String(error));
+      });


Is there a case where you can have string error instead of Error?
If no, you can just remove this catch (and other catches too).

I'm not entirely sure but I agree that this could be redundant, removed catch blocks

0xmad · 2025-05-16T15:36:10Z

plugins/maciVoting/contexts/CoordinatorContext.tsx

+  const publicClient = usePublicClient();
+  const { signMessageAsync } = useSignMessage();
+
+  const getPublicKey = async (): Promise<KeyLike> => {


Let's use useCallback here and add it to the deps for other useCallbacks

0xmad · 2025-05-16T15:37:40Z

plugins/maciVoting/contexts/CoordinatorContext.tsx

+    }: {
+      pollId: number;
+      encryptedCoordinatorPrivateKey: string;
+      startBlock: number;
+      endBlock: number;
+    }): Promise<CoordinatorServiceResult<GenerateResponse>> => {


Let's move it to the interface

0xmad · 2025-05-16T15:38:39Z

plugins/maciVoting/contexts/types.ts

+  }: {
+    pollId: number;
+    encryptedCoordinatorPrivateKey: string;
+    startBlock: number;
+    endBlock: number;
+  }) => Promise<CoordinatorServiceResult<GenerateResponse>>;


Let's move it to the interface and reuse

0xmad · 2025-05-16T15:38:57Z

plugins/maciVoting/contexts/types.ts

+export type SubmitResponse = z.infer<typeof SubmitResponseSchema>;
+export type CoordinatorServiceResult<T, E = Error> = { success: true; data: T } | { success: false; error: E };
+
+export interface CoordinatorContextType {


Suggested change

export interface CoordinatorContextType {

export interface ICoordinatorContextType {

0xmad · 2025-05-16T15:39:03Z

plugins/maciVoting/contexts/types.ts


 export interface IVoteArgs {
  voteOptionIndex: bigint;
  newVoteWeight: bigint;
 }

+export type GenerateResponse = z.infer<typeof GenerateResponseSchema>;


Suggested change

export type GenerateResponse = z.infer<typeof GenerateResponseSchema>;

export type TGenerateResponse = z.infer<typeof GenerateResponseSchema>;

0xmad · 2025-05-16T15:39:12Z

plugins/maciVoting/contexts/types.ts


 export interface IVoteArgs {
  voteOptionIndex: bigint;
  newVoteWeight: bigint;
 }

+export type GenerateResponse = z.infer<typeof GenerateResponseSchema>;
+export type SubmitResponse = z.infer<typeof SubmitResponseSchema>;


Suggested change

export type SubmitResponse = z.infer<typeof SubmitResponseSchema>;

export type TSubmitResponse = z.infer<typeof SubmitResponseSchema>;

JohnGuilding · 2025-05-19T20:41:32Z

@0xmad addressed your newer comments, but in this new PR

Nico and myself thought raising a new PR would be simpler than trying to update the existing one with the latest changes from main

JohnGuilding · 2025-05-19T20:42:09Z

Replaced with #19

JohnGuilding marked this pull request as ready for review May 14, 2025 21:31

JohnGuilding requested review from 0xmad, NicoSerranoP and ctrlc03 May 14, 2025 21:32

JohnGuilding commented May 14, 2025

View reviewed changes

NicoSerranoP requested changes May 14, 2025

View reviewed changes

0xmad requested changes May 14, 2025

View reviewed changes

NicoSerranoP reviewed May 16, 2025

View reviewed changes

JohnGuilding added 11 commits May 16, 2025 11:52

feat: copy required finalise step code from coordinator

2499e5c

feat: fix zod types & add err handling

9a3964d

chore: tidy up files

68a8102

Add schemas.ts Add errors.ts Add final TODOs Note copied code

chore: clean up TODOs

eb89203

chore: fix lint issue + minor changes

7f6cc03

The component with the lint error did not even use props, so removed the interface

feat: move coordinator logic into context

140e193

refactor: delete unused files + renames

59b9f2c

refactor: update auth functions

554e1f5

chore: revert eslint rule change

134e921

refactor: code style

87bce97

chore: update function arguments

0d438d7

JohnGuilding force-pushed the feat/scaffold-coordinator-service-api-calls branch from ac3194c to 0d438d7 Compare May 16, 2025 14:53

0xmad reviewed May 16, 2025

View reviewed changes

JohnGuilding changed the base branch from feat/maci-plugin to main May 19, 2025 17:17

JohnGuilding changed the base branch from main to feat/maci-plugin May 19, 2025 18:21

JohnGuilding changed the base branch from feat/maci-plugin to main May 19, 2025 18:30

JohnGuilding changed the base branch from main to feat/maci-plugin May 19, 2025 18:31

JohnGuilding closed this May 19, 2025

JohnGuilding mentioned this pull request May 19, 2025

feat: scaffold coordinator context #19

Merged

JohnGuilding deleted the feat/scaffold-coordinator-service-api-calls branch May 19, 2025 21:02

		startBlock: Number(blockNumber) - 100,
		endBlock: Number(blockNumber) + 100,

	export const PUB_COORDINATOR_SERVICE_URL = process.env.NEXT_PUBLIC_COORDINATOR_SERVICE_URL ?? "";
	export const PUBLIC_COORDINATOR_SERVICE_URL = process.env.NEXT_PUBLIC_COORDINATOR_SERVICE_URL ?? "";

	export interface CoordinatorContextType {
	export interface ICoordinatorContextType {

	export type GenerateResponse = z.infer<typeof GenerateResponseSchema>;
	export type TGenerateResponse = z.infer<typeof GenerateResponseSchema>;

	export type SubmitResponse = z.infer<typeof SubmitResponseSchema>;
	export type TSubmitResponse = z.infer<typeof SubmitResponseSchema>;

Feat/scaffold coordinator service api calls #11

Feat/scaffold coordinator service api calls #11

Uh oh!

Conversation

JohnGuilding commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

NicoSerranoP commented May 14, 2025

Uh oh!

NicoSerranoP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xmad left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

JohnGuilding commented May 14, 2025 •

edited

Loading