Merge pull request #7558 from BitGo/BTC-2732.parseTxFixes

feat(abstract-utxo): improve transaction parsing and handling

Author: OttoAllmendinger

modules/abstract-utxo/src/abstractUtxoCoin.ts

@@ -561,12 +561,13 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
 
   toCanonicalTransactionRecipient(output: { valueString: string; address?: string }): {
     amount: bigint;
-    address?: string;
+    address: string;
   } {
     const amount = BigInt(output.valueString);
     assertValidTransactionRecipient({ amount, address: output.address });
-    if (!output.address) {
-      return { amount };
+    assert(output.address, 'address is required');
+    if (isScriptRecipient(output.address)) {
+      return { amount, address: output.address };
     }
     return { amount, address: this.canonicalAddress(output.address) };
   }

modules/abstract-utxo/src/transaction/fixedScript/parseTransaction.ts

@@ -1,7 +1,7 @@
 import assert from 'assert';
 
 import _ from 'lodash';
-import { Triple, VerificationOptions, Wallet } from '@bitgo/sdk-core';
+import { ITransactionRecipient, Triple, VerificationOptions, Wallet } from '@bitgo/sdk-core';
 import * as utxolib from '@bitgo/utxo-lib';
 
 import type { AbstractUtxoCoin, ParseTransactionOptions } from '../../abstractUtxoCoin';
@@ -17,12 +17,74 @@ export type ComparableOutputWithExternal<TValue> = ComparableOutput<TValue> & {
   external: boolean | undefined;
 };
 
+async function parseRbfTransaction<TNumber extends bigint | number>(
+  coin: AbstractUtxoCoin,
+  params: ParseTransactionOptions<TNumber>
+): Promise<ParsedTransaction<TNumber>> {
+  const { txParams, wallet } = params;
+
+  assert(txParams.rbfTxIds);
+  assert(txParams.rbfTxIds.length === 1);
+
+  const txToBeReplaced = await wallet.getTransaction({ txHash: txParams.rbfTxIds[0], includeRbf: true });
+  const recipients = txToBeReplaced.outputs.flatMap(
+    (output: { valueString: string; address?: string; wallet?: string }) => {
+      // For self-sends, the walletId will be the same as the wallet's id
+      if (output.wallet === wallet.id()) {
+        return [];
+      }
+      return [coin.toCanonicalTransactionRecipient(output)];
+    }
+  );
+
+  // Recurse into parseTransaction with the derived recipients and without rbfTxIds
+  return parseTransaction(coin, {
+    ...params,
+    txParams: {
+      ...txParams,
+      recipients,
+      rbfTxIds: undefined,
+    },
+  });
+}
+
+function toExpectedOutputs(
+  coin: AbstractUtxoCoin,
+  txParams: {
+    recipients?: ITransactionRecipient[];
+    allowExternalChangeAddress?: boolean;
+    changeAddress?: string;
+  }
+): Output[] {
+  // verify that each recipient from txParams has their own output
+  const expectedOutputs = (txParams.recipients ?? []).flatMap((output) => {
+    if (output.address === undefined) {
+      if (output.amount.toString() !== '0') {
+        throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${output}`);
+      }
+      return [output];
+    }
+    return [{ ...output, address: coin.canonicalAddress(output.address) }];
+  });
+  if (txParams.allowExternalChangeAddress && txParams.changeAddress) {
+    // when an external change address is explicitly specified, count all outputs going towards that
+    // address in the expected outputs (regardless of the output amount)
+    expectedOutputs.push({ address: coin.canonicalAddress(txParams.changeAddress), amount: 'max' });
+  }
+  return expectedOutputs;
+}
+
 export async function parseTransaction<TNumber extends bigint | number>(
   coin: AbstractUtxoCoin,
   params: ParseTransactionOptions<TNumber>
 ): Promise<ParsedTransaction<TNumber>> {
   const { txParams, txPrebuild, wallet, verification = {}, reqId } = params;
 
+  // Branch off early for RBF transactions
+  if (txParams.rbfTxIds) {
+    return parseRbfTransaction(coin, params);
+  }
+
   if (!_.isUndefined(verification.disableNetworking) && !_.isBoolean(verification.disableNetworking)) {
     throw new Error('verification.disableNetworking must be a boolean');
   }
@@ -47,56 +109,7 @@ export async function parseTransaction<TNumber extends bigint | number>(
     throw new Error('missing required txPrebuild property txHex');
   }
 
-  // obtain all outputs
-  const explanation: TransactionExplanation = await coin.explainTransaction<TNumber>({
-    txHex: txPrebuild.txHex,
-    txInfo: txPrebuild.txInfo,
-    pubs: keychainArray.map((k) => k.pub) as Triple<string>,
-  });
-
-  const allOutputs = [...explanation.outputs, ...explanation.changeOutputs];
-
-  let expectedOutputs;
-  if (txParams.rbfTxIds) {
-    assert(txParams.rbfTxIds.length === 1);
-
-    const txToBeReplaced = await wallet.getTransaction({ txHash: txParams.rbfTxIds[0], includeRbf: true });
-    expectedOutputs = txToBeReplaced.outputs.flatMap(
-      (output: { valueString: string; address?: string; wallet?: string }) => {
-        // For self-sends, the walletId will be the same as the wallet's id
-        if (output.wallet === wallet.id()) {
-          return [];
-        }
-        return [coin.toCanonicalTransactionRecipient(output)];
-      }
-    );
-  } else {
-    // verify that each recipient from txParams has their own output
-    expectedOutputs = (txParams.recipients ?? []).flatMap((output) => {
-      if (output.address === undefined) {
-        if (output.amount.toString() !== '0') {
-          throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${output}`);
-        }
-        return [output];
-      }
-      return [{ ...output, address: coin.canonicalAddress(output.address) }];
-    });
-    if (txParams.allowExternalChangeAddress && txParams.changeAddress) {
-      // when an external change address is explicitly specified, count all outputs going towards that
-      // address in the expected outputs (regardless of the output amount)
-      expectedOutputs.push(
-        ...allOutputs.flatMap((output) => {
-          if (
-            output.address === undefined ||
-            output.address !== coin.canonicalAddress(txParams.changeAddress as string)
-          ) {
-            return [];
-          }
-          return [{ ...output, address: coin.canonicalAddress(output.address) }];
-        })
-      );
-    }
-  }
+  const expectedOutputs = toExpectedOutputs(coin, txParams);
 
   // get the keychains from the custom change wallet if needed
   let customChange: CustomChangeOptions | undefined;
