Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
155 changes: 121 additions & 34 deletions modules/abstract-substrate/src/abstractSubstrateCoin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
UnexpectedAddressError,
verifyEddsaTssWalletAddress,
VerifyTransactionOptions,
EDDSAUtils,
decryptKeychainPrivateKey,
} from '@bitgo/sdk-core';
import { CoinFamily, BaseCoin as StaticsBaseCoin } from '@bitgo/statics';
import { KeyPair as SubstrateKeyPair, Transaction } from './lib';
Expand All @@ -38,6 +40,12 @@ 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 @@ -356,42 +364,17 @@ export class SubstrateCoin extends BaseCoin {
throw new Error('missing wallet passphrase');
}

const userKey = params.userKey.replace(/\s/g, '');
const backupKey = params.backupKey.replace(/\s/g, '');

// Decrypt private keys from KeyCard values
let userPrv;
try {
userPrv = await this.bitgo.decrypt({
input: userKey,
password: params.walletPassphrase,
});
} catch (e) {
throw new Error(`Error decrypting user keychain: ${e.message}`);
}
const userSigningMaterial = JSON.parse(userPrv) as EDDSAMethodTypes.UserSigningMaterial;

let backupPrv;
try {
backupPrv = await this.bitgo.decrypt({
input: backupKey,
password: params.walletPassphrase,
});
} catch (e) {
throw new Error(`Error decrypting backup keychain: ${e.message}`);
}
const backupSigningMaterial = JSON.parse(backupPrv) as EDDSAMethodTypes.BackupSigningMaterial;

// add signature
const signatureHex = await EDDSAMethods.getTSSSignature(
userSigningMaterial,
backupSigningMaterial,
const signingMaterial = await this.isMpcV2Keycard(params.userKey!, params.walletPassphrase!);
await this.addSubstrateRecoverySignature(
Comment thread
vibhavgo marked this conversation as resolved.
txBuilder,
signingMaterial,
params.backupKey!.replace(/\s/g, ''),
params.walletPassphrase!,
unsignedTransaction,
currPath,
unsignedTransaction
bitgoKey,
accountId
);

const substrateKeyPair = new SubstrateKeyPair({ pub: accountId });
txBuilder.addSignature({ pub: substrateKeyPair.getKeys().pub }, signatureHex);
const signedTransaction = await txBuilder.build();
serializedTx = signedTransaction.toBroadcastFormat();
} else {
Expand Down Expand Up @@ -526,6 +509,110 @@ 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;
}
Comment thread
vibhavgo marked this conversation as resolved.

/**
* 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 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);
}

/**
* Adds an MPCv1 or MPCv2 signature to a Substrate transaction builder.
* MPCv2 signatures are prefixed with ED25519_MULTI_SIGNATURE_PREFIX (Ed25519 discriminant
* in the Substrate MultiSignature enum).
*/
protected async addSubstrateRecoverySignature(

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 method has no unit tests. It's the critical signing path — it branches on MPCv1 vs MPCv2, prepends the 0x00 Ed25519 discriminant for MPCv2, and calls into EDDSAUtils.signRecoveryEddsaMPCv2. A bug here would silently produce an invalid signature at recovery time.

The existing tests only cover isMpcV2Keycard(). Would suggest adding at least:

  • MPCv2 path: verify the 0x00 prefix is prepended to the raw signature
  • MPCv1 path: verify getTSSSignature is called and the result passed to addSignature
  • commonKeyChain mismatch: verify the guard throws

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since this was a refactor to create common utility for children of the substrate class
Earlier had decided to add tests in the child consumer PRs for MPCv2 path
But I agree, will also add tests here

txBuilder: NativeTransferBuilder,
signingMaterial: SubstrateSigningMaterial,
backupKey: string,
walletPassphrase: string,
unsignedTransaction: Transaction,
currPath: string,
bitgoKey: string,
accountId: string
): Promise<void> {
const ED25519_MULTI_SIGNATURE_PREFIX = 0x00;
const substrateKeyPair = new SubstrateKeyPair({ pub: accountId });

if (signingMaterial.version === 'v2') {
const { userKeyShare, backupKeyShare, commonKeyChain } = await this.getEddsaMpcV2RecoveryKeyShares(
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
);
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 backupSigningMaterial = JSON.parse(backupPrv) as EDDSAMethodTypes.BackupSigningMaterial;

const signatureHex = await EDDSAMethods.getTSSSignature(
userSigningMaterial,
backupSigningMaterial,
currPath,
unsignedTransaction
);
txBuilder.addSignature({ pub: substrateKeyPair.getKeys().pub }, signatureHex);
}
}

