recovery multisig for eve and mbe by mtexeira-simtlix · Pull Request #23 · BitGo/advanced-wallets

mtexeira-simtlix · 2025-06-11T19:23:29Z

Description:

recovery full flow, mbe & eve

pranavjain97 · 2025-06-11T20:18:56Z

Ticket: WP-4637.

Reviews needed in order to avoid delays: @pranavjain97 I'm missing the enclavedExpressClient.signMultisigTransaction method (eve api), mind if you let me know when merged? I need that to finish the handleRecoveryWalletOnPrem on masterBitgoExpress/recoveryWallet.ts, i left the calls but there is no method implemented currently.

@pranavjain97 I need a hand to type the txBuild on enclavedApiSpec.ts, on a normal signing it could be a TransactionPrebuild and in a recovery it could be other types that comes from the SDK (Typescript types), the issue is that currently:

and because it's all io-ts, i didn't managed to find a way to export the types from the TS side to the IO-TS side so I need your advice (worst case we could leave the types as t.any and find a way later to reach the demo date on Friday). Thread about this: https://bitgo.slack.com/archives/C08RCSLF398/p1749666974629419

@mohammadalfaiyazbitgo mind checking the main body of handleRecoveryWalletOnPrem, particulary the part about ETH please? let me know if you find something weird or any kind of improvements please. In order to understand why am I doing this (on mbe):

you need to also check side by side with this (on eve signing)

@pranavjain97 & @mohammadalfaiyazbitgo mind checking the signMultisigTransactionRecovery.ts file please? It's an alternative version that i created for signing as i'm not sure how to integrate the recovery signing with a normal signing, It's not as trivial as it sounds and I'm presenting some alternatives and concerns that I tried to express some days ago but wasn't as visible as now for the lack of a more polished code examples. I'll need a hand in order to unity the recovery code with the normal signing, on things like verifying the tx i.e.

Also, the proposed code path that i left is pretty simple and did some refactors in order to remove some cluther:

@pranavjain97 @mohammadalfaiyazbitgo do we need this kind of validation? (related to the previous comment), if not please let me know to remove the extras and avoid investing time on this. Thanks guys!

Those are my main concerns for now and things that blocks the delivery of the PR. If i noticed something else i'll let you know guys. Thanks!

Discussed/resolved all questions on meet

pranavjain97 · 2025-06-12T14:59:05Z

+        const unsignedTx = await coin.recover({
+          ...commonRecoveryParams,
+          //TODO: it's needed for keycard debugging, the walletPassphrase
+          //walletPassphrase: passphrase,
+        });


This will use blockchain APIs to build, will need to be in MBE. Enclave will need to accept the unsignedSweepPrebuild

Hi @pranavjain97 thanks for the reviews! Is this one solved for you? let me know if we need more changes and if that's not the case please "Resolve conversation". Thank you!

mohammadalfaiyazbitgo · 2025-06-12T14:37:46Z

