Skip to content

Commit

Permalink
fix(sdk-core): choose correct default apiVersion for tss
Browse files Browse the repository at this point in the history
TICKET: WP-3038
  • Loading branch information
zahin-mohammad authored and alvin-dai-bitgo committed Dec 13, 2024
1 parent f6e5392 commit c3bc7f0
Show file tree
Hide file tree
Showing 7 changed files with 178 additions and 42 deletions.
2 changes: 2 additions & 0 deletions modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ const createKeccakHash = require('keccak');
const encryptNShare = ECDSAMethods.encryptNShare;
type KeyShare = ECDSA.KeyShare;

openpgp.config.rejectCurves = new Set();

describe('TSS Ecdsa Utils:', async function () {
const isThirdPartyBackup = false;
const coinName = 'hteth';
Expand Down
10 changes: 5 additions & 5 deletions modules/bitgo/test/v2/unit/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2417,15 +2417,15 @@ describe('V2 Wallet:', function () {
type: 'transfer',
};

['eddsa', 'ecdsa'].forEach((keyCurvve: string) => {
describe(keyCurvve, () => {
const wallet = keyCurvve === 'eddsa' ? tssSolWallet : tssEthWallet;
['eddsa', 'ecdsa'].forEach((keyCurve: string) => {
describe(keyCurve, () => {
const wallet = keyCurve === 'eddsa' ? tssSolWallet : tssEthWallet;

beforeEach(function () {
sandbox
.stub(Keychains.prototype, 'getKeysForSigning')
.resolves([{ commonKeychain: 'test', id: '', pub: '', type: 'independent' }]);
if (keyCurvve === 'eddsa') {
if (keyCurve === 'eddsa') {
sandbox.stub(Tsol.prototype, 'verifyTransaction').resolves(true);
} else {
sandbox.stub(Teth.prototype, 'verifyTransaction').resolves(true);
Expand Down Expand Up @@ -2679,7 +2679,7 @@ describe('V2 Wallet:', function () {
],
type: 'transfer',
})
.should.be.rejectedWith(`Custodial and ECDSA MPC algorithm must always use 'full' api version`);
.should.be.rejectedWith('For non self-custodial (hot) tss wallets, parameter `apiVersion` must be `full`.');
});

it('should build a single recipient transfer transaction for full', async function () {
Expand Down
14 changes: 6 additions & 8 deletions modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,20 +530,18 @@ export default class BaseTssUtils<KeyShare> extends MpcUtils implements ITssUtil

/**
* Returns supported TxRequest versions for this wallet
* @deprecated Whenever needed, use apiVersion 'full' for TSS wallets
*/
public supportedTxRequestVersions(): TxRequestVersion[] {
const walletType = this._wallet?.type();
const supportedWalletTypes = ['custodial', 'cold', 'hot'];
if (!walletType || this._wallet?.multisigType() !== 'tss' || !supportedWalletTypes.includes(walletType)) {
if (!this._wallet || this._wallet.type() === 'trading' || this._wallet.multisigType() !== 'tss') {
return [];
} else if (this._wallet?.baseCoin.getMPCAlgorithm() === 'ecdsa') {
} else if (this._wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
return ['full'];
} else if (walletType === 'custodial' || walletType === 'cold') {
return ['full'];
} else if (this._wallet?.baseCoin.getMPCAlgorithm() === 'eddsa' && walletType === 'hot') {
} else if (this._wallet.baseCoin.getMPCAlgorithm() === 'eddsa' && this._wallet.type() === 'hot') {
return ['lite', 'full'];
} else {
return ['full'];
}
return [];
}

/**
Expand Down
42 changes: 42 additions & 0 deletions modules/sdk-core/src/bitgo/utils/txRequest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { ApiVersion, IWallet } from '../wallet';
import assert from 'assert';

export function validateTxRequestApiVersion(wallet: IWallet, requestedApiVersion: ApiVersion): void {
if (wallet.multisigType() !== 'tss') {
// only tss wallets have api version requirements
return;
}
if (wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
// ecdsa wallets can only use full, even if they are hot wallets
assert(requestedApiVersion === 'full', 'For ECDSA tss wallets, parameter `apiVersion` must be `full`.');
} else if (wallet.type() !== 'hot') {
// all other cases should use full!
assert(
requestedApiVersion === 'full',
'For non self-custodial (hot) tss wallets, parameter `apiVersion` must be `full`.'
);
}
return;
}

/**
* Get the api version for the provided wallet.
* If the user requested api version is invalid, this will throw an error.
* @param wallet
* @param requestedApiVersion
*/
export function getTxRequestApiVersion(wallet: IWallet, requestedApiVersion?: ApiVersion): ApiVersion {
if (requestedApiVersion) {
validateTxRequestApiVersion(wallet, requestedApiVersion);
return requestedApiVersion;
}
if (wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
return 'full';
} else if (wallet.type() === 'hot') {
// default to lite for hot eddsa tss wallets
return 'lite';
} else {
// default to full for all other wallet types
return 'full';
}
}
2 changes: 1 addition & 1 deletion modules/sdk-core/src/bitgo/wallet/iWallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ export interface IWallet {
nftBalances(): NftBalance[] | undefined;
unsupportedNftBalances(): NftBalance[] | undefined;
coin(): string;
type(): WalletType | undefined;
type(): WalletType;
multisigType(): 'onchain' | 'tss';
multisigTypeVersion(): 'MPCv2' | undefined;
label(): string;
Expand Down
36 changes: 8 additions & 28 deletions modules/sdk-core/src/bitgo/wallet/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ import { postWithCodec } from '../utils/postWithCodec';
import { TxSendBody } from '@bitgo/public-types';
import { AddressBook, IAddressBook } from '../address-book';
import { IRequestTracer } from '../../api';
import { getTxRequestApiVersion, validateTxRequestApiVersion } from '../utils/txRequest';

const debug = require('debug')('bitgo:v2:wallet');

Expand Down Expand Up @@ -263,8 +264,8 @@ export class Wallet implements IWallet {
return this._wallet.coin;
}

type(): WalletType | undefined {
return this._wallet.type;
type(): WalletType {
return this._wallet.type || 'hot';
}

multisigType(): 'onchain' | 'tss' {
Expand Down Expand Up @@ -2096,11 +2097,8 @@ export class Wallet implements IWallet {
error.code = 'recipients_not_allowed_for_fillnonce_and_acceleration_tx_type';
throw error;
}
const supportedTxRequestVersions = this.tssUtils?.supportedTxRequestVersions() || [];
if (params.apiVersion && !supportedTxRequestVersions.includes(params.apiVersion)) {
throw new Error(
`prebuildAndSignTransaction params.apiVersion=${params.apiVersion} must be one of ${supportedTxRequestVersions}`
);
if (params.apiVersion) {
validateTxRequestApiVersion(this, params.apiVersion);
}

// Doing a sanity check for password here to avoid doing further work if we know it's wrong
Expand Down Expand Up @@ -3087,19 +3085,7 @@ export class Wallet implements IWallet {
private async prebuildTransactionTss(params: PrebuildTransactionOptions = {}): Promise<PrebuildTransactionResult> {
const reqId = params.reqId || new RequestTracer();
this.bitgo.setRequestTracer(reqId);

if (
params.apiVersion === 'lite' &&
(this._wallet.type === 'custodial' || this._wallet.type === 'cold' || this.baseCoin.getMPCAlgorithm() === 'ecdsa')
) {
throw new Error(`Custodial and ECDSA MPC algorithm must always use 'full' api version`);
}

const apiVersion =
params.apiVersion ||
(this._wallet.type === 'custodial' || this._wallet.type === 'cold' || this.baseCoin.getMPCAlgorithm() === 'ecdsa'
? 'full'
: 'lite');
const apiVersion = getTxRequestApiVersion(this, params.apiVersion);
// Two options different implementations of fees seems to now be supported, for now we will support both to be backwards compatible
// TODO(BG-59685): deprecate one of these so that we have a single way to pass fees
let feeOptions;
Expand Down Expand Up @@ -3558,20 +3544,14 @@ export class Wallet implements IWallet {
* @param params send options
*/
private async sendManyTss(params: SendManyOptions = {}): Promise<any> {
const { apiVersion } = params;
const supportedTxRequestVersions = this.tssUtils?.supportedTxRequestVersions() ?? [];
const onlySupportsTxRequestFull =
supportedTxRequestVersions.length === 1 && supportedTxRequestVersions.includes('full');
if (apiVersion === 'lite' && onlySupportsTxRequestFull) {
throw new Error('TxRequest Lite API is not supported for this wallet');
}
params.apiVersion = getTxRequestApiVersion(this, params.apiVersion);

const signedTransaction = (await this.prebuildAndSignTransaction(params)) as SignedTransactionRequest;
if (!signedTransaction.txRequestId) {
throw new Error('txRequestId missing from signed transaction');
}

if (onlySupportsTxRequestFull || apiVersion === 'full') {
if (params.apiVersion === 'full') {
const latestTxRequest = await getTxRequest(this.bitgo, this.id(), signedTransaction.txRequestId, params.reqId);
const reqId = params.reqId || new RequestTracer();
this.bitgo.setRequestTracer(reqId);
Expand Down
114 changes: 114 additions & 0 deletions modules/sdk-core/test/unit/bitgo/utils/txRequest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import 'should';
import { ApiVersion, IWallet } from '../../../../src';
import { getTxRequestApiVersion } from '../../../../src/bitgo/utils/txRequest';

describe('txRequest utils', () => {
describe('getTxRequestApiVersion', function () {
const testCases = [
{
wallet: {
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
type: () => 'hot',
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: 'lite',
expectedApiVersion: '',
expectedErrorMessage: 'For ECDSA tss wallets, parameter `apiVersion` must be `full`.',
},
{
wallet: {
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
type: () => 'cold',
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: 'lite',
expectedApiVersion: '',
expectedErrorMessage: 'For non self-custodial (hot) tss wallets, parameter `apiVersion` must be `full`.',
},
{
wallet: {
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
type: () => 'hot',
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: undefined,
expectedApiVersion: 'lite',
expectedErrorMessage: '',
},
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
return {
wallet: {
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
type: () => walletType,
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: 'full',
expectedApiVersion: 'full',
expectedErrorMessage: '',
shouldThrow: false,
};
}),
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
return {
wallet: {
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
type: () => walletType,
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: undefined,
expectedApiVersion: 'full',
expectedErrorMessage: '',
shouldThrow: false,
};
}),
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
return {
wallet: {
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
type: () => walletType,
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: 'full',
expectedApiVersion: 'full',
expectedErrorMessage: '',
shouldThrow: false,
};
}),
...['cold', 'custodial', 'backing'].map((walletType) => {
return {
wallet: {
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
type: () => walletType,
multisigType: () => 'tss',
} as any as IWallet,
requestedApiVersion: undefined,
expectedApiVersion: 'full',
expectedErrorMessage: '',
shouldThrow: false,
};
}),
];

testCases.forEach((testCase) => {
if (testCase.expectedErrorMessage) {
it(`should throw an error if requested apiVersion is ${
testCase.requestedApiVersion
} for wallet type ${testCase.wallet.type()} for a ${testCase.wallet.baseCoin.getMPCAlgorithm()} wallet`, () => {
(() =>
getTxRequestApiVersion(
testCase.wallet,
testCase.requestedApiVersion as ApiVersion | undefined
)).should.throw(testCase.expectedErrorMessage);
});
} else {
it(`should return ${testCase.expectedApiVersion} if requested apiVersion is ${
testCase.requestedApiVersion
} for wallet type ${testCase.wallet.type()} for a ${testCase.wallet.baseCoin.getMPCAlgorithm()} wallet`, () => {
getTxRequestApiVersion(testCase.wallet, testCase.requestedApiVersion as ApiVersion | undefined).should.equal(
testCase.expectedApiVersion
);
});
}
});
});
});

0 comments on commit c3bc7f0

Please sign in to comment.