@@ -126,6 +139,15 @@ export async function parseTransaction<TNumber extends bigint | number>(
     }
   }
 
+  // obtain all outputs
+  const explanation: TransactionExplanation = await coin.explainTransaction<TNumber>({
+    txHex: txPrebuild.txHex,
+    txInfo: txPrebuild.txInfo,
+    pubs: keychainArray.map((k) => k.pub) as Triple<string>,
+  });
+
+  const allOutputs = [...explanation.outputs, ...explanation.changeOutputs];
+
   /**
    * Loop through all the outputs and classify each of them as either internal spends
    * or external spends by setting the "external" property to true or false on the output object.

modules/abstract-utxo/src/transaction/recipient.ts

@@ -47,7 +47,9 @@ export function assertValidTransactionRecipient(output: { amount: bigint | numbe
   // We will verify that the amount is zero, and if it isnt then we will throw an error.
   if (!output.address || isScriptRecipient(output.address)) {
     if (output.amount.toString() !== '0') {
-      throw new Error(`Only zero amounts allowed for non-encodeable scriptPubkeys: ${JSON.stringify(output)}`);
+      throw new Error(
+        `Only zero amounts allowed for non-encodeable scriptPubkeys: amount: ${output.amount}, address: ${output.address}`
+      );
     }
   }
 }

modules/abstract-utxo/test/unit/transaction/fixedScript/explainPsbt.ts

@@ -7,14 +7,7 @@ import { fixedScriptWallet, Triple } from '@bitgo/wasm-utxo';
 import type { TransactionExplanation } from '../../../../src/transaction/fixedScript/explainTransaction';
 import { explainPsbt, explainPsbtWasm } from '../../../../src/transaction/fixedScript';
 
-function hasWasmUtxoSupport(network: utxolib.Network): boolean {
-  return ![
-    utxolib.networks.bitcoincash,
-    utxolib.networks.bitcoingold,
-    utxolib.networks.ecash,
-    utxolib.networks.zcash,
-  ].includes(utxolib.getMainnet(network));
-}
+import { hasWasmUtxoSupport } from './util';
 
 function describeTransactionWith(acidTest: testutil.AcidTest) {
   describe(`${acidTest.name}`, function () {
@@ -32,7 +25,7 @@ function describeTransactionWith(acidTest: testutil.AcidTest) {
       // note: `outputs` means external outputs here
       assert.strictEqual(refExplanation.outputs.length, 3);
       assert.strictEqual(refExplanation.changeOutputs.length, acidTest.outputs.length - 3);
-      assert.strictEqual(refExplanation.outputAmount, '2700');
+      assert.strictEqual(refExplanation.outputAmount, '1800');
       assert.strictEqual(refExplanation.changeOutputs.length, acidTest.outputs.length - 3);
       refExplanation.changeOutputs.forEach((change) => {
         assert.strictEqual(change.amount, '900');