fix(abstract-utxo): handle explicit internal recipients correctly

Previously, we only considered the external outputs of the transaction prebuild
when calculating the output differences. When a user explicitly specified an
internal recipient, this triggered a missing output error.

We now compare the full output list of the prebuild with the expected outputs.

TICKET: BTC-1791

Author: OttoAllmendinger

modules/abstract-utxo/src/transaction/descriptor/parse.ts

@@ -40,9 +40,8 @@ function parseOutputsWithPsbt(
   recipientOutputs: RecipientOutput[]
 ): ParsedOutputs {
   const parsed = coreDescriptors.parse(psbt, descriptorMap, psbt.network);
-  const externalOutputs = parsed.outputs.filter((o) => o.scriptId === undefined);
   const changeOutputs = parsed.outputs.filter((o) => o.scriptId !== undefined);
-  const outputDiffs = outputDifferencesWithExpected(externalOutputs, recipientOutputs);
+  const outputDiffs = outputDifferencesWithExpected(parsed.outputs, recipientOutputs);
   return {
     outputs: parsed.outputs,
     changeOutputs,
@@ -72,9 +71,11 @@ function toBaseOutputs(
 export type ParsedOutputsBigInt = BaseParsedTransactionOutputs<bigint, BaseOutput<bigint | 'max'>>;
 
 function toBaseParsedTransactionOutputs(
-  { outputs, changeOutputs, explicitExternalOutputs, implicitExternalOutputs, missingOutputs }: ParsedOutputs,
+  { outputs, changeOutputs, explicitOutputs, implicitOutputs, missingOutputs }: ParsedOutputs,
   network: utxolib.Network
 ): ParsedOutputsBigInt {
+  const explicitExternalOutputs = explicitOutputs.filter((o) => o.scriptId === undefined);
+  const implicitExternalOutputs = implicitOutputs.filter((o) => o.scriptId === undefined);
   return {
     outputs: toBaseOutputs(outputs, network),
     changeOutputs: toBaseOutputs(changeOutputs, network),

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

@@ -42,33 +42,33 @@ export function outputDifference<A extends ActualOutput | ExpectedOutput, B exte
 
 export type OutputDifferenceWithExpected<TActual extends ActualOutput, TExpected extends ExpectedOutput> = {
   /** These are the external outputs that were expected and found in the transaction. */
-  explicitExternalOutputs: TActual[];
+  explicitOutputs: TActual[];
   /**
    * These are the surprise external outputs that were not explicitly specified in the transaction.
    * They can be PayGo fees.
    */
-  implicitExternalOutputs: TActual[];
+  implicitOutputs: TActual[];
   /**
    * These are the outputs that were expected to be in the transaction but were not found.
    */
   missingOutputs: TExpected[];
 };
 
 /**
- * @param actualExternalOutputs - external outputs in the transaction
- * @param expectedExternalOutputs - external outputs that were expected to be in the transaction
+ * @param actualOutputs - external outputs in the transaction
+ * @param expectedOutputs - external outputs that were expected to be in the transaction
  * @returns the difference between the actual and expected external outputs
  */
 export function outputDifferencesWithExpected<TActual extends ActualOutput, TExpected extends ExpectedOutput>(
-  actualExternalOutputs: TActual[],
-  expectedExternalOutputs: TExpected[]
+  actualOutputs: TActual[],
+  expectedOutputs: TExpected[]
 ): OutputDifferenceWithExpected<TActual, TExpected> {
-  const implicitExternalOutputs = outputDifference(actualExternalOutputs, expectedExternalOutputs);
-  const explicitExternalOutputs = outputDifference(actualExternalOutputs, implicitExternalOutputs);
-  const missingOutputs = outputDifference(expectedExternalOutputs, actualExternalOutputs);
+  const implicitOutputs = outputDifference(actualOutputs, expectedOutputs);
+  const explicitOutputs = outputDifference(actualOutputs, implicitOutputs);
+  const missingOutputs = outputDifference(expectedOutputs, actualOutputs);
   return {
-    explicitExternalOutputs,
-    implicitExternalOutputs,
+    explicitOutputs,
+    implicitOutputs,
     missingOutputs,
   };
 }

modules/abstract-utxo/test/transaction/descriptor/fixtures/parseWithInternalRecipient.json

@@ -28,11 +28,5 @@
     }
   ],
   "implicitExternalSpendAmount": "400000",
-  "missingOutputs": [
-    {
-      "address": "bc1q2yau645jl7k577lmanqn9a0ulcgcqm0wrrmx09dppd3kcwguvyzqk86umj",
-      "amount": "400000",
-      "external": true
-    }
-  ]
+  "missingOutputs": []
 }

modules/abstract-utxo/test/transaction/descriptor/outputDifference.ts

@@ -76,8 +76,8 @@ describe('outputDifference', function () {
     ) {
       const result = outputDifferencesWithExpected(outputs, recipients);
       assert.deepStrictEqual(result, {
-        explicitExternalOutputs: expected.explicit,
-        implicitExternalOutputs: expected.implicit,
+        explicitOutputs: expected.explicit,
+        implicitOutputs: expected.implicit,
         missingOutputs: expected.missing,
       });
     }

modules/abstract-utxo/test/transaction/descriptor/parse.ts

@@ -1,8 +1,11 @@
 import assert from 'assert';
 
-import { mockPsbtDefaultWithDescriptorTemplate } from '../../core/descriptor/psbt/mock.utils';
+import * as utxolib from '@bitgo/utxo-lib';
+import { Descriptor } from '@bitgo/wasm-miniscript';
+
+import { mockPsbtDefault } from '../../core/descriptor/psbt/mock.utils';
 import { ParsedOutputsBigInt, toBaseParsedTransactionOutputsFromPsbt } from '../../../src/transaction/descriptor/parse';
-import { getDefaultXPubs, getDescriptorMap } from '../../core/descriptor/descriptor.utils';
+import { getDefaultXPubs, getDescriptor, getDescriptorMap } from '../../core/descriptor/descriptor.utils';
 import { toPlainObject } from '../../core/toPlainObject.utils';
 import {
   AggregateValidationError,
@@ -12,6 +15,7 @@ import {
 } from '../../../src/transaction/descriptor/verifyTransaction';
 import { toAmountType } from '../../../src/transaction/descriptor/parseToAmountType';
 import { BaseOutput } from '../../../src';
+import { createAddressFromDescriptor } from '../../../src/core/descriptor';
 
 import { getFixtureRoot } from './fixtures.utils';
 
@@ -46,9 +50,23 @@ function toMaxOutput(output: OutputWithValue): OutputWithValue<'max'> {
 }
 
 describe('parse', function () {
-  const psbt = mockPsbtDefaultWithDescriptorTemplate('Wsh2Of3');
+  const descriptorSelf = getDescriptor('Wsh2Of3', getDefaultXPubs('a'));
+  const descriptorOther = getDescriptor('Wsh2Of3', getDefaultXPubs('b'));
+  const psbt = mockPsbtDefault({ descriptorSelf, descriptorOther });
+
+  function recipient(descriptor: Descriptor, index: number, value = 1000) {
+    return { value, address: createAddressFromDescriptor(descriptor, index, utxolib.networks.bitcoin) };
+  }
+
+  function internalRecipient(index: number, value?: number): OutputWithValue {
+    return recipient(descriptorSelf, index, value);
+  }
 
-  function getBaseParsedTransaction(recipients: OutputWithValue[]): ParsedOutputsBigInt {
+  function externalRecipient(index: number, value?: number): OutputWithValue {
+    return recipient(descriptorOther, index, value);
+  }
+
+  function getBaseParsedTransaction(psbt: utxolib.bitgo.UtxoPsbt, recipients: OutputWithValue[]): ParsedOutputsBigInt {
     return toBaseParsedTransactionOutputsFromPsbt(
       psbt,
       getDescriptorMap('Wsh2Of3', getDefaultXPubs('a')),
@@ -59,27 +77,30 @@ describe('parse', function () {
 
   describe('toBase', function () {
     it('should return the correct BaseParsedTransactionOutputs', async function () {
-      await assertEqualFixture('parseWithoutRecipients.json', toPlainObject(getBaseParsedTransaction([])));
+      await assertEqualFixture('parseWithoutRecipients.json', toPlainObject(getBaseParsedTransaction(psbt, [])));
       await assertEqualFixture(
         'parseWithExternalRecipient.json',
-        toPlainObject(getBaseParsedTransaction([psbt.txOutputs[0]]))
+        toPlainObject(getBaseParsedTransaction(psbt, [psbt.txOutputs[0]]))
       );
       await assertEqualFixture(
         'parseWithInternalRecipient.json',
-        toPlainObject(getBaseParsedTransaction([psbt.txOutputs[1]]))
+        toPlainObject(getBaseParsedTransaction(psbt, [psbt.txOutputs[1]]))
       );
       await assertEqualFixture(
         'parseWithExternalRecipient.json',
         // max recipient: ignore actual value
-        toPlainObject(getBaseParsedTransaction([toMaxOutput(psbt.txOutputs[0])]))
+        toPlainObject(getBaseParsedTransaction(psbt, [toMaxOutput(psbt.txOutputs[0])]))
       );
     });
 
     function assertEqualValidationError(actual: unknown, expected: AggregateValidationError) {
+      function normErrors(e: Error[]): Error[] {
+        return e.map((e) => ({ ...e, stack: undefined }));
+      }
       if (actual instanceof AggregateValidationError) {
-        assert.deepStrictEqual(actual.errors, expected.errors);
+        assert.deepStrictEqual(normErrors(actual.errors), normErrors(expected.errors));
       } else {
-        throw new Error('unexpected error type');
+        throw new Error('unexpected error type: ' + actual);
       }
     }
 
@@ -90,35 +111,48 @@ describe('parse', function () {
       });
     }
 
-    it('should throw expected errors', function () {
+    function implicitOutputError(output: OutputWithValue, { external = true } = {}): ErrorImplicitExternalOutputs {
+      return new ErrorImplicitExternalOutputs([{ ...toBaseOutputBigInt(output), external }]);
+    }
+
+    function missingOutputError(output: OutputWithValue, { external = true } = {}): ErrorMissingOutputs {
+      return new ErrorMissingOutputs([{ ...toBaseOutputBigInt(output), external }]);
+    }
+
+    it('should throw expected error: no recipient requested', function () {
       assertValidationError(
-        () => assertExpectedOutputDifference(getBaseParsedTransaction([])),
-        new AggregateValidationError([
-          new ErrorImplicitExternalOutputs([{ ...toBaseOutputBigInt(psbt.txOutputs[0]), external: true }]),
-        ])
+        () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [])),
+        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
       );
+    });
 
+    it('should throw expected error: only internal recipient requested', function () {
       assertValidationError(
-        () => assertExpectedOutputDifference(getBaseParsedTransaction([])),
-        new AggregateValidationError([
-          new ErrorImplicitExternalOutputs([{ ...toBaseOutputBigInt(psbt.txOutputs[0]), external: true }]),
-        ])
+        () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [psbt.txOutputs[1]])),
+        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
       );
+    });
 
+    it('should throw expected error: only internal max recipient requested', function () {
       assertValidationError(
-        () => assertExpectedOutputDifference(getBaseParsedTransaction([psbt.txOutputs[1]])),
-        new AggregateValidationError([
-          new ErrorMissingOutputs([{ ...toBaseOutputBigInt(psbt.txOutputs[1]), external: true }]),
-          new ErrorImplicitExternalOutputs([{ ...toBaseOutputBigInt(psbt.txOutputs[0]), external: true }]),
-        ])
+        () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [toMaxOutput(psbt.txOutputs[1])])),
+        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
       );
+    });
+
+    it('should throw expected error: swapped recipient', function () {
+      const recipient = externalRecipient(99);
+      assertValidationError(
+        () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [recipient])),
+        new AggregateValidationError([missingOutputError(recipient), implicitOutputError(psbt.txOutputs[0])])
+      );
+    });
 
+    it('should throw expected error: missing internal recipient', function () {
+      const recipient = internalRecipient(99);
       assertValidationError(
-        () => assertExpectedOutputDifference(getBaseParsedTransaction([toMaxOutput(psbt.txOutputs[1])])),
-        new AggregateValidationError([
-          new ErrorMissingOutputs([{ ...toBaseOutputBigInt(toMaxOutput(psbt.txOutputs[1])), external: true }]),
-          new ErrorImplicitExternalOutputs([{ ...toBaseOutputBigInt(psbt.txOutputs[0]), external: true }]),
-        ])
+        () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [recipient])),
+        new AggregateValidationError([missingOutputError(recipient), implicitOutputError(psbt.txOutputs[0])])
       );
     });
   });