Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vibhavgo wants to merge 3 commits into from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ffc4be
feat(sdk-core): extract and consume shared EdDSA MPCv2 recovery helpers
vibhavgo Aug 10, 2026
2b4c897
feat(sdk-coin-ton): consume shared EdDSA MPCv2 recovery helpers
vibhavgo Aug 10, 2026
070b63f
refactor(abstract-substrate): collapse MPCv2 wrappers, inline decrypt…
vibhavgo Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 22 additions & 72 deletions modules/abstract-substrate/src/abstractSubstrateCoin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import {
UnexpectedAddressError,
verifyEddsaTssWalletAddress,
VerifyTransactionOptions,
EDDSAUtils,
decryptKeychainPrivateKey,
isMpcV2Keycard as sharedIsMpcV2Keycard,
signEddsaMpcV2RecoveryTx,
EddsaSigningMaterial,
} from '@bitgo/sdk-core';
import { CoinFamily, BaseCoin as StaticsBaseCoin } from '@bitgo/statics';
import { KeyPair as SubstrateKeyPair, Transaction } from './lib';
Expand All @@ -40,12 +42,6 @@ import { ApiPromise } from '@polkadot/api';

export const DEFAULT_SCAN_FACTOR = 20;

/**
* Discriminated union carrying keycard version and decrypted V1 user key (to avoid re-decryption).
* V1 keycards are JSON; V2 keycards are CBOR-encoded reduced key shares.
*/
type SubstrateSigningMaterial = { version: 'v1'; userPrv: string } | { version: 'v2'; encryptedUserKey: string };

