feat: sign eddsa recovery transaction by mohammadalfaiyazbitgo · Pull Request #65 · BitGo/advanced-wallets

mohammadalfaiyazbitgo · 2025-07-10T15:19:50Z

No description provided.

mohammadalfaiyazbitgo · 2025-07-11T03:29:36Z

Copilot

Pull Request Overview

This PR adds support for EdDSA recovery transactions, introducing a new TSS recovery flow alongside the existing multi-signature recovery mechanism. The changes enable signing recovery transactions for EdDSA-based coins (Solana, Near, Sui, Ada, Dot) through a new MPC endpoint.

Implements EdDSA recovery transaction signing functionality with TSS keyshare support
Adds new API endpoint /api/{coin}/mpc/recovery for EdDSA recovery operations
Restructures existing recovery API to support both TSS and traditional multi-signature recovery modes

Reviewed Changes

Copilot reviewed 12 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/shared/coinUtils.ts	Adds utility functions for Solana coin detection and EdDSA algorithm checking
src/enclavedBitgoExpress/routers/enclavedApiSpec.ts	Defines new MPC recovery endpoint with request/response types and handler routing
src/api/master/routers/masterApiSpec.ts	Updates recovery wallet API structure to support both TSS and multi-signature recovery parameters
src/api/master/handlers/recoveryWallet.ts	Implements dual recovery flow logic handling both EdDSA TSS and traditional multi-signature recovery
src/api/master/handlers/recoverEddsaWallets.ts	New module for coin-specific EdDSA wallet recovery with dynamic imports
src/api/master/clients/enclavedExpressClient.ts	Adds MPC recovery client method with transaction format handling
src/api/enclaved/mpcFinalize.ts	Updates signing material structure for user vs backup key differentiation
src/api/enclaved/handlers/signEddsaRecoveryTransaction.ts	New handler implementing EdDSA signature generation with TSS support
src/tests/api/master/recoveryWallet.test.ts	Updates test structure to match new API parameter organization
src/tests/api/master/musigRecovery.test.ts	Updates test cases and adds TSS recovery validation test
package.json	Adds EdDSA coin SDK dependencies and version resolution
masterBitgoExpress.json	Updates OpenAPI specification for new recovery API structure

Copilot · 2025-07-11T03:30:36Z

+      recoveryDestination,
+      apiKey: params.apiKey,
+    });
+    console.log('Unsigned sweep tx');


Console.log statements should not be used in production code. Consider using a proper logging framework or removing this debug output.

Copilot · 2025-07-11T03:30:36Z

+      apiKey: params.apiKey,
+    });
+    console.log('Unsigned sweep tx');
+    console.log(JSON.stringify(unsignedSweepPrebuildTx, null, 2));


Console.log statements should not be used in production code. Consider using a proper logging framework or removing this debug output.

Copilot · 2025-07-11T03:30:36Z