/** inherited doc */
async createBroadcastableSweepTransaction(params: MPCSweepRecoveryOptions): Promise<MPCTxs> {
const req = params.signatureShares;
Expand Down
167 changes: 167 additions & 0 deletions modules/abstract-substrate/test/unit/abstractSubstrateCoin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
import * as sinon from 'sinon';
import * as sdkCore from '@bitgo/sdk-core';
// Cross-module relative import: tsx resolves TypeScript source directly in the monorepo,
// avoiding a circular devDependency (sdk-coin-tao depends on abstract-substrate at runtime).
import { Ttao } from '../../../sdk-coin-tao/src';

interface SubstrateCoinTestAccessor {
isMpcV2Keycard(userKey: string, walletPassphrase: string): Promise<{ version: 'v1' | 'v2' }>;
addSubstrateRecoverySignature(
txBuilder: unknown,
signingMaterial: unknown,
backupKey: string,
walletPassphrase: string,
unsignedTransaction: unknown,
currPath: string,
bitgoKey: string,
accountId: string
): Promise<void>;
}

// 32-byte all-zeros hex: minimal valid Ed25519 public key for SubstrateKeyPair construction.
const MOCK_ACCOUNT_ID = '0'.repeat(64);
const MOCK_BITGO_KEY = 'aa'.repeat(32);
const MOCK_UNSIGNED_TX = { signablePayload: Buffer.from('deadbeef', 'hex') };

describe('SubstrateCoin MPCv2 recovery helpers:', function () {
const sandBox = sinon.createSandbox();
// Bypass the constructor (which requires a full BitGoBase) — we only need the
// prototype methods.
const basecoin = Object.create(Ttao.prototype) as Ttao & SubstrateCoinTestAccessor;
// isEddsaMpcV1SigningMaterial is gated behind non-configurable namespace getters that
// sinon cannot replace. Provide bitgo.decrypt so it uses that path instead of sjcl,
// then control V1 vs V2 detection by returning JSON (V1) or non-JSON (V2).
let decryptStub: sinon.SinonStub;

beforeEach(function () {
decryptStub = sinon.stub();
(basecoin as unknown as { bitgo: unknown }).bitgo = { decrypt: decryptStub };
});

afterEach(function () {
sandBox.restore();
});

describe('isMpcV2Keycard()', function () {
it('should return version v2 for a CBOR (MPCv2) keycard', async function () {
// Non-JSON decrypted value → V2 CBOR keycard
decryptStub.resolves('not-json-cbor-bytes');
const result = await basecoin.isMpcV2Keycard('encryptedKey', 'passphrase');
result.version.should.equal('v2');
});

it('should return version v1 for a JSON (MPCv1) keycard', async function () {
// isMpcV2Keycard checks uShare.seed + bitgoYShare.u + backupYShare.u
decryptStub.resolves(
JSON.stringify({
uShare: { seed: 'deadbeef' },
bitgoYShare: { u: 'aabbcc' },
backupYShare: { u: 'ddeeff' },
})
);
const result = await basecoin.isMpcV2Keycard('encryptedKey', 'passphrase');
result.version.should.equal('v1');
});

it('should throw with a descriptive message when decryption fails', async function () {
decryptStub.rejects(new Error('bad password'));
await basecoin
.isMpcV2Keycard('encryptedKey', 'wrong-passphrase')
.should.be.rejectedWith(/Error decrypting user keychain/);
});
});

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).
// EDDSAMethods.getTSSSignature is a regular writable property — sinon can stub it directly.
let addSignatureStub: sinon.SinonStub;
let coin: SubstrateCoinTestAccessor;

beforeEach(function () {
addSignatureStub = sinon.stub();
const instanceDecryptStub = sinon.stub();
coin = Object.create(Ttao.prototype) as Ttao & SubstrateCoinTestAccessor;
(coin as unknown as { bitgo: unknown }).bitgo = { decrypt: instanceDecryptStub };
});

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);

await coin.addSubstrateRecoverySignature(
{ addSignature: addSignatureStub },
{ version: 'v2', encryptedUserKey: 'encKey' },
'encBackupKey',
'passphrase',
MOCK_UNSIGNED_TX,
'm/0',
MOCK_BITGO_KEY,
MOCK_ACCOUNT_ID
);

addSignatureStub.calledOnce.should.be.true();
const sig: Buffer = addSignatureStub.firstCall.args[1];
sig[0].should.equal(0x00);
sig.slice(1).should.deepEqual(rawSig);
});

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',
});

await coin
.addSubstrateRecoverySignature(
{ addSignature: addSignatureStub },
{ version: 'v2', encryptedUserKey: 'encKey' },
'encBackupKey',
'passphrase',
MOCK_UNSIGNED_TX,
'm/0',
MOCK_BITGO_KEY,
MOCK_ACCOUNT_ID
)
.should.be.rejectedWith(/commonKeyChain from keycard does not match bitgoKey/);
});

it('should call getTSSSignature and pass result to addSignature on MPCv1 path', async function () {
const mockSig = 'ff'.repeat(64);
sandBox.stub(sdkCore.EDDSAMethods, 'getTSSSignature').resolves(mockSig);
// decryptKeychain calls decryptKeychainPrivateKey → bitgo.decrypt for the backup key
(coin as unknown as { bitgo: { decrypt: sinon.SinonStub } }).bitgo.decrypt.resolves(
JSON.stringify({ yShares: {} })
);

const userPrv = JSON.stringify({
uShare: { seed: 'deadbeef' },
bitgoYShare: { u: 'aabbcc' },
backupYShare: { u: 'ddeeff' },
});

await coin.addSubstrateRecoverySignature(
{ addSignature: addSignatureStub },
{ version: 'v1', userPrv },
'encBackupKey',
'passphrase',
MOCK_UNSIGNED_TX,
'm/0',
MOCK_BITGO_KEY,
MOCK_ACCOUNT_ID
);

(sdkCore.EDDSAMethods.getTSSSignature as sinon.SinonStub).calledOnce.should.be.true();
addSignatureStub.calledOnce.should.be.true();
addSignatureStub.firstCall.args[1].should.equal(mockSig);
});
});
});
Loading