+  // If you check the type of coin before and after the "if", you may see "BaseCoin" vs "AbstractEthLikeCoin"
+  if (coin.isEVM()) {
+    // Every recovery method on every coin family varies one from another so we need to ensure with a guard.
+    if (isEthCoin(coin)) {


i think you can expand this to ethlike they all use AbstractEthLike as the base class iirc.

Sure! it's an issue of bad naming of the func, it says "isEthCoin" but the implementations checks for pure eth and all the eth like coins. I'm gonna change the name.
Good catch, thanks!

mohammadalfaiyazbitgo · 2025-06-12T18:52:56Z

+  const isEthPure =
+    isFamily(coin, 'eth', 'gteth') ||
+    isFamily(coin, 'eth', 'hteth') ||
+    isFamily(coin, 'ethw', 'tethw');
+
+  const isEthLike =
+    isFamily(coin, 'rbtc', 'trbtc') ||
+    isFamily(coin, 'etc', 'tetc') ||
+    isFamily(coin, 'avaxc', 'tavaxc') ||
+    isFamily(coin, 'polygon', 'tpolygon') ||
+    isFamily(coin, 'arbeth', 'tarbeth') ||
+    isFamily(coin, 'opeth', 'topeth') ||
+    isFamily(coin, 'bsc', 'tbsc') ||
+    isFamily(coin, 'baseeth', 'tbaseeth') ||
+    isFamily(coin, 'coredao', 'tcoredao') ||
+    isFamily(coin, 'oas', 'toas') ||
+    isFamily(coin, 'flr', 'tflr') ||
+    isFamily(coin, 'sgb', 'tsgb') ||
+    isFamily(coin, 'wemix', 'twemix') ||
+    isFamily(coin, 'xdc', 'txdc');
+
+  return isEthPure || isEthLike;


use the CoinFamily enum https://github.com/BitGo/BitGoJS/blob/f1a721be2963094c84fa14d7a4dbad22da102094/modules/statics/src/base.ts#L22, I don't think you need to type the test versions either.

I don't think you need to type the test versions either.
True! I copied the "isFamily" method from some FE utility on OVC but in the description of "getFamily" it clearly states that the family test version of a coin is in fact the family of the prod version so we don't need that check.
Thanks for the catch.

I'm gonna upload an update without the test versions of the coin and using the enum. Although i was surprised when checked the family of ethw as an example and discovered that it's not coming from ETH.
I suppose that it's because ethw is a "wrapper" of ethereum in another chain.

A little preview of the code pending to be uploaded:

This was added on my last commit, thanks!

mohammadalfaiyazbitgo · 2025-06-12T18:54:10Z

+  const {
+    userPub,
+    backupPub,
+    walletContractAddress,
+    recoveryDestinationAddress,
+    coinSpecificParams,
+    apiKey,
+  } = req.body;


use req.decoded

Added the change, check it on next commit when re review requested please. Thanks Mohammad!

Changed on my last commit, thanks!

mtexeira-simtlix · 2025-06-13T14:17:22Z

Moving from draft -> ready for review.
Still working on yesterdays feedback but I don't wanna push and forget to change the PR state so i'm changing it now (blocked by requested-changes so no risk of merging it by mistake)

Copilot

Pull Request Overview

This PR introduces end-to-end recovery support for multisig wallets on both the master Express and enclaved Express services, including new routes, handlers, client methods, and test mocks.

Added coin classification utilities (isEthLikeCoin, isUtxoCoin, etc.) in coinUtils.ts.
Defined and implemented recovery endpoints and handlers for master Express (v1.wallet.recovery) and enclaved Express (v1.multisig.recovery).
Introduced KMS key retrieval helper and recovery transaction logic in the enclaved API, along with supporting JSON mocks.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/shared/coinUtils.ts	New utilities to detect coin families (ETH-like, UTXO, EOS, STX, XTZ)
src/masterBitgoExpress/routers/masterApiSpec.ts	API spec and route registration for `/wallet/recovery`
src/masterBitgoExpress/recoveryWallet.ts	`handleRecoveryWalletOnPrem` implementation for ETH-like recovery
src/masterBitgoExpress/enclavedExpressClient.ts	Added `recoveryMultisig` client method
src/enclavedBitgoExpress/routers/enclavedApiSpec.ts	API spec and route for `/multisig/recovery`
src/api/enclaved/utils.ts	`retrieveKmsKey` helper for fetching keys from KMS
src/api/enclaved/recoveryMultisigTransaction.ts	Handler logic for signing and assembling the full recovery transaction
mocks/wallet-recovery/*.json	Sample request/response mocks for the recovery flow

Comments suppressed due to low confidence (2)

src/enclavedBitgoExpress/routers/enclavedApiSpec.ts:83

Typo in constant name: EnclavedAPiSpec should be EnclavedApiSpec to match project naming conventions.

export const EnclavedAPiSpec = apiSpec({

src/enclavedBitgoExpress/routers/enclavedApiSpec.ts:57

The request codec for multisig recovery omits walletContractAddress, but the handler and client expect it. Add this field to the request schema.

const RecoveryMultisigRequest = {

Copilot · 2025-06-16T20:27:33Z

+    isFamily(coin, CoinFamily.BCH) ||
+    isFamily(coin, CoinFamily.ZEC) ||
+    isFamily(coin, CoinFamily.DASH) ||
+    isFamily(coin, CoinFamily.DASH) ||


There are two identical checks for CoinFamily.DASH in isUtxoCoin. Remove the duplicate or replace one with the intended family.

Suggested change

isFamily(coin, CoinFamily.DASH) ||

Copilot · 2025-06-16T20:27:34Z

  }
+
+  /**
+   * Recovery a multisig transaction


Minor grammar: change the JSDoc comment to Recover a multisig transaction.

Suggested change

* Recovery a multisig transaction

* Recover a multisig transaction

mohammadalfaiyazbitgo

mainly nits: but lgtm.

mohammadalfaiyazbitgo · 2025-06-16T20:30:49Z

+  // recoveryDestinationAddress: string;
+}


remove commented code.

mohammadalfaiyazbitgo · 2025-06-16T20:31:06Z

+        unsignedSweepPrebuildTx,
+        coinSpecificParams,
+        walletContractAddress,
+        // recoveryDestinationAddress,


remove commented code.

Changed! thanks.

pranavjain97 · 2025-06-16T22:52:50Z

+      // TODO: populate coinSpecificParams with things like replayProtectionOptions
+      // coinSpecificParams type could be "recoverOptions"


is this still needed?

Yes, but not for this PR, still an usefull TODO to left for the immediate follow up on btc.

I'll remove it to avoid any confusions and add it later, just in case. Thanks Pranav.

pranavjain97 · 2025-06-16T22:53:16Z

+          walletContractAddress,
+        });
+
+        const { halfSigned } = halfSignedTx as any;


why do we need to typecast to any here? Please use the types, especially since you seem to know the fields you're using

signingKeyNonce: halfSigned.signingKeyNonce ?? 0, backupKeyNonce: halfSigned.backupKeyNonce ?? 0, recipients: halfSigned.recipients ?? [],

You need the typecast to any because something on the SDK types isn't finished unfortunately and halfSigned is not recognized as a prop of halfSignedTx (the output of coin.signTransaction) despite that it's there.

In @mohammadalfaiyazbitgo initial example code, I found the same any, so I decided to try to remove it and found this ungrateful surprise : (.

I think that you also got confused with what we're casting @pranavjain97 , the cast is not on halfSigned, but in halfSignedTx, for halfSigned as you say we know that it have those properties you mentioned and there is no cast.

pranavjain97 · 2025-06-16T22:56:03Z

+// TODO: this function is duplicated in multisigTransactioSign.ts but as hardcoded. Replace that code later with this call (to avoid merge conflicts/duplication)
+import { KmsClient } from '../../kms/kmsClient';
+import { EnclavedConfig } from '../../types';
+export async function retrieveKmsKey({


I'm sure this already exists. See kmsClient.ts -> getKey(). Please use that

Hi! it's not the same code that's why I left a comment as an explanation to avoid confusions but i must admit that it's kinda short and in the rush we're a proper explanation could be better so let me proceed with that please.
The code in retrieveKmsKey doesn't do the logic inside get, what it does is calling kms.getKey and wrapping the error responses as in the already existing piece of code on signMultisigTransaction.ts:

Instead of throwing the piece of code that does this directly in the main flow of the code, I moved it to a function to avoid clutter and let the one reading the function behavior focus on the important parts instead of the error responses.

The comment is also a TODO for latter, the code on signMultisigTransaction.ts was worked by Alex at that time and since I didn't know how much could someone change it, I didn't wanna to start randomly moving things on code non related to my PR to avoid conflicts.

Hope that clarifies a little! Thanks.

pranavjain97 · 2025-06-16T22:58:24Z

+  assert(
+    isMasterExpressConfig(req.config),
+    'Expected req.config to be of type MasterExpressConfig',
+  );
+  const enclavedExpressClient = createEnclavedExpressClient(req.config, coin);
+  if (!enclavedExpressClient) {
+    throw new Error(
+      'Enclaved express client not configured - enclaved express features will be disabled',
+    );
+  }


Rebase. All this is not needed anymore. These are checked in the middleware now

Sure! sorry, my PR is in review since Friday so I suppose that this got merged before my re-review request and got out of date. I'm gonna rebase. Thanks @pranavjain97 !

pranavjain97 · 2025-06-17T15:52:19Z

+// Response type for /multisig/recovery endpoint
+const RecoveryMultisigResponse: HttpResponse = {
+  // TODO: Define proper response type for recovery multisig transaction
+  200: t.any, // the full signed tx


Please type this out

pranavjain97

Discussed offline. Approving to unblock dev.
Followup needed-

Type out t.any in the response and half signed tx.
Making the kms retrieve key consistent.

mtexeira-simtlix requested review from mohammadalfaiyazbitgo and pranavjain97 June 11, 2025 19:23

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch 3 times, most recently from 8f58dd6 to 478d5e1 Compare June 12, 2025 13:51

pranavjain97 reviewed Jun 12, 2025

View reviewed changes

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch 2 times, most recently from 30647cc to bc0e6bd Compare June 12, 2025 16:48

mohammadalfaiyazbitgo requested changes Jun 12, 2025

View reviewed changes

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch from bc0e6bd to 693c2a8 Compare June 12, 2025 19:27

mtexeira-simtlix marked this pull request as ready for review June 13, 2025 14:17

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch from 693c2a8 to c0be450 Compare June 13, 2025 15:08

mtexeira-simtlix requested review from mohammadalfaiyazbitgo and pranavjain97 June 13, 2025 15:48

mohammadalfaiyazbitgo requested a review from Copilot June 16, 2025 20:25

Copilot AI reviewed Jun 16, 2025

View reviewed changes

mohammadalfaiyazbitgo previously approved these changes Jun 16, 2025

View reviewed changes

pranavjain97 requested changes Jun 16, 2025

View reviewed changes

mtexeira-simtlix added 7 commits June 17, 2025 10:21

feat: recovery handler router on mbe

8639449

feat: recovery for eth and other musig2 coins wip

ed414f7

feat: added partial typing - eve calls from mbe

efaab2b

feat: call route handler on eve client for recovery

3642a2a

feat: typed mbe recovery endpoint req params

f84a799

feat: eth like coins family and follow up feedback

0ec107a

fix: missing cfg prop on kms client call post rebase master

a5ff857

mtexeira-simtlix dismissed mohammadalfaiyazbitgo’s stale review via e96785c June 17, 2025 14:39

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch from c0be450 to e96785c Compare June 17, 2025 14:39

feat: behavior adjustments based on reviews

bbf2bf4

mtexeira-simtlix force-pushed the WP-4637_recovery_multisig branch from e96785c to bbf2bf4 Compare June 17, 2025 14:52

pranavjain97 reviewed Jun 17, 2025

View reviewed changes

pranavjain97 approved these changes Jun 17, 2025

View reviewed changes

mohammadalfaiyazbitgo merged commit d09b586 into master Jun 17, 2025
3 checks passed

mohammadalfaiyazbitgo deleted the WP-4637_recovery_multisig branch June 17, 2025 16:00

	* Recovery a multisig transaction
	* Recover a multisig transaction

		// TODO: populate coinSpecificParams with things like replayProtectionOptions
		// coinSpecificParams type could be "recoverOptions"

Conversation

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

Uh oh!

Uh oh!

pranavjain97 commented Jun 11, 2025 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavjain97 Jun 12, 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!

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!

mtexeira-simtlix commented Jun 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 16, 2025

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!

Copilot AI Jun 16, 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!

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

mtexeira-simtlix commented Jun 11, 2025 •

edited

Loading

pranavjain97 commented Jun 11, 2025 •

edited by atlassian Bot

Loading

pranavjain97 Jun 12, 2025 •

edited

Loading

mtexeira-simtlix Jun 17, 2025 •

edited

Loading