+
+    return { txBuilder, publicKey };
+  } catch (error) {
+    console.error(error);


Console.error should be replaced with a proper logging framework for consistency with the rest of the codebase.

Suggested change

console.error(error);

logger.error(error);

Copilot · 2025-07-11T03:30:37Z

    throw new Error('Invalid user public key format');
  } else if (!sdkCoin.isValidPub(backupPub)) {
-    throw new Error('Invalid backup public');
+    throw new Error('Invalid backup public key format');


The error message should be 'Invalid backup public key format' to be consistent with the user key error message format.

Copilot · 2025-07-11T03:30:37Z

+    userPub: string;
+    backupPub: string;
+    apiKey: string;
+    coinSpecificParams: any;


Using 'any' type for coinSpecificParams reduces type safety. Consider defining a proper interface or union type for coin-specific parameters.

Suggested change

coinSpecificParams: any;

coinSpecificParams: CoinSpecificParams;

Copilot · 2025-07-11T03:30:37Z

+  coinFamily: CoinFamily,
+  signableHex: string,
+  accountId: string,
+): Promise<{ txBuilder: any; publicKey: string }> {


Using 'any' type for txBuilder reduces type safety. Consider defining a proper interface or union type for transaction builders.

pranavjain97

Nice work! Some comments

pranavjain97 · 2025-07-11T18:18:32Z

+  'v1.mpc.recovery': {
+    post: httpRoute({
+      method: 'POST',
+      path: `/api/{coin}/mpc/recovery`,
+      request: httpRequest({
+        params: {
+          coin: t.string,
+        },
+        body: RecoveryEdDSARequest,


why is the body specific to EDDSA?

pranavjain97 · 2025-07-11T18:19:05Z

  }),
 };

+const RecoveryEdDSARequest = {


woudn't it be similar for ECDSA?

pranavjain97 · 2025-07-11T18:22:18Z

+  const { recoveryDestination, userKey } = commonRecoveryParams;
+  try {
+    const unsignedSweepPrebuildTx = await recoverEddsaWallets(bitgo, sdkCoin, {
+      bitgoKey: userKey,


I would change userKey to call it commonKeychain so it isnt misleading

lol that's how they decided to name the field on the sdk. Would just mean more transforming of the parameters. I can do it in a follow up.

pranavjain97 · 2025-07-11T18:34:02Z

+      if ('txRequests' in tx && Array.isArray(tx.txRequests)) {
+        // MPCTxs format
+        const firstRequest = tx.txRequests[0];
+        if (firstRequest && firstRequest.transactions && firstRequest.transactions[0]) {
+          const firstTx = firstRequest.transactions[0];
+          txRequest.signableHex = firstTx.unsignedTx?.serializedTx || '';
+          txRequest.derivationPath = firstTx.unsignedTx?.derivationPath || '';
+        }
+      } else if ('signableHex' in tx) {
+        // MPCTx format
+        txRequest.signableHex = tx.signableHex || '';
+        txRequest.derivationPath = tx.derivationPath || '';
+      } else if (Array.isArray(tx) && tx.length > 0) {
+        // MPCSweepTxs format
+        const firstTx = tx[0];
+        if (firstTx && firstTx.transactions && firstTx.transactions[0]) {
+          const transaction = firstTx.transactions[0];
+          txRequest.signableHex = transaction.unsignedTx?.signableHex || '';
+          txRequest.derivationPath = transaction.unsignedTx?.derivationPath || '';
+        }
+      }


Could you add comments in which scenario are which formats used? I'm surprised it differs so much just between MPC.

Let's move this to a helper. Like prepareUnsignedSweepTxRequest

The client is already too big; we need to start modularizing it.

can do it in a follow up.

pranavjain97 · 2025-07-11T18:34:10Z

+        txRequest.signableHex = tx.signableHex || '';
+        txRequest.derivationPath = tx.derivationPath || '';
+      } else if (Array.isArray(tx) && tx.length > 0) {
+        // MPCSweepTxs format


arent the above two sweeps as well?

they are, just different transaction formats that are returned by recovery

pranavjain97 · 2025-07-11T18:38:58Z

+  const hdTree = await Ed25519Bip32HdTree.initialize();
+  const MPC = await Eddsa.initialize(hdTree);
+
+  const accountId = MPC.deriveUnhardened(request.commonKeychain, request.derivationPath).slice(


what is this accountId?

https://github.com/BitGo/BitGoJS/blob/52c248404bdfadf4b285f46e9585909446824f33/modules/sdk-coin-sol/src/sol.ts#L742 something like a public key I guess?

pranavjain97 · 2025-07-11T18:39:41Z

+    accountId,
+  );
+
+  const txBuilder = builder;


nit: prolly not needed

Ticket: WP-5166

mohammadalfaiyazbitgo requested a review from alextse-bg July 10, 2025 15:19

mohammadalfaiyazbitgo force-pushed the WP-5166-eddsa-recovery-v2 branch from de845fb to cbe8005 Compare July 11, 2025 03:27

mohammadalfaiyazbitgo changed the title ~~WIP: draft for eddsa recvoery~~ feat: sign eddsa recovery transaction Jul 11, 2025

mohammadalfaiyazbitgo requested review from Copilot and pranavjain97 July 11, 2025 03:29

mohammadalfaiyazbitgo marked this pull request as ready for review July 11, 2025 03:29

mohammadalfaiyazbitgo force-pushed the WP-5166-eddsa-recovery-v2 branch from cbe8005 to 3a3ea85 Compare July 11, 2025 03:30

Copilot AI reviewed Jul 11, 2025

View reviewed changes

pranavjain97 reviewed Jul 11, 2025

View reviewed changes

mohammadalfaiyazbitgo force-pushed the WP-5166-eddsa-recovery-v2 branch 2 times, most recently from ac800dc to ff6ecf8 Compare July 11, 2025 19:50

feat: eddsa recover txn signature

88eac6b

Ticket: WP-5166

mohammadalfaiyazbitgo force-pushed the WP-5166-eddsa-recovery-v2 branch from ff6ecf8 to 88eac6b Compare July 11, 2025 20:05

mohammadalfaiyazbitgo requested a review from pranavjain97 July 11, 2025 20:12

pranavjain97 approved these changes Jul 14, 2025

View reviewed changes

pranavjain97 merged commit c1cfe09 into master Jul 14, 2025
3 checks passed

pranavjain97 deleted the WP-5166-eddsa-recovery-v2 branch July 14, 2025 22:14

	coinSpecificParams: any;
	coinSpecificParams: CoinSpecificParams;

Conversation

mohammadalfaiyazbitgo commented Jul 10, 2025

Uh oh!

mohammadalfaiyazbitgo commented Jul 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

pranavjain97 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!

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!

3 participants