feat(sdk-core): extract isMpcV2Keycard and signEddsaMpcV2RecoveryTx into shared utils by vibhavgo · Pull Request #9449 · BitGo/BitGoJS

vibhavgo · 2026-08-07T09:08:52Z

Summary

Adds EddsaSigningMaterial discriminated union (v1 | v2) to eddsaMPCv2.ts
Adds isMpcV2Keycard(userKey, walletPassphrase, bitgo?) — detects v1 (JSON/SJCL) vs v2 (CBOR) keycard, returns typed signing material; sjcl fallback retained for optional bitgo
Adds signEddsaMpcV2RecoveryTx(params) — full MPCv2 recovery signing: decrypt key shares → validate commonKeyChain → MPS DSG
Exports all new symbols from sdk-core package root
Replaces inline implementations in abstract-substrate and sdk-coin-sol with the shared helpers — drop-in, no behaviour change, no test modifications
18 tests including end-to-end signature verification

Context

WCI-1276 — isMpcV2Keycard and signEddsaMpcV2RecoveryTx were duplicated inline across coin modules (Substrate/DOT/POLYX, SOL). This PR extracts them into sdk-core and replaces all call sites.

This branch is based on feat/abstract-substrate/WCI-1239 and should be merged after that branch lands.

Modules changed

Module	Change
`@bitgo/sdk-core`	New shared helpers + types + exports
`@bitgo/abstract-substrate`	Delegates `isMpcV2Keycard` to shared helper; removes local `SubstrateSigningMaterial` type
`@bitgo/sdk-coin-sol`	Delegates `isMpcv2SigningMaterial` and MPCv2 signing block to shared helpers

Test plan

yarn test in modules/sdk-core — 18 passing
yarn build in modules/abstract-substrate — clean
yarn test in modules/sdk-coin-sol — 482 passing (3 pre-existing env-var failures unrelated to this PR)

🤖 Generated with Claude Code

Marzooqa

Nice consolidation — moving isMpcV2Keycard/signEddsaMpcV2RecoveryTx into sdk-core is the right call since the logic is genuinely duplicated across substrate/sol/ton (and soon ada/sui/near/iota per WCI-1276).

Two follow-up cleanup spots in abstractSubstrateCoin.ts now that the shared helpers exist — left inline.

rishikeshdadam136 · 2026-08-10T12:36:57Z

Will defer to marzooqa review

davidkaplanbitgo

unclear why a BTC review is needed. Don't have enough context

Ticket: WCI-1276 Add EddsaSigningMaterial discriminated union, isMpcV2Keycard, and signEddsaMpcV2RecoveryTx to eddsaMPCv2.ts. Replace duplicated inline implementations in abstract-substrate and sdk-coin-sol with these shared helpers. sjcl fallback retained for optional bitgo parameter. Export all new symbols from sdk-core package root. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Ticket: WCI-1276 Replace inline isMpcV2Keycard and addRecoverySignature MPCv2 block in sdk-coin-ton with shared helpers from sdk-core. Removes TonSigningMaterial local type in favour of EddsaSigningMaterial. Drop-in — no behaviour change, no test modifications. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…Keychain Ticket: WCI-1276 Address PR review comments: - Remove decryptKeychain private method; inline decryptKeychainPrivateKey at its one remaining call site (MPCv1 backup-key path) - Replace getEddsaMpcV2RecoveryKeyShares + signEddsaMpcV2Recovery split wrappers with a single signSubstrateMpcV2Recovery wrapper that delegates to signEddsaMpcV2RecoveryTx from sdk-core - Remove EDDSAUtils import (no longer referenced) - Update test stubs to target the new consolidated wrapper Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dpkjnr · 2026-08-10T17:14:07Z

+ * For v2: returns the encrypted key as-is for use with signEddsaMpcV2RecoveryTx.
+ * Identical logic across all EdDSA coin recovery implementations.
+ */
+export async function isMpcV2Keycard(


Naming nit: isMpcV2Keycard reads like a boolean predicate, but this returns EddsaSigningMaterial. That mismatch encourages the Sol boolean-only misuse below.

Consider renaming to something like getEddsaSigningMaterial / detectEddsaKeycardMaterial before more coins adopt it under WCI-1276.

dpkjnr · 2026-08-10T17:14:07Z

+  }
+  if (isV1) {
+    if (!bitgo) throw new Error('bitgo instance required for MPCv1 keycard decryption');
+    const userPrv = await bitgo.decrypt({ input: normalized, password: walletPassphrase });


For v1 keycards this decrypts twice: once inside isEddsaMpcV1SigningMaterial, then again here via bitgo.decrypt.

Since isEddsaMpcV1SigningMaterial already materializes the plaintext to JSON-parse it, can we reuse that decrypted value (or fold detection into one decrypt) so callers don't pay a second Argon2id/SJCL decrypt?

Also: isEddsaMpcV1SigningMaterial still supports sjcl fallback when bitgo is omitted, but this path then hard-requires bitgo for v1. Worth clarifying in the docstring that detection can be sjcl-only, while materializing v1 plaintext requires bitgo.

dpkjnr · 2026-08-10T17:14:07Z


    const isMpcV2 = params.walletPassphrase
-      ? !(await EDDSAUtils.isEddsaMpcV1SigningMaterial(userKey, params.walletPassphrase, this.bitgo))
+      ? (await isMpcV2Keycard(userKey, params.walletPassphrase, this.bitgo)).version === 'v2'


This used to be a cheap boolean check via isEddsaMpcV1SigningMaterial. Switching to isMpcV2Keycard(...).version === 'v2' means v1 keycards decrypt twice and then discard userPrv.

Same pattern at the consolidations call site (~1823) and in isMpcv2SigningMaterial (~1988).

For boolean-only detection, prefer keeping isEddsaMpcV1SigningMaterial (or a thin boolean helper) until the shared helper stops double-decrypting on v1.

dpkjnr · 2026-08-10T17:14:07Z

+    if (!walletPassphrase) return false;
+    if (!userKey) throw new Error('missing userKey');
+    if (!backupKey) throw new Error('missing backupKey');
+    const material = await isMpcV2Keycard(userKey.replace(/\s/g, ''), walletPassphrase, this.bitgo);


Same double-decrypt concern here: this method only needs a boolean, but isMpcV2Keycard materializes full v1 signing material just to compare .version === 'v2'.

If we keep the material-returning helper, maybe add a dedicated boolean helper (or have this call isEddsaMpcV1SigningMaterial directly) so recovery/consolidation hot paths don't pay the extra decrypt.

dpkjnr · 2026-08-10T17:14:07Z

-      });
+      sinon
+        .stub(coin as unknown, 'signSubstrateMpcV2Recovery')
+        .rejects(new Error('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey'));


This test used to exercise the real commonKeyChain mismatch check. After the consolidation it stubs signSubstrateMpcV2Recovery to reject with that exact error, then asserts the same message — so it's now tautological and no longer validates substrate wiring.

Suggest either:

Drop this case (sdk-core already covers mismatch in signEddsaMpcV2RecoveryTx), or

Keep a thin propagation/args assertion without hardcoding the failure inside the stub.

dpkjnr · 2026-08-10T17:14:07Z

+    const result = await isMpcV2Keycard(encrypted, PASSPHRASE, mockBitgo);
+
+    assert.strictEqual(result.version, 'v1');
+    assert.ok('userPrv' in result);


Minor: this only asserts version === 'v1' and that userPrv exists. Please also assert the decrypted content, e.g. result.userPrv === JSON.stringify(MPCv1_MATERIAL) (and/or that bitgo.decrypt was called with the normalized key).

dpkjnr · 2026-08-10T17:14:07Z

-      return { version: 'v1', userPrv };
-    }
-    return { version: 'v2', encryptedUserKey: normalized };
+  private async isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<EddsaSigningMaterial> {


Nit: this private wrapper is now a pure pass-through to sharedIsMpcV2Keycard. Fine if you want a stable local call site, but if nothing stubs it, callers could use the shared helper directly and drop the alias import.

vibhavgo requested review from a team as code owners August 7, 2026 09:08

vibhavgo requested review from danielpeng1 and rishikeshdadam136 August 7, 2026 09:08

vibhavgo marked this pull request as draft August 7, 2026 10:18

vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 1340716 to 32bab87 Compare August 10, 2026 05:21

vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 32bab87 to 37ba5b1 Compare August 10, 2026 05:34

vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from cba5249 to 541f899 Compare August 10, 2026 06:17

vibhavgo requested a review from Marzooqa August 10, 2026 07:16

vibhavgo marked this pull request as ready for review August 10, 2026 07:31

Marzooqa reviewed Aug 10, 2026

View reviewed changes

Comment thread modules/abstract-substrate/src/abstractSubstrateCoin.ts Outdated

Comment thread modules/abstract-substrate/src/abstractSubstrateCoin.ts

davidkaplanbitgo reviewed Aug 10, 2026

View reviewed changes

vibhavgo requested a review from Marzooqa August 10, 2026 14:45

Base automatically changed from feat/abstract-substrate/WCI-1239 to master August 10, 2026 15:01

vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch 2 times, most recently from 8ce5176 to 7c1c3cc Compare August 10, 2026 15:09

vibhavgo and others added 3 commits August 10, 2026 22:05

vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 7c1c3cc to 070b63f Compare August 10, 2026 16:36

dpkjnr reviewed Aug 10, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(sdk-core): extract isMpcV2Keycard and signEddsaMpcV2RecoveryTx into shared utils - #9449

feat(sdk-core): extract isMpcV2Keycard and signEddsaMpcV2RecoveryTx into shared utils#9449
vibhavgo wants to merge 3 commits into
masterfrom
feat/sdk-core/extract-mpcv2-recovery-helper

vibhavgo commented Aug 7, 2026 •

edited

Loading

Uh oh!

Marzooqa left a comment

Uh oh!

Uh oh!

Uh oh!

rishikeshdadam136 commented Aug 10, 2026

Uh oh!

davidkaplanbitgo left a comment

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

dpkjnr Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

vibhavgo commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Context

Modules changed

Test plan

Uh oh!

Marzooqa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rishikeshdadam136 commented Aug 10, 2026

Uh oh!

davidkaplanbitgo left a comment

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

dpkjnr Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vibhavgo commented Aug 7, 2026 •

edited

Loading