diff --git a/modules/abstract-utxo/src/keychains.ts b/modules/abstract-utxo/src/keychains.ts index e0e945063e..0fe91b9f01 100644 --- a/modules/abstract-utxo/src/keychains.ts +++ b/modules/abstract-utxo/src/keychains.ts @@ -1,6 +1,7 @@ import assert from 'assert'; import * as t from 'io-ts'; +import { bitgo } from '@bitgo/utxo-lib'; import { BIP32Interface, bip32 } from '@bitgo/secp256k1'; import { IRequestTracer, IWallet, KeyIndices, promiseProps, Triple } from '@bitgo/sdk-core'; @@ -47,9 +48,15 @@ export function toKeychainTriple(keychains: UtxoNamedKeychains): Triple | Triple + keychains: bitgo.RootWalletKeys | UtxoNamedKeychains | Triple<{ pub: string }> | string[] ): Triple { + if (keychains instanceof bitgo.RootWalletKeys) { + return keychains.triple; + } if (Array.isArray(keychains)) { + if (keychains.length !== 3) { + throw new Error('expected 3 keychains'); + } return keychains.map((keychain: { pub: string } | string) => { const v = typeof keychain === 'string' ? keychain : keychain.pub; return bip32.fromBase58(v); @@ -59,6 +66,34 @@ export function toBip32Triple( return toBip32Triple(toKeychainTriple(keychains)); } +function toXpub(keychain: { pub: string } | string | BIP32Interface): string { + if (typeof keychain === 'string') { + if (keychain.startsWith('xpub')) { + return keychain; + } + throw new Error('expected xpub'); + } + if ('neutered' in keychain) { + return keychain.neutered().toBase58(); + } + if ('pub' in keychain) { + return toXpub(keychain.pub); + } + throw new Error('expected keychain'); +} + +export function toXpubTriple( + keychains: UtxoNamedKeychains | Triple<{ pub: string }> | Triple | Triple +): Triple { + if (Array.isArray(keychains)) { + if (keychains.length !== 3) { + throw new Error('expected 3 keychains'); + } + return keychains.map((k) => toXpub(k)) as Triple; + } + return toXpubTriple(toKeychainTriple(keychains)); +} + export async function fetchKeychains( coin: AbstractUtxoCoin, wallet: IWallet, diff --git a/modules/abstract-utxo/src/transaction/descriptor/explainPsbt.ts b/modules/abstract-utxo/src/transaction/descriptor/explainPsbt.ts index 1ad3537058..1976cc0d5d 100644 --- a/modules/abstract-utxo/src/transaction/descriptor/explainPsbt.ts +++ b/modules/abstract-utxo/src/transaction/descriptor/explainPsbt.ts @@ -3,7 +3,7 @@ import { ITransactionRecipient } from '@bitgo/sdk-core'; import * as coreDescriptors from '@bitgo/utxo-core/descriptor'; import { toExtendedAddressFormat } from '../recipient'; -import type { TransactionExplanationUtxolibPsbt } from '../fixedScript/explainTransaction'; +import type { TransactionExplanationDescriptor } from '../fixedScript/explainTransaction'; function toRecipient(output: coreDescriptors.ParsedOutput, network: utxolib.Network): ITransactionRecipient { return { @@ -34,7 +34,7 @@ function getInputSignatures(psbt: utxolib.bitgo.UtxoPsbt): number[] { export function explainPsbt( psbt: utxolib.bitgo.UtxoPsbt, descriptors: coreDescriptors.DescriptorMap -): TransactionExplanationUtxolibPsbt { +): TransactionExplanationDescriptor { const parsedTransaction = coreDescriptors.parse(psbt, descriptors, psbt.network); const { inputs, outputs } = parsedTransaction; const externalOutputs = outputs.filter((o) => o.scriptId === undefined); diff --git a/modules/abstract-utxo/src/transaction/fixedScript/explainPsbtWasm.ts b/modules/abstract-utxo/src/transaction/fixedScript/explainPsbtWasm.ts index 7789c5b519..0f884a3fd5 100644 --- a/modules/abstract-utxo/src/transaction/fixedScript/explainPsbtWasm.ts +++ b/modules/abstract-utxo/src/transaction/fixedScript/explainPsbtWasm.ts @@ -9,6 +9,35 @@ function scriptToAddress(script: Uint8Array): string { return `scriptPubKey:${Buffer.from(script).toString('hex')}`; } +type ParsedWalletOutput = fixedScriptWallet.ParsedOutput & { scriptId: fixedScriptWallet.ScriptId }; +type ParsedExternalOutput = fixedScriptWallet.ParsedOutput & { scriptId: null }; + +function isParsedWalletOutput(output: ParsedWalletOutput | ParsedExternalOutput): output is ParsedWalletOutput { + return output.scriptId !== null; +} + +function isParsedExternalOutput(output: ParsedWalletOutput | ParsedExternalOutput): output is ParsedExternalOutput { + return output.scriptId === null; +} + +function toChangeOutput(output: ParsedWalletOutput): FixedScriptWalletOutput { + return { + address: output.address ?? scriptToAddress(output.script), + amount: output.value.toString(), + chain: output.scriptId.chain, + index: output.scriptId.index, + external: false, + }; +} + +function toExternalOutput(output: ParsedExternalOutput): Output { + return { + address: output.address ?? scriptToAddress(output.script), + amount: output.value.toString(), + external: true, + }; +} + export function explainPsbtWasm( psbt: fixedScriptWallet.BitGoPsbt, walletXpubs: Triple, @@ -17,44 +46,45 @@ export function explainPsbtWasm( checkSignature?: boolean; outputScripts: Buffer[]; }; + customChangeWalletXpubs?: Triple; } ): TransactionExplanationWasm { const parsed = psbt.parseTransactionWithWalletKeys(walletXpubs, params.replayProtection); const changeOutputs: FixedScriptWalletOutput[] = []; const outputs: Output[] = []; + const parsedCustomChangeOutputs = params.customChangeWalletXpubs + ? psbt.parseOutputsWithWalletKeys(params.customChangeWalletXpubs) + : undefined; - parsed.outputs.forEach((output) => { - const address = output.address ?? scriptToAddress(output.script); + const customChangeOutputs: FixedScriptWalletOutput[] = []; - if (output.scriptId) { + parsed.outputs.forEach((output, i) => { + const parseCustomChangeOutput = parsedCustomChangeOutputs?.[i]; + if (isParsedWalletOutput(output)) { // This is a change output - changeOutputs.push({ - address, - amount: output.value.toString(), - chain: output.scriptId.chain, - index: output.scriptId.index, - external: false, - }); + changeOutputs.push(toChangeOutput(output)); + } else if (parseCustomChangeOutput && isParsedWalletOutput(parseCustomChangeOutput)) { + customChangeOutputs.push(toChangeOutput(parseCustomChangeOutput)); + } else if (isParsedExternalOutput(output)) { + outputs.push(toExternalOutput(output)); } else { - // This is an external output - outputs.push({ - address, - amount: output.value.toString(), - external: true, - }); + throw new Error('Invalid output'); } }); - const changeAmount = changeOutputs.reduce((sum, output) => sum + BigInt(output.amount), BigInt(0)); const outputAmount = outputs.reduce((sum, output) => sum + BigInt(output.amount), BigInt(0)); + const changeAmount = changeOutputs.reduce((sum, output) => sum + BigInt(output.amount), BigInt(0)); + const customChangeAmount = customChangeOutputs.reduce((sum, output) => sum + BigInt(output.amount), BigInt(0)); return { id: psbt.unsignedTxid(), outputAmount: outputAmount.toString(), changeAmount: changeAmount.toString(), + customChangeAmount: customChangeAmount.toString(), outputs, changeOutputs, + customChangeOutputs, fee: parsed.minerFee.toString(), }; } diff --git a/modules/abstract-utxo/src/transaction/fixedScript/explainTransaction.ts b/modules/abstract-utxo/src/transaction/fixedScript/explainTransaction.ts index 9d11fd14e8..00b98f5361 100644 --- a/modules/abstract-utxo/src/transaction/fixedScript/explainTransaction.ts +++ b/modules/abstract-utxo/src/transaction/fixedScript/explainTransaction.ts @@ -9,13 +9,17 @@ import type { Bip322Message } from '../../abstractUtxoCoin'; import type { Output, FixedScriptWalletOutput } from '../types'; import { toExtendedAddressFormat } from '../recipient'; import { getPayGoVerificationPubkey } from '../getPayGoVerificationPubkey'; +import { toBip32Triple } from '../../keychains'; // ===== Transaction Explanation Type Definitions ===== -export interface AbstractUtxoTransactionExplanation extends BaseTransactionExplanation { +export interface AbstractUtxoTransactionExplanation + extends BaseTransactionExplanation { /** NOTE: this actually only captures external outputs */ outputs: Output[]; - changeOutputs: Output[]; + changeOutputs: TChangeOutput[]; + customChangeOutputs?: TChangeOutput[]; + customChangeAmount?: string; /** * BIP322 messages extracted from the transaction inputs. @@ -25,7 +29,8 @@ export interface AbstractUtxoTransactionExplanation extends BaseT } /** @deprecated - the signature fields are not very useful */ -interface TransactionExplanationWithSignatures extends AbstractUtxoTransactionExplanation { +interface TransactionExplanationWithSignatures + extends AbstractUtxoTransactionExplanation { /** @deprecated - unused outside of tests */ locktime?: number; @@ -43,7 +48,7 @@ interface TransactionExplanationWithSignatures extends AbstractUt } /** For our wasm backend, we do not return the deprecated fields. We set TFee to string for backwards compatibility. */ -export type TransactionExplanationWasm = AbstractUtxoTransactionExplanation; +export type TransactionExplanationWasm = AbstractUtxoTransactionExplanation; /** When parsing the legacy transaction format, we cannot always infer the fee so we set it to string | undefined */ export type TransactionExplanationUtxolibLegacy = TransactionExplanationWithSignatures; @@ -51,6 +56,8 @@ export type TransactionExplanationUtxolibLegacy = TransactionExplanationWithSign /** When parsing a PSBT, we can infer the fee so we set TFee to string. */ export type TransactionExplanationUtxolibPsbt = TransactionExplanationWithSignatures; +export type TransactionExplanationDescriptor = TransactionExplanationWithSignatures; + export type TransactionExplanation = | TransactionExplanationUtxolibLegacy | TransactionExplanationUtxolibPsbt @@ -62,22 +69,47 @@ export type ChangeAddressInfo = { index: number; }; +function toChangeOutput( + txOutput: utxolib.TxOutput, + network: utxolib.Network, + changeInfo: ChangeAddressInfo[] | undefined +): FixedScriptWalletOutput | undefined { + if (!changeInfo) { + return undefined; + } + const address = toExtendedAddressFormat(txOutput.script, network); + const change = changeInfo.find((change) => change.address === address); + if (!change) { + return undefined; + } + return { + address, + amount: txOutput.value.toString(), + chain: change.chain, + index: change.index, + external: false, + }; +} + +function outputSum(outputs: { amount: string | number }[]): bigint { + return outputs.reduce((sum, output) => sum + BigInt(output.amount), BigInt(0)); +} + function explainCommon( tx: bitgo.UtxoTransaction, params: { changeInfo?: ChangeAddressInfo[]; + customChangeInfo?: ChangeAddressInfo[]; feeInfo?: string; }, network: utxolib.Network ) { const displayOrder = ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs']; - let spendAmount = BigInt(0); - let changeAmount = BigInt(0); const changeOutputs: FixedScriptWalletOutput[] = []; - const outputs: Output[] = []; + const customChangeOutputs: FixedScriptWalletOutput[] = []; + const externalOutputs: Output[] = []; - const { changeInfo } = params; - const changeAddresses = changeInfo?.map((info) => info.address) ?? []; + const { changeInfo, customChangeInfo } = params; tx.outs.forEach((currentOutput) => { // Try to encode the script pubkey with an address. If it fails, try to parse it as an OP_RETURN output with the prefix. @@ -85,26 +117,19 @@ function explainCommon( const currentAddress = toExtendedAddressFormat(currentOutput.script, network); const currentAmount = BigInt(currentOutput.value); - if (changeAddresses.includes(currentAddress)) { - // this is change - changeAmount += currentAmount; - const change = changeInfo?.find((change) => change.address === currentAddress); + const changeOutput = toChangeOutput(currentOutput, network, changeInfo); + if (changeOutput) { + changeOutputs.push(changeOutput); + return; + } - if (!change) { - throw new Error('changeInfo must have change information for all change outputs'); - } - changeOutputs.push({ - address: currentAddress, - amount: currentAmount.toString(), - chain: change.chain, - index: change.index, - external: false, - }); + const customChangeOutput = toChangeOutput(currentOutput, network, customChangeInfo); + if (customChangeOutput) { + customChangeOutputs.push(customChangeOutput); return; } - spendAmount += currentAmount; - outputs.push({ + externalOutputs.push({ address: currentAddress, amount: currentAmount.toString(), // If changeInfo has a length greater than or equal to zero, it means that the change information @@ -117,10 +142,14 @@ function explainCommon( }); const outputDetails = { - outputAmount: spendAmount.toString(), - changeAmount: changeAmount.toString(), - outputs, + outputs: externalOutputs, + outputAmount: outputSum(externalOutputs).toString(), + changeOutputs, + changeAmount: outputSum(changeOutputs).toString(), + + customChangeAmount: outputSum(customChangeOutputs).toString(), + customChangeOutputs, }; let fee: string | undefined; @@ -215,25 +244,27 @@ function getChainAndIndexFromBip32Derivations(output: bitgo.PsbtOutput) { return utxolib.bitgo.getChainAndIndexFromPath(paths[0]); } -function getChangeInfo(psbt: bitgo.UtxoPsbt): ChangeAddressInfo[] | undefined { +function getChangeInfo(psbt: bitgo.UtxoPsbt, walletKeys?: Triple): ChangeAddressInfo[] | undefined { try { - return utxolib.bitgo.findInternalOutputIndices(psbt).map((i) => { - const derivationInformation = getChainAndIndexFromBip32Derivations(psbt.data.outputs[i]); - if (!derivationInformation) { - throw new Error('could not find derivation information on bip32Derivation or tapBip32Derivation'); - } - return { - address: utxolib.address.fromOutputScript(psbt.txOutputs[i].script, psbt.network), - external: false, - ...derivationInformation, - }; - }); + walletKeys = walletKeys ?? utxolib.bitgo.getSortedRootNodes(psbt); } catch (e) { if (e instanceof utxolib.bitgo.ErrorNoMultiSigInputFound) { return undefined; } throw e; } + + return utxolib.bitgo.findWalletOutputIndices(psbt, walletKeys).map((i) => { + const derivationInformation = getChainAndIndexFromBip32Derivations(psbt.data.outputs[i]); + if (!derivationInformation) { + throw new Error('could not find derivation information on bip32Derivation or tapBip32Derivation'); + } + return { + address: utxolib.address.fromOutputScript(psbt.txOutputs[i].script, psbt.network), + external: false, + ...derivationInformation, + }; + }); } /** @@ -341,11 +372,17 @@ function getBip322MessageInfoAndVerify(psbt: bitgo.UtxoPsbt, network: utxolib.Ne /** * Decompose a raw psbt into useful information, such as the total amounts, * change amounts, and transaction outputs. + * + * @param psbt {bitgo.UtxoPsbt} The PSBT to explain + * @param pubs {bitgo.RootWalletKeys | string[]} The public keys to use for the explanation + * @param network {utxolib.Network} The network to use for the explanation + * @param strict {boolean} Whether to throw an error if the PayGo address proof is invalid */ export function explainPsbt( psbt: bitgo.UtxoPsbt, params: { pubs?: bitgo.RootWalletKeys | string[]; + customChangePubs?: bitgo.RootWalletKeys | string[]; }, network: utxolib.Network, { strict = false }: { strict?: boolean } = {} @@ -368,8 +405,11 @@ export function explainPsbt( const messages = getBip322MessageInfoAndVerify(psbt, network); const changeInfo = getChangeInfo(psbt); + const customChangeInfo = params.customChangePubs + ? getChangeInfo(psbt, toBip32Triple(params.customChangePubs)) + : undefined; const tx = psbt.getUnsignedTx(); - const common = explainCommon(tx, { ...params, changeInfo }, network); + const common = explainCommon(tx, { ...params, changeInfo, customChangeInfo }, network); const inputSignaturesCount = getPsbtInputSignaturesCount(psbt, params); // Set fee from subtracting inputs from outputs diff --git a/modules/abstract-utxo/src/transaction/fixedScript/parseOutput.ts b/modules/abstract-utxo/src/transaction/fixedScript/parseOutput.ts index 232b24a411..aabcdf691d 100644 --- a/modules/abstract-utxo/src/transaction/fixedScript/parseOutput.ts +++ b/modules/abstract-utxo/src/transaction/fixedScript/parseOutput.ts @@ -268,11 +268,14 @@ export async function parseOutput({ return currentOutput; } /** - * The only way to determine whether an address is known on the wallet is to initiate a network request and - * fetch it. Should the request fail and return a 404, it will throw and therefore has to be caught. For that - * reason, address wallet ownership detection is wrapped in a try/catch. Additionally, once the address - * details are fetched on the wallet, a local address validation is run, whose errors however are generated - * client-side and can therefore be analyzed with more granularity and type checking. + * For transaction with the legacy transaction format, the only way to + * determine whether an address is known on the wallet is to initiate a + * network request and fetch it. Should the request fail and return a 404, + * it will throw and therefore has to be caught. For that reason, address + * wallet ownership detection is wrapped in a try/catch. Additionally, once + * the address details are fetched on the wallet, a local address validation + * is run, whose errors however are generated client-side and can therefore + * be analyzed with more granularity and type checking. */ /** diff --git a/modules/abstract-utxo/test/unit/transaction/fixedScript/explainPsbt.ts b/modules/abstract-utxo/test/unit/transaction/fixedScript/explainPsbt.ts index 5bc6a3eaf4..1ba43bfdbf 100644 --- a/modules/abstract-utxo/test/unit/transaction/fixedScript/explainPsbt.ts +++ b/modules/abstract-utxo/test/unit/transaction/fixedScript/explainPsbt.ts @@ -18,14 +18,25 @@ function hasWasmUtxoSupport(network: utxolib.Network): boolean { function describeTransactionWith(acidTest: testutil.AcidTest) { describe(`${acidTest.name}`, function () { + let psbt: utxolib.bitgo.UtxoPsbt; let psbtBytes: Buffer; + let walletXpubs: Triple; + let customChangeWalletXpubs: Triple | undefined; + let wasmPsbt: fixedScriptWallet.BitGoPsbt; let refExplanation: TransactionExplanation; before('prepare', function () { - const psbt = acidTest.createPsbt(); + psbt = acidTest.createPsbt(); refExplanation = explainPsbt(psbt, { pubs: acidTest.rootWalletKeys }, acidTest.network, { strict: true, }); psbtBytes = psbt.toBuffer(); + const networkName = utxolib.getNetworkName(acidTest.network); + assert(networkName); + walletXpubs = acidTest.rootWalletKeys.triple.map((k) => k.neutered().toBase58()) as Triple; + customChangeWalletXpubs = acidTest.otherWalletKeys.triple.map((k) => k.neutered().toBase58()) as Triple; + if (hasWasmUtxoSupport(acidTest.network)) { + wasmPsbt = fixedScriptWallet.BitGoPsbt.fromBytes(psbtBytes, networkName); + } }); it('should match the expected values for explainPsbt', function () { @@ -40,15 +51,25 @@ function describeTransactionWith(acidTest: testutil.AcidTest) { }); }); + it('reference implementation should support custom change outputs', function () { + const customChangeExplanation = explainPsbt( + psbt, + { pubs: acidTest.rootWalletKeys, customChangePubs: acidTest.otherWalletKeys }, + acidTest.network, + { strict: true } + ); + assert.ok(customChangeExplanation.customChangeOutputs); + assert.strictEqual(customChangeExplanation.changeOutputs.length, refExplanation.changeOutputs.length); + assert.strictEqual(customChangeExplanation.outputs.length, refExplanation.outputs.length - 1); + assert.strictEqual(customChangeExplanation.customChangeOutputs.length, 1); + assert.strictEqual(customChangeExplanation.customChangeOutputs[0].amount, '900'); + }); + it('should match explainPsbtWasm', function () { if (!hasWasmUtxoSupport(acidTest.network)) { return this.skip(); } - const networkName = utxolib.getNetworkName(acidTest.network); - assert(networkName); - const wasmPsbt = fixedScriptWallet.BitGoPsbt.fromBytes(psbtBytes, networkName); - const walletXpubs = acidTest.rootWalletKeys.triple.map((k) => k.neutered().toBase58()) as Triple; const wasmExplanation = explainPsbtWasm(wasmPsbt, walletXpubs, { replayProtection: { outputScripts: [acidTest.getReplayProtectionOutputScript()], @@ -71,6 +92,25 @@ function describeTransactionWith(acidTest: testutil.AcidTest) { } } }); + + if (acidTest.network !== utxolib.networks.bitcoin) { + return; + } + + // extended test suite for bitcoin + + it('returns custom change outputs when parameter is set', function () { + const wasmExplanation = explainPsbtWasm(wasmPsbt, walletXpubs, { + replayProtection: { + outputScripts: [acidTest.getReplayProtectionOutputScript()], + }, + customChangeWalletXpubs, + }); + assert.ok(wasmExplanation.customChangeOutputs); + assert.deepStrictEqual(wasmExplanation.outputs.length, 2); + assert.deepStrictEqual(wasmExplanation.customChangeOutputs.length, 1); + assert.deepStrictEqual(wasmExplanation.customChangeOutputs[0].amount, '900'); + }); }); }