feat(ebe): support mpc signing for eddsa by pranavjain97 · Pull Request #34 · BitGo/advanced-wallets

pranavjain97 · 2025-06-19T22:09:41Z

No description provided.

mtexeira-simtlix · 2025-06-20T18:47:31Z

+  R = 'r',
+  G = 'g',
+
+  // TODO: Add ECDSA share types


Hope this helps, for ECDSA I found this variant of shares on account-lib/mpc/tss/ecdsa/types.ts file from the SDK:

export type SignConvert = { xShare?: XShareWithChallenges; // XShare of the current participant yShare?: YShare; // YShare corresponding to the other participant kShare?: KShare; // KShare received from the other participant bShare?: BShare; // Private Beta share of the participant muShare?: MUShare; // muShare received from the other participant aShare?: AShare; wShare?: WShare; };

TL;DR: x, y, k, a, b, mu, w are the shares for ECDSA (also g but already added for EdDSA).

Dw about ECSSA, i'll add them when we get to ECDSA signing

mtexeira-simtlix · 2025-06-20T18:50:15Z

+
+export async function signMpcTransaction(req: EnclavedApiSpecRouteRequest<'v1.mpc.sign', 'post'>) {
+  const { source, pub, encryptedDataKey } = req.decoded;
+  const { coin: coinName, shareType } = req.params;


change: "coin" also comes from req.decoded (not params).

mtexeira-simtlix · 2025-06-20T18:51:10Z

+  const { coin: coinName, shareType } = req.params;
+
+  if (!source || !pub) {
+    throw new Error('Source and public key are required for MPC signing');


question: shouldn't we also need to do the logger.error call here? (and in the shareType check).

mtexeira-simtlix · 2025-06-20T19:03:04Z

+// Response type for /mpc/sign endpoint
+const SignMpcResponse: HttpResponse = {
+  // TODO: Define proper response type for MPC transaction signing
+  200: t.any,


reminder: add type to return type (is always the same type or does it varies depending what coin are you signing?)

mtexeira-simtlix · 2025-06-20T19:05:34Z

+    try {
+      kmsResponse = await superagent
+        .post(`${this.url}/generateDataKey`)
+        .set('x-api-key', 'abc')


Don't need to keep adding the api key, I removed it some weeks ago : D

mtexeira-simtlix · 2025-06-20T19:05:54Z

+    try {
+      kmsResponse = await superagent
+        .post(`${this.url}/decryptDataKey`)
+        .set('x-api-key', 'abc')


same as prev comment.

mtexeira-simtlix

Added a couple of comments

Ticket: WP-4759

Ticket: WP-00000

Copilot

Pull Request Overview

This PR adds support for MPC signing for EDDSA by introducing new routes, handlers, and associated tests while refactoring import paths to a shared types directory.

Updates import paths across multiple modules to use the new shared folder structure.
Introduces new API route "v1.mpc.sign" with corresponding handler (signMpcTransaction) and supporting functions for EDDSA MPC signing.
Adjusts test cases to align with the new file structure and updated dependency versions.

Reviewed Changes

Copilot reviewed 33 out of 37 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/types/request.ts	Updated import paths for Config and EnclavedExpressClient.
src/shared/responseHandler.ts	Changed Config import to shared/types.
src/shared/middleware.ts	Updated import path for isMasterExpressConfig.
src/shared/appUtils.ts	Adjusted import for Config and TlsMode.
src/routes/master.ts	Moved master API router imports to new locations.
src/routes/enclaved.ts	Corrected import order and paths for EnclavedConfig.
src/masterExpressApp.ts	Updated import order for config initialization.
src/kms/kmsClient.ts	Added functions to generate and decrypt data keys.
src/enclavedBitgoExpress/routers/enclavedApiSpec.ts	Added new route definition for v1.mpc.sign.
src/enclavedApp.ts	Adjusted imports for Enclaved configuration.
src/app.ts	Updated imports for determineAppMode and AppMode.
src/api/master/routers/*	Updated handler imports and middleware usage.
src/api/enclaved/handlers/signMpcTransaction.ts	Implements new EDDSA MPC signing logic.
Tests	Updated test imports and configurations for new structure.

Comments suppressed due to low confidence (1)

src/api/enclaved/handlers/signMpcTransaction.ts:156

[nitpick] Verify that using 'RSA-2048' as the key type for generating the data key is intentional in the context of EDDSA MPC signing, as the algorithm mismatch might be confusing.

      const dataKey = await generateDataKey({ keyType: 'RSA-2048', cfg });

Copilot · 2025-06-20T22:13:30Z

+        .set('x-api-key', 'abc')
+        .send(params);
+    } catch (error: any) {
+      console.log('Error generating data key from KMS', error);


Consider replacing console.log with the configured logger (e.g., logger.error) for better consistency in error logging.

Suggested change

console.log('Error generating data key from KMS', error);

debugLogger('Error generating data key from KMS: %O', error);

let's do that.

mohammadalfaiyazbitgo

lgtm, couple of nits that can be followed up on.

mohammadalfaiyazbitgo · 2025-06-20T22:24:03Z

+  if (!source || !pub) {
+    throw new Error('Source and public key are required for MPC signing');
+  }


why not just type them as required in the codec?

oh I guess because this endpoint handles both signing rounds?

mohammadalfaiyazbitgo · 2025-06-20T22:24:14Z

+  if (!shareType) {
+    throw new Error('Share type is required for MPC signing');
+  }


mohammadalfaiyazbitgo · 2025-06-20T22:28:41Z

+      if (!txRequest) {
+        throw new Error('txRequest is required for commitment share generation');
+      }


this check is in both rounds, why not just make it required?

mohammadalfaiyazbitgo · 2025-06-20T22:30:36Z

+const SignMpcRequest = {
+  source: t.string,
+  pub: t.string,
+  txRequest: t.union([t.undefined, t.any]),


isn't this required in both signing cases so can we define it as non-nullable?

mohammadalfaiyazbitgo · 2025-06-20T22:35:27Z

+        .set('x-api-key', 'abc')
+        .send(params);
+    } catch (error: any) {
+      console.log('Error generating data key from KMS', error);


let's do that.

mohammadalfaiyazbitgo · 2025-06-20T22:36:07Z

+        .set('x-api-key', 'abc')
+        .send(params);
+    } catch (error: any) {
+      console.log('Error decrypting data key from KMS', error);


debugLogger.

mohammadalfaiyazbitgo · 2025-06-20T22:36:57Z

@@ -0,0 +1,27 @@
+import z from 'zod';


is the kms code moved to the same repo? i didn't realize we're using zod for it.

pranavjain97 force-pushed the WP-4759-ecdsa-signing-advanced-wallets branch from b57fd78 to 62c91a3 Compare June 19, 2025 22:12

pranavjain97 changed the title ~~Wp 4759 ecdsa signing advanced wallets~~ Wp 4759 addsa signing advanced wallets Jun 19, 2025

pranavjain97 changed the title ~~Wp 4759 addsa signing advanced wallets~~ feat(ebe): support mpc signing for eddsa Jun 19, 2025

pranavjain97 marked this pull request as ready for review June 20, 2025 18:34

mtexeira-simtlix reviewed Jun 20, 2025

View reviewed changes

Comment thread src/api/enclaved/signMpcTransaction.ts

mtexeira-simtlix reviewed Jun 20, 2025

View reviewed changes

Comment thread src/enclavedBitgoExpress/routers/enclavedApiSpec.ts

mtexeira-simtlix reviewed Jun 20, 2025

View reviewed changes

pranavjain97 added 5 commits June 20, 2025 16:43

chore: upgrade sdk

ac0006f

Ticket: WP-4759

chore: use sdk "onPrem" subtype

4df7ccf

Ticket: WP-4759

feat(ebe): add mpc eddsa signing for ebe

69c2eca

Ticket: WP-4759

chore: upgrade to TS@5 and fix build

50a5dea

Ticket: WP-4759

chore: add mpc signing response type

bf30d2a

Ticket: WP-4759

pranavjain97 force-pushed the WP-4759-ecdsa-signing-advanced-wallets branch from 2b43409 to bf30d2a Compare June 20, 2025 20:45

pranavjain97 added 2 commits June 20, 2025 16:51

chore: refactor all API handlers/test hierarchy

e938b71

chore: import types from shares/types instead of initConfig

fabf21e

Ticket: WP-00000

pranavjain97 enabled auto-merge June 20, 2025 20:52

mohammadalfaiyazbitgo requested a review from Copilot June 20, 2025 22:13

Copilot AI reviewed Jun 20, 2025

View reviewed changes

mohammadalfaiyazbitgo approved these changes Jun 20, 2025

View reviewed changes

pranavjain97 merged commit 43fcf25 into master Jun 20, 2025
3 checks passed

pranavjain97 deleted the WP-4759-ecdsa-signing-advanced-wallets branch June 20, 2025 22:37

pranavjain97 mentioned this pull request Jun 25, 2025

chore: address follow up comments #44

Merged

	console.log('Error generating data key from KMS', error);
	debugLogger('Error generating data key from KMS: %O', error);

Conversation

pranavjain97 commented Jun 19, 2025

Uh oh!

mtexeira-simtlix Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtexeira-simtlix left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohammadalfaiyazbitgo left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtexeira-simtlix Jun 20, 2025 •

edited

Loading