export class SubstrateCoin extends BaseCoin {
protected readonly _staticsCoin: Readonly<StaticsBaseCoin>;
readonly MAX_VALIDITY_DURATION = 2400;
Expand Down Expand Up @@ -509,57 +505,14 @@ export class SubstrateCoin extends BaseCoin {
return { transactions: consolidationTransactions, lastScanIndex };
}

/**
* Decrypts an encrypted keychain value, wrapping errors with a descriptive message.
*/
private async decryptKeychain(encryptedKey: string, passphrase: string, label: string): Promise<string> {
const prv = await decryptKeychainPrivateKey(this.bitgo, { encryptedPrv: encryptedKey }, passphrase);
if (!prv) {
throw new Error(`Error decrypting ${label} keychain: invalid password or corrupted key`);
}
return prv;
}

/**
* Probes the key format and returns a discriminated union so callers avoid a second decrypt.
* V1 keycards are JSON; V2 keycards are CBOR-encoded reduced key shares.
*/
protected async isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<SubstrateSigningMaterial> {
const normalized = userKey.replace(/\s/g, '');
let isV1: boolean;
try {
isV1 = await EDDSAUtils.isEddsaMpcV1SigningMaterial(normalized, walletPassphrase, this.bitgo);
} catch (e) {
throw new Error(`Error decrypting user keychain: ${e instanceof Error ? e.message : String(e)}`);
}
if (isV1) {
const userPrv = await this.decryptKeychain(normalized, walletPassphrase, 'user');
return { version: 'v1', userPrv };
}
return { version: 'v2', encryptedUserKey: normalized };
}

// Protected so tests can stub via instance overrides without adding new test dependencies.
protected async getEddsaMpcV2RecoveryKeyShares(
encryptedUserKey: string,
encryptedBackupKey: string,
walletPassphrase: string
): ReturnType<typeof EDDSAUtils.getEddsaMpcV2RecoveryKeySharesFromReducedKey> {
return EDDSAUtils.getEddsaMpcV2RecoveryKeySharesFromReducedKey(
encryptedUserKey,
encryptedBackupKey,
walletPassphrase,
this.bitgo
);
protected async isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<EddsaSigningMaterial> {
return sharedIsMpcV2Keycard(userKey, walletPassphrase, this.bitgo);
Comment thread
vibhavgo marked this conversation as resolved.
}

// Protected so tests can stub via instance overrides without adding new test dependencies.
protected async signEddsaMpcV2Recovery(
signablePayload: Buffer,
currPath: string,
...args: Parameters<typeof EDDSAUtils.signRecoveryEddsaMPCv2> extends [Buffer, string, ...infer R] ? R : never
): Promise<Buffer> {
return EDDSAUtils.signRecoveryEddsaMPCv2(signablePayload, currPath, ...args);
// Protected so tests can stub via instance overrides — direct module function bindings
// cannot be intercepted by sinon after import.
protected async signSubstrateMpcV2Recovery(params: Parameters<typeof signEddsaMpcV2RecoveryTx>[0]): Promise<Buffer> {
return signEddsaMpcV2RecoveryTx(params);
}

/**
Expand All @@ -569,7 +522,7 @@ export class SubstrateCoin extends BaseCoin {
*/
protected async addSubstrateRecoverySignature(
txBuilder: NativeTransferBuilder,
signingMaterial: SubstrateSigningMaterial,
signingMaterial: EddsaSigningMaterial,
backupKey: string,
walletPassphrase: string,
unsignedTransaction: Transaction,
Expand All @@ -581,26 +534,23 @@ export class SubstrateCoin extends BaseCoin {
const substrateKeyPair = new SubstrateKeyPair({ pub: accountId });

if (signingMaterial.version === 'v2') {
const { userKeyShare, backupKeyShare, commonKeyChain } = await this.getEddsaMpcV2RecoveryKeyShares(
signingMaterial.encryptedUserKey,
const rawSig = await this.signSubstrateMpcV2Recovery({
message: unsignedTransaction.signablePayload,
userKey: signingMaterial.encryptedUserKey,
backupKey,
walletPassphrase
);
if (commonKeyChain.toLowerCase() !== bitgoKey.toLowerCase()) {
throw new Error('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey');
}
const rawSig = await this.signEddsaMpcV2Recovery(
unsignedTransaction.signablePayload,
currPath,
userKeyShare,
backupKeyShare,
commonKeyChain
);
walletPassphrase,
bitgoKey,
derivationPath: currPath,
bitgo: this.bitgo,
});
const substrateSig = Buffer.concat([Buffer.from([ED25519_MULTI_SIGNATURE_PREFIX]), rawSig]);
txBuilder.addSignature({ pub: substrateKeyPair.getKeys().pub }, substrateSig);
} else {
const userSigningMaterial = JSON.parse(signingMaterial.userPrv) as EDDSAMethodTypes.UserSigningMaterial;
const backupPrv = await this.decryptKeychain(backupKey, walletPassphrase, 'backup');
const backupPrv = await decryptKeychainPrivateKey(this.bitgo, { encryptedPrv: backupKey }, walletPassphrase);
if (!backupPrv) {
throw new Error('Error decrypting backup keychain: invalid password or corrupted key');
}
const backupSigningMaterial = JSON.parse(backupPrv) as EDDSAMethodTypes.BackupSigningMaterial;

const signatureHex = await EDDSAMethods.getTSSSignature(
Expand Down
22 changes: 7 additions & 15 deletions modules/abstract-substrate/test/unit/abstractSubstrateCoin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,9 @@ describe('SubstrateCoin MPCv2 recovery helpers:', function () {
});

describe('addSubstrateRecoverySignature()', function () {
// EDDSAUtils.* are exported via `export * as Namespace`, compiling to non-configurable
// property getters — sinon cannot replace them. Instead, SubstrateCoin exposes
// getEddsaMpcV2RecoveryKeyShares() and signEddsaMpcV2Recovery() as protected methods
// so they can be stubbed on the instance (own property shadows the prototype).
// signEddsaMpcV2RecoveryTx is a directly-imported module binding — sinon cannot intercept
// it after import. SubstrateCoin exposes signSubstrateMpcV2Recovery() as a protected
// wrapper so tests can stub it on the instance (own property shadows the prototype).
// EDDSAMethods.getTSSSignature is a regular writable property — sinon can stub it directly.
let addSignatureStub: sinon.SinonStub;
let coin: SubstrateCoinTestAccessor;
Expand All @@ -89,12 +88,7 @@ describe('SubstrateCoin MPCv2 recovery helpers:', function () {

it('should prepend ED25519 0x00 discriminant on MPCv2 path', async function () {
const rawSig = Buffer.alloc(64, 0xab);
sinon.stub(coin as unknown, 'getEddsaMpcV2RecoveryKeyShares').resolves({
userKeyShare: 'ks1',
backupKeyShare: 'ks2',
commonKeyChain: MOCK_BITGO_KEY,
});
sinon.stub(coin as unknown, 'signEddsaMpcV2Recovery').resolves(rawSig);
sinon.stub(coin as unknown, 'signSubstrateMpcV2Recovery').resolves(rawSig);

await coin.addSubstrateRecoverySignature(
{ addSignature: addSignatureStub },
Expand All @@ -114,11 +108,9 @@ describe('SubstrateCoin MPCv2 recovery helpers:', function () {
});

it('should throw when commonKeyChain does not match bitgoKey on MPCv2 path', async function () {
sinon.stub(coin as unknown, 'getEddsaMpcV2RecoveryKeyShares').resolves({
userKeyShare: 'ks1',
backupKeyShare: 'ks2',
commonKeyChain: 'mismatch',
});
sinon
.stub(coin as unknown, 'signSubstrateMpcV2Recovery')
.rejects(new Error('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey'));

@dpkjnr dpkjnr Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Drop this case (sdk-core already covers mismatch in signEddsaMpcV2RecoveryTx), or
  2. Keep a thin propagation/args assertion without hardcoding the failure inside the stub.


await coin
.addSubstrateRecoverySignature(
Expand Down
63 changes: 18 additions & 45 deletions modules/sdk-coin-sol/src/sol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ import {
DeriveAddressOptions,
DeriveAddressResult,
UnexpectedAddressError,
EDDSAUtils,
isMpcV2Keycard,
signEddsaMpcV2RecoveryTx,
} from '@bitgo/sdk-core';
import { auditEddsaPrivateKey, getDerivationPath } from '@bitgo/sdk-lib-mpc';
import { BaseNetwork, CoinFamily, coins, SolCoin, BaseCoin as StaticsBaseCoin } from '@bitgo/statics';
Expand Down Expand Up @@ -1700,7 +1701,7 @@ export class Sol extends BaseCoin {
const userKey = params.userKey?.replace(/\s/g, '') ?? '';

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

@dpkjnr dpkjnr Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

: false;

const index = params.index || 0;
Expand Down Expand Up @@ -1819,7 +1820,7 @@ export class Sol extends BaseCoin {
// Detect once at the top to avoid decrypting the keycard on every iteration of the scan loop.
// For unsigned sweep (no passphrase), isMpcV2 is false — cold MPCv2 is out of scope.
const isMpcV2 = params.walletPassphrase
? !(await EDDSAUtils.isEddsaMpcV1SigningMaterial(userKey, params.walletPassphrase, this.bitgo))
? (await isMpcV2Keycard(userKey, params.walletPassphrase, this.bitgo)).version === 'v2'
: false;

const baseAddressIndex = 0;
Expand Down Expand Up @@ -1963,25 +1964,15 @@ export class Sol extends BaseCoin {
);
txBuilder.addSignature({ pub: bs58EncodedPublicKey } as PublicKey, signatureHex);
} else {
const { userKeyShare, backupKeyShare, commonKeyChain } =
await EDDSAUtils.getEddsaMpcV2RecoveryKeySharesFromReducedKey(
userKey,
backupKey,
params.walletPassphrase!,
this.bitgo
);

if (commonKeyChain.toLowerCase() !== bitgoKey.toLowerCase()) {
throw new Error('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey');
}

const signature = await EDDSAUtils.signRecoveryEddsaMPCv2(
unsignedTransaction.signablePayload,
currPath,
userKeyShare,
backupKeyShare,
commonKeyChain
);
const signature = await signEddsaMpcV2RecoveryTx({
message: unsignedTransaction.signablePayload,
userKey,
backupKey,
walletPassphrase: params.walletPassphrase!,
bitgoKey,
derivationPath: currPath,
bitgo: this.bitgo,
});
txBuilder.addSignature({ pub: bs58EncodedPublicKey } as PublicKey, signature);
}
}
Expand All @@ -1991,29 +1982,11 @@ export class Sol extends BaseCoin {
backupKey?: string,
walletPassphrase?: string
): Promise<boolean> {
let isMpcV2 = false;
if (walletPassphrase) {
if (!userKey) {
throw new Error('missing userKey');
}
if (!backupKey) {
throw new Error('missing backupKey');
}
// Detect MPCv2 keycards — will throw if decryption fails (e.g., wrong password).
// MPCv1 keycards decrypt to JSON with uShare/bitgoYShare; MPCv2 keycards are CBOR.
try {
const isV1 = await EDDSAUtils.isEddsaMpcV1SigningMaterial(
userKey.replace(/\s/g, ''),
walletPassphrase,
this.bitgo
);
isMpcV2 = !isV1;
} catch (e) {
// Re-wrap decryption errors with context
throw new Error(`Error decrypting user keychain: ${e instanceof Error ? e.message : String(e)}`);
}
}
return isMpcV2;
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);

@dpkjnr dpkjnr Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return material.version === 'v2';
}

async broadcastTransaction({
Expand Down
49 changes: 15 additions & 34 deletions modules/sdk-coin-ton/src/ton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import {
BitGoBase,
decryptKeychainPrivateKey,
EDDSAMethods,
EDDSAUtils,
isMpcV2Keycard as sharedIsMpcV2Keycard,
signEddsaMpcV2RecoveryTx,
EddsaSigningMaterial,
InvalidAddressError,
KeyPair,
MPCAlgorithm,
Expand Down Expand Up @@ -46,8 +48,6 @@ export interface TonParseTransactionOptions extends ParseTransactionOptions {
toAddressBounceable?: boolean;
}

type TonSigningMaterial = { version: 'v1'; userPrv: string } | { version: 'v2'; encryptedUserKey: string };

export class Ton extends BaseCoin {
protected readonly _staticsCoin: Readonly<StaticsBaseCoin>;
protected constructor(bitgo: BitGoBase, staticsCoin?: Readonly<StaticsBaseCoin>) {
Expand Down Expand Up @@ -323,19 +323,8 @@ export class Ton extends BaseCoin {
* Discriminated union carrying keycard version and decrypted V1 user key (to avoid re-decryption).
* V1 keycards are JSON; V2 keycards are CBOR-encoded reduced key shares.
*/
private async isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<TonSigningMaterial> {
const normalized = userKey.replace(/\s/g, '');
let isV1: boolean;
try {
isV1 = await EDDSAUtils.isEddsaMpcV1SigningMaterial(normalized, walletPassphrase, this.bitgo);
} catch (e) {
throw new Error(`Error decrypting user keychain: ${e instanceof Error ? e.message : String(e)}`);
}
if (isV1) {
const userPrv = await this.decryptKeychain(normalized, walletPassphrase, 'user');
return { version: 'v1', userPrv };
}
return { version: 'v2', encryptedUserKey: normalized };
private async isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<EddsaSigningMaterial> {

@dpkjnr dpkjnr Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return sharedIsMpcV2Keycard(userKey, walletPassphrase, this.bitgo);
}

private async decryptKeychain(encryptedKey: string, passphrase: string, label: string): Promise<string> {
Expand All @@ -347,7 +336,7 @@ export class Ton extends BaseCoin {
}

private async addRecoverySignature(
signingMaterial: TonSigningMaterial,
signingMaterial: EddsaSigningMaterial,
txBuilder: TransactionBuilder,
senderAddr: string,
unsignedTransaction: any,
Expand All @@ -357,23 +346,15 @@ export class Ton extends BaseCoin {
walletPassphrase: string
): Promise<void> {
if (signingMaterial.version === 'v2') {
const { userKeyShare, backupKeyShare, commonKeyChain } =
await EDDSAUtils.getEddsaMpcV2RecoveryKeySharesFromReducedKey(
signingMaterial.encryptedUserKey,
backupKey,
walletPassphrase,
this.bitgo
);
if (commonKeyChain.toLowerCase() !== bitgoKey.toLowerCase()) {
throw new Error('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey');
}
const signature = await EDDSAUtils.signRecoveryEddsaMPCv2(
unsignedTransaction.signablePayload,
currPath,
userKeyShare,
backupKeyShare,
commonKeyChain
);
const signature = await signEddsaMpcV2RecoveryTx({
message: unsignedTransaction.signablePayload,
userKey: signingMaterial.encryptedUserKey,
backupKey,
walletPassphrase,
bitgoKey,
derivationPath: currPath,
bitgo: this.bitgo,
});
txBuilder.addSignature({ pub: senderAddr } as PublicKey, signature);
} else {
const userSigningMaterial = JSON.parse(signingMaterial.userPrv) as EDDSAMethodTypes.UserSigningMaterial;
Expand Down
Loading
Loading