Skip to content

Commit c075a38

Browse files
authored
Merge pull request #5106 from BitGo/WP-3038/fix-tx-request-api-version
fix(sdk-core): choose correct default apiVersion for tss
2 parents 8117ff6 + 1bfcc36 commit c075a38

File tree

8 files changed

+193
-47
lines changed

8 files changed

+193
-47
lines changed

modules/bitgo/test/v2/unit/coins/utxo/prebuildAndSign.ts

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,17 +96,23 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
9696
feeMultiplier?: number;
9797
selfSend?: boolean;
9898
nockOutputAddresses?: boolean;
99+
txFormat?: string;
99100
}): nock.Scope[] {
100101
const nocks: nock.Scope[] = [];
101102

102103
// Nock the prebuild route (/tx/build, blockheight)
104+
const expected_params = {
105+
recipients: [params.recipient],
106+
rbfTxIds: params.rbfTxIds,
107+
feeMultiplier: params.feeMultiplier,
108+
changeAddressType: ['p2trMusig2', 'p2wsh', 'p2shP2wsh', 'p2sh', 'p2tr'],
109+
};
110+
if (params.txFormat) {
111+
expected_params['txFormat'] = params.txFormat;
112+
}
103113
nocks.push(
104114
nock(params.bgUrl)
105-
.post(`/api/v2/${coin.getChain()}/wallet/${params.wallet.id()}/tx/build`, {
106-
recipients: [params.recipient],
107-
rbfTxIds: params.rbfTxIds,
108-
feeMultiplier: params.feeMultiplier,
109-
})
115+
.post(`/api/v2/${coin.getChain()}/wallet/${params.wallet.id()}/tx/build`, expected_params)
110116
.reply(200, { txHex: params.prebuild.toHex(), txInfo: {} })
111117
);
112118
nocks.push(nock(params.bgUrl).get(`/api/v2/${coin.getChain()}/public/block/latest`).reply(200, { height: 1000 }));
@@ -212,6 +218,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
212218

213219
[true, false].forEach((useWebauthn) => {
214220
it(`should succeed with ${useWebauthn ? 'webauthn encryptedPrv' : 'encryptedPrv'}`, async function () {
221+
const txCoins = ['tzec', 'zec', 'ltc', 'bcha', 'doge', 'dash', 'btg', 'bch'];
215222
const nocks = createNocks({
216223
bgUrl,
217224
wallet,
@@ -220,6 +227,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
220227
recipient,
221228
addressInfo,
222229
nockOutputAddresses: txFormat !== 'psbt',
230+
txFormat: !txCoins.includes(coin.getChain()) ? 'psbt' : undefined,
223231
});
224232

225233
// call prebuild and sign, nocks should be consumed
@@ -257,6 +265,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
257265
it(`should be able to build, sign, & verify a replacement transaction with selfSend: ${selfSend}`, async function () {
258266
const rbfTxIds = ['tx-to-be-replaced'],
259267
feeMultiplier = 1.5;
268+
const txCoins = ['tzec', 'zec', 'ltc', 'bcha', 'doge', 'dash', 'btg', 'bch'];
260269
const nocks = createNocks({
261270
bgUrl,
262271
wallet,
@@ -268,6 +277,7 @@ function run(coin: AbstractUtxoCoin, inputScripts: ScriptType[], txFormat: TxFor
268277
feeMultiplier,
269278
selfSend,
270279
nockOutputAddresses: txFormat !== 'psbt',
280+
txFormat: !txCoins.includes(coin.getChain()) ? 'psbt' : undefined,
271281
});
272282

273283
// call prebuild and sign, nocks should be consumed

modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ const createKeccakHash = require('keccak');
5555
const encryptNShare = ECDSAMethods.encryptNShare;
5656
type KeyShare = ECDSA.KeyShare;
5757

58+
openpgp.config.rejectCurves = new Set();
59+
5860
describe('TSS Ecdsa Utils:', async function () {
5961
const isThirdPartyBackup = false;
6062
const coinName = 'hteth';

modules/bitgo/test/v2/unit/wallet.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2417,15 +2417,15 @@ describe('V2 Wallet:', function () {
24172417
type: 'transfer',
24182418
};
24192419

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

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

26852685
it('should build a single recipient transfer transaction for full', async function () {

modules/sdk-core/src/bitgo/utils/tss/baseTSSUtils.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -530,20 +530,18 @@ export default class BaseTssUtils<KeyShare> extends MpcUtils implements ITssUtil
530530

531531
/**
532532
* Returns supported TxRequest versions for this wallet
533+
* @deprecated Whenever needed, use apiVersion 'full' for TSS wallets
533534
*/
534535
public supportedTxRequestVersions(): TxRequestVersion[] {
535-
const walletType = this._wallet?.type();
536-
const supportedWalletTypes = ['custodial', 'cold', 'hot'];
537-
if (!walletType || this._wallet?.multisigType() !== 'tss' || !supportedWalletTypes.includes(walletType)) {
536+
if (!this._wallet || this._wallet.type() === 'trading' || this._wallet.multisigType() !== 'tss') {
538537
return [];
539-
} else if (this._wallet?.baseCoin.getMPCAlgorithm() === 'ecdsa') {
538+
} else if (this._wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
540539
return ['full'];
541-
} else if (walletType === 'custodial' || walletType === 'cold') {
542-
return ['full'];
543-
} else if (this._wallet?.baseCoin.getMPCAlgorithm() === 'eddsa' && walletType === 'hot') {
540+
} else if (this._wallet.baseCoin.getMPCAlgorithm() === 'eddsa' && this._wallet.type() === 'hot') {
544541
return ['lite', 'full'];
542+
} else {
543+
return ['full'];
545544
}
546-
return [];
547545
}
548546

549547
/**
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import { ApiVersion, IWallet } from '../wallet';
2+
import assert from 'assert';
3+
4+
export function validateTxRequestApiVersion(wallet: IWallet, requestedApiVersion: ApiVersion): void {
5+
if (wallet.multisigType() !== 'tss') {
6+
// only tss wallets have api version requirements
7+
return;
8+
}
9+
if (wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
10+
// ecdsa wallets can only use full, even if they are hot wallets
11+
assert(requestedApiVersion === 'full', 'For ECDSA tss wallets, parameter `apiVersion` must be `full`.');
12+
} else if (wallet.type() !== 'hot') {
13+
// all other cases should use full!
14+
assert(
15+
requestedApiVersion === 'full',
16+
'For non self-custodial (hot) tss wallets, parameter `apiVersion` must be `full`.'
17+
);
18+
}
19+
return;
20+
}
21+
22+
/**
23+
* Get the api version for the provided wallet.
24+
* If the user requested api version is invalid, this will throw an error.
25+
* @param wallet
26+
* @param requestedApiVersion
27+
*/
28+
export function getTxRequestApiVersion(wallet: IWallet, requestedApiVersion?: ApiVersion): ApiVersion {
29+
if (requestedApiVersion) {
30+
validateTxRequestApiVersion(wallet, requestedApiVersion);
31+
return requestedApiVersion;
32+
}
33+
if (wallet.baseCoin.getMPCAlgorithm() === 'ecdsa') {
34+
return 'full';
35+
} else if (wallet.type() === 'hot') {
36+
// default to lite for hot eddsa tss wallets
37+
return 'lite';
38+
} else {
39+
// default to full for all other wallet types
40+
return 'full';
41+
}
42+
}

modules/sdk-core/src/bitgo/wallet/iWallet.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -808,7 +808,7 @@ export interface IWallet {
808808
nftBalances(): NftBalance[] | undefined;
809809
unsupportedNftBalances(): NftBalance[] | undefined;
810810
coin(): string;
811-
type(): WalletType | undefined;
811+
type(): WalletType;
812812
multisigType(): 'onchain' | 'tss';
813813
multisigTypeVersion(): 'MPCv2' | undefined;
814814
label(): string;

modules/sdk-core/src/bitgo/wallet/wallet.ts

Lines changed: 8 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ import { postWithCodec } from '../utils/postWithCodec';
109109
import { TxSendBody } from '@bitgo/public-types';
110110
import { AddressBook, IAddressBook } from '../address-book';
111111
import { IRequestTracer } from '../../api';
112+
import { getTxRequestApiVersion, validateTxRequestApiVersion } from '../utils/txRequest';
112113

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

@@ -263,8 +264,8 @@ export class Wallet implements IWallet {
263264
return this._wallet.coin;
264265
}
265266

266-
type(): WalletType | undefined {
267-
return this._wallet.type;
267+
type(): WalletType {
268+
return this._wallet.type || 'hot';
268269
}
269270

270271
multisigType(): 'onchain' | 'tss' {
@@ -2096,11 +2097,8 @@ export class Wallet implements IWallet {
20962097
error.code = 'recipients_not_allowed_for_fillnonce_and_acceleration_tx_type';
20972098
throw error;
20982099
}
2099-
const supportedTxRequestVersions = this.tssUtils?.supportedTxRequestVersions() || [];
2100-
if (params.apiVersion && !supportedTxRequestVersions.includes(params.apiVersion)) {
2101-
throw new Error(
2102-
`prebuildAndSignTransaction params.apiVersion=${params.apiVersion} must be one of ${supportedTxRequestVersions}`
2103-
);
2100+
if (params.apiVersion) {
2101+
validateTxRequestApiVersion(this, params.apiVersion);
21042102
}
21052103

21062104
// Doing a sanity check for password here to avoid doing further work if we know it's wrong
@@ -3084,19 +3082,7 @@ export class Wallet implements IWallet {
30843082
private async prebuildTransactionTss(params: PrebuildTransactionOptions = {}): Promise<PrebuildTransactionResult> {
30853083
const reqId = params.reqId || new RequestTracer();
30863084
this.bitgo.setRequestTracer(reqId);
3087-
3088-
if (
3089-
params.apiVersion === 'lite' &&
3090-
(this._wallet.type === 'custodial' || this._wallet.type === 'cold' || this.baseCoin.getMPCAlgorithm() === 'ecdsa')
3091-
) {
3092-
throw new Error(`Custodial and ECDSA MPC algorithm must always use 'full' api version`);
3093-
}
3094-
3095-
const apiVersion =
3096-
params.apiVersion ||
3097-
(this._wallet.type === 'custodial' || this._wallet.type === 'cold' || this.baseCoin.getMPCAlgorithm() === 'ecdsa'
3098-
? 'full'
3099-
: 'lite');
3085+
const apiVersion = getTxRequestApiVersion(this, params.apiVersion);
31003086
// Two options different implementations of fees seems to now be supported, for now we will support both to be backwards compatible
31013087
// TODO(BG-59685): deprecate one of these so that we have a single way to pass fees
31023088
let feeOptions;
@@ -3555,20 +3541,14 @@ export class Wallet implements IWallet {
35553541
* @param params send options
35563542
*/
35573543
private async sendManyTss(params: SendManyOptions = {}): Promise<any> {
3558-
const { apiVersion } = params;
3559-
const supportedTxRequestVersions = this.tssUtils?.supportedTxRequestVersions() ?? [];
3560-
const onlySupportsTxRequestFull =
3561-
supportedTxRequestVersions.length === 1 && supportedTxRequestVersions.includes('full');
3562-
if (apiVersion === 'lite' && onlySupportsTxRequestFull) {
3563-
throw new Error('TxRequest Lite API is not supported for this wallet');
3564-
}
3544+
params.apiVersion = getTxRequestApiVersion(this, params.apiVersion);
35653545

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

3571-
if (onlySupportsTxRequestFull || apiVersion === 'full') {
3551+
if (params.apiVersion === 'full') {
35723552
const latestTxRequest = await getTxRequest(this.bitgo, this.id(), signedTransaction.txRequestId, params.reqId);
35733553
const reqId = params.reqId || new RequestTracer();
35743554
this.bitgo.setRequestTracer(reqId);
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
import 'should';
2+
import { ApiVersion, IWallet } from '../../../../src';
3+
import { getTxRequestApiVersion } from '../../../../src/bitgo/utils/txRequest';
4+
5+
describe('txRequest utils', () => {
6+
describe('getTxRequestApiVersion', function () {
7+
const testCases = [
8+
{
9+
wallet: {
10+
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
11+
type: () => 'hot',
12+
multisigType: () => 'tss',
13+
} as any as IWallet,
14+
requestedApiVersion: 'lite',
15+
expectedApiVersion: '',
16+
expectedErrorMessage: 'For ECDSA tss wallets, parameter `apiVersion` must be `full`.',
17+
},
18+
{
19+
wallet: {
20+
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
21+
type: () => 'cold',
22+
multisigType: () => 'tss',
23+
} as any as IWallet,
24+
requestedApiVersion: 'lite',
25+
expectedApiVersion: '',
26+
expectedErrorMessage: 'For non self-custodial (hot) tss wallets, parameter `apiVersion` must be `full`.',
27+
},
28+
{
29+
wallet: {
30+
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
31+
type: () => 'hot',
32+
multisigType: () => 'tss',
33+
} as any as IWallet,
34+
requestedApiVersion: undefined,
35+
expectedApiVersion: 'lite',
36+
expectedErrorMessage: '',
37+
},
38+
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
39+
return {
40+
wallet: {
41+
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
42+
type: () => walletType,
43+
multisigType: () => 'tss',
44+
} as any as IWallet,
45+
requestedApiVersion: 'full',
46+
expectedApiVersion: 'full',
47+
expectedErrorMessage: '',
48+
shouldThrow: false,
49+
};
50+
}),
51+
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
52+
return {
53+
wallet: {
54+
baseCoin: { getMPCAlgorithm: () => 'ecdsa' },
55+
type: () => walletType,
56+
multisigType: () => 'tss',
57+
} as any as IWallet,
58+
requestedApiVersion: undefined,
59+
expectedApiVersion: 'full',
60+
expectedErrorMessage: '',
61+
shouldThrow: false,
62+
};
63+
}),
64+
...['hot', 'cold', 'custodial', 'backing'].map((walletType) => {
65+
return {
66+
wallet: {
67+
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
68+
type: () => walletType,
69+
multisigType: () => 'tss',
70+
} as any as IWallet,
71+
requestedApiVersion: 'full',
72+
expectedApiVersion: 'full',
73+
expectedErrorMessage: '',
74+
shouldThrow: false,
75+
};
76+
}),
77+
...['cold', 'custodial', 'backing'].map((walletType) => {
78+
return {
79+
wallet: {
80+
baseCoin: { getMPCAlgorithm: () => 'eddsa' },
81+
type: () => walletType,
82+
multisigType: () => 'tss',
83+
} as any as IWallet,
84+
requestedApiVersion: undefined,
85+
expectedApiVersion: 'full',
86+
expectedErrorMessage: '',
87+
shouldThrow: false,
88+
};
89+
}),
90+
];
91+
92+
testCases.forEach((testCase) => {
93+
if (testCase.expectedErrorMessage) {
94+
it(`should throw an error if requested apiVersion is ${
95+
testCase.requestedApiVersion
96+
} for wallet type ${testCase.wallet.type()} for a ${testCase.wallet.baseCoin.getMPCAlgorithm()} wallet`, () => {
97+
(() =>
98+
getTxRequestApiVersion(
99+
testCase.wallet,
100+
testCase.requestedApiVersion as ApiVersion | undefined
101+
)).should.throw(testCase.expectedErrorMessage);
102+
});
103+
} else {
104+
it(`should return ${testCase.expectedApiVersion} if requested apiVersion is ${
105+
testCase.requestedApiVersion
106+
} for wallet type ${testCase.wallet.type()} for a ${testCase.wallet.baseCoin.getMPCAlgorithm()} wallet`, () => {
107+
getTxRequestApiVersion(testCase.wallet, testCase.requestedApiVersion as ApiVersion | undefined).should.equal(
108+
testCase.expectedApiVersion
109+
);
110+
});
111+
}
112+
});
113+
});
114+
});

0 commit comments

Comments
 (0)