feat: improve transaction verification error handling

Replace custom validation errors with SDK's TxIntentMismatchRecipientError to
provide more detailed information about mismatched transaction outputs. This
enhances error reporting by including specific details about missing or
unexpected outputs, helping users understand what differs between their intent
and the actual transaction.

Co-authored-by: llm-git <llm-git@ttll.de>

Ticket: BTC-2579

TICKET: WP-6189

Author: lcovar

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

@@ -1,50 +1,54 @@
 import * as utxolib from '@bitgo/utxo-lib';
-import { ITransactionRecipient, TxIntentMismatchError, VerifyTransactionOptions } from '@bitgo/sdk-core';
+import {
+  ITransactionRecipient,
+  MismatchedRecipient,
+  TxIntentMismatchError,
+  TxIntentMismatchRecipientError,
+  VerifyTransactionOptions,
+} from '@bitgo/sdk-core';
 import { DescriptorMap } from '@bitgo/utxo-core/descriptor';
 
 import { AbstractUtxoCoin, BaseOutput, BaseParsedTransactionOutputs } from '../../abstractUtxoCoin';
 
 import { toBaseParsedTransactionOutputsFromPsbt } from './parse';
 
-export class ValidationError extends Error {
-  constructor(message: string) {
-    super(message);
-  }
-}
-
-export class ErrorMissingOutputs extends ValidationError {
-  constructor(public missingOutputs: BaseOutput<bigint | 'max'>[]) {
-    super(`missing outputs (count=${missingOutputs.length})`);
-  }
-}
-
-export class ErrorImplicitExternalOutputs extends ValidationError {
-  constructor(public implicitExternalOutputs: BaseOutput<bigint | 'max'>[]) {
-    super(`unexpected implicit external outputs (count=${implicitExternalOutputs.length})`);
-  }
-}
-
-export class AggregateValidationError extends ValidationError {
-  constructor(public errors: ValidationError[]) {
-    super(`aggregate validation error (count=${errors.length})`);
-  }
-}
-
+/**
+ * Assert that the transaction outputs match expected outputs
+ *
+ * @param parsedOutputs - Parsed transaction outputs including missing and implicit external outputs
+ * @throws {TxIntentMismatchRecipientError} if outputs don't match expected recipients
+ */
 export function assertExpectedOutputDifference(
   parsedOutputs: BaseParsedTransactionOutputs<bigint, BaseOutput<bigint | 'max'>>
 ): void {
-  const errors: ValidationError[] = [];
+  const errorMessages: string[] = [];
+  const allMismatchedRecipients: MismatchedRecipient[] = [];
+
+  // Check for missing outputs - these are recipients that the user intended but weren't included
   if (parsedOutputs.missingOutputs.length > 0) {
-    errors.push(new ErrorMissingOutputs(parsedOutputs.missingOutputs));
+    errorMessages.push(`missing outputs (count=${parsedOutputs.missingOutputs.length})`);
+    const missingRecipients: MismatchedRecipient[] = parsedOutputs.missingOutputs.map((output) => ({
+      address: output.address,
+      amount: output.amount.toString(),
+    }));
+    allMismatchedRecipients.push(...missingRecipients);
   }
+
+  // Check for implicit external outputs - these are outputs the user didn't request
   if (parsedOutputs.implicitExternalOutputs.length > 0) {
     // FIXME: for paygo we need to relax this a little bit
-    errors.push(new ErrorImplicitExternalOutputs(parsedOutputs.implicitExternalOutputs));
+    errorMessages.push(`unexpected implicit external outputs (count=${parsedOutputs.implicitExternalOutputs.length})`);
+    const implicitRecipients: MismatchedRecipient[] = parsedOutputs.implicitExternalOutputs.map((output) => ({
+      address: output.address,
+      amount: output.amount.toString(),
+    }));
+    allMismatchedRecipients.push(...implicitRecipients);
   }
-  if (errors.length > 0) {
-    // FIXME(BTC-1688): enable ES2021
-    // throw new AggregateError(errors);
-    throw new AggregateValidationError(errors);
+
+  // If there are any mismatches, throw a single error with all the details
+  if (errorMessages.length > 0) {
+    const message = errorMessages.join(', ');
+    throw new TxIntentMismatchRecipientError(message, undefined, [], undefined, allMismatchedRecipients);
   }
 }
 
@@ -68,6 +72,7 @@ export function assertValidTransaction(
  * @param descriptorMap
  * @returns {boolean} True if verification passes
  * @throws {TxIntentMismatchError} if transaction validation fails
+ * @throws {TxIntentMismatchRecipientError} if transaction recipients don't match user intent
  */
 export async function verifyTransaction(
   coin: AbstractUtxoCoin,

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

@@ -55,11 +55,11 @@ export async function verifyTransaction<TNumber extends bigint | number>(
   };
 
   if (!_.isUndefined(verification.disableNetworking) && !_.isBoolean(verification.disableNetworking)) {
-    throwTxMismatch('verification.disableNetworking must be a boolean');
+    throw new TypeError('verification.disableNetworking must be a boolean');
   }
   const isPsbt = txPrebuild.txHex && utxolib.bitgo.isPsbt(txPrebuild.txHex);
   if (isPsbt && txPrebuild.txInfo?.unspents) {
-    throwTxMismatch('should not have unspents in txInfo for psbt');
+    throw new Error('should not have unspents in txInfo for psbt');
   }
   const disableNetworking = !!verification.disableNetworking;
   const parsedTransaction: ParsedTransaction<TNumber> = await coin.parseTransaction<TNumber>({
@@ -97,7 +97,7 @@ export async function verifyTransaction<TNumber extends bigint | number>(
     const isBackupKeySignatureValid = verify(keychains.backup, keySignatures.backupPub);
     const isBitgoKeySignatureValid = verify(keychains.bitgo, keySignatures.bitgoPub);
     if (!isBackupKeySignatureValid || !isBitgoKeySignatureValid) {
-      throwTxMismatch('secondary public key signatures invalid');
+      throw new Error('secondary public key signatures invalid');
     }
     debug('successfully verified backup and bitgo key signatures');
   } else if (!disableNetworking) {
@@ -108,11 +108,11 @@ export async function verifyTransaction<TNumber extends bigint | number>(
 
   if (parsedTransaction.needsCustomChangeKeySignatureVerification) {
     if (!keychains.user || !userPublicKeyVerified) {
-      throwTxMismatch('transaction requires verification of user public key, but it was unable to be verified');
+      throw new Error('transaction requires verification of user public key, but it was unable to be verified');
     }
     const customChangeKeySignaturesVerified = verifyCustomChangeKeySignatures(parsedTransaction, keychains.user);
     if (!customChangeKeySignaturesVerified) {
-      throwTxMismatch(
+      throw new Error(
         'transaction requires verification of custom change key signatures, but they were unable to be verified'
       );
     }
@@ -168,7 +168,7 @@ export async function verifyTransaction<TNumber extends bigint | number>(
 
   const allOutputs = parsedTransaction.outputs;
   if (!txPrebuild.txHex) {
-    throw new TxIntentMismatchError(`txPrebuild.txHex not set`, reqId, [txParams], undefined);
+    throw new Error(`txPrebuild.txHex not set`);
   }
   const inputs = isPsbt
     ? getPsbtTxInputs(txPrebuild.txHex, coin.network).map((v) => ({
@@ -185,7 +185,7 @@ export async function verifyTransaction<TNumber extends bigint | number>(
   const fee = inputAmount - outputAmount;
 
   if (fee < 0) {
-    throwTxMismatch(
+    throw new Error(
       `attempting to spend ${outputAmount} satoshis, which exceeds the input amount (${inputAmount} satoshis) by ${-fee}`
     );
   }

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

@@ -10,14 +10,10 @@ import {
 } from '@bitgo/utxo-core/testutil/descriptor';
 import { toPlainObject } from '@bitgo/utxo-core/testutil';
 import { createAddressFromDescriptor } from '@bitgo/utxo-core/descriptor';
+import { TxIntentMismatchRecipientError } from '@bitgo/sdk-core';
 
 import { ParsedOutputsBigInt, toBaseParsedTransactionOutputsFromPsbt } from '../../../src/transaction/descriptor/parse';
-import {
-  AggregateValidationError,
-  assertExpectedOutputDifference,
-  ErrorImplicitExternalOutputs,
-  ErrorMissingOutputs,
-} from '../../../src/transaction/descriptor/verifyTransaction';
+import { assertExpectedOutputDifference } from '../../../src/transaction/descriptor/verifyTransaction';
 import { toAmountType } from '../../../src/transaction/descriptor/parseToAmountType';
 import { BaseOutput } from '../../../src';
 
@@ -38,10 +34,6 @@ function toBaseOutput<TNumber>(output: OutputWithValue, amountType: 'bigint' | '
   };
 }
 
-function toBaseOutputBigInt(output: OutputWithValue): BaseOutput<bigint> {
-  return toBaseOutput(output, 'bigint');
-}
-
 function toBaseOutputString(output: OutputWithValue): BaseOutput<string> {
   return toBaseOutput(output, 'string');
 }
@@ -97,66 +89,69 @@ describe('parse', function () {
       );
     });
 
-    function assertEqualValidationError(actual: unknown, expected: AggregateValidationError) {
-      function normErrors(e: Error[]): Error[] {
-        return e.map((e) => ({ ...e, stack: undefined }));
-      }
-      if (actual instanceof AggregateValidationError) {
-        assert.deepStrictEqual(normErrors(actual.errors), normErrors(expected.errors));
-      } else {
-        throw new Error('unexpected error type: ' + actual);
-      }
-    }
-
-    function assertValidationError(f: () => void, expected: AggregateValidationError) {
-      assert.throws(f, (err) => {
-        assertEqualValidationError(err, expected);
+    function assertTxIntentMismatchError(
+      f: () => void,
+      expectedMessage: string,
+      expectedMismatchedRecipients: { address?: string; amount: string }[]
+    ) {
+      assert.throws(f, (err: unknown) => {
+        if (!(err instanceof TxIntentMismatchRecipientError)) {
+          const error = err as Error;
+          throw new Error(
+            `Expected TxIntentMismatchRecipientError but got ${error.constructor.name}: ${error.message}`
+          );
+        }
+        assert.strictEqual(err.message, expectedMessage);
+        assert.deepStrictEqual(err.mismatchedRecipients, expectedMismatchedRecipients);
         return true;
       });
     }
 
-    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(
+      assertTxIntentMismatchError(
         () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [])),
-        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
+        'unexpected implicit external outputs (count=1)',
+        [{ address: psbt.txOutputs[0].address, amount: psbt.txOutputs[0].value.toString() }]
       );
     });
 
     it('should throw expected error: only internal recipient requested', function () {
-      assertValidationError(
+      assertTxIntentMismatchError(
         () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [psbt.txOutputs[1]])),
-        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
+        'unexpected implicit external outputs (count=1)',
+        [{ address: psbt.txOutputs[0].address, amount: psbt.txOutputs[0].value.toString() }]
       );
     });
 
     it('should throw expected error: only internal max recipient requested', function () {
-      assertValidationError(
+      assertTxIntentMismatchError(
         () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [toMaxOutput(psbt.txOutputs[1])])),
-        new AggregateValidationError([implicitOutputError(psbt.txOutputs[0])])
+        'unexpected implicit external outputs (count=1)',
+        [{ address: psbt.txOutputs[0].address, amount: psbt.txOutputs[0].value.toString() }]
       );
     });
 
     it('should throw expected error: swapped recipient', function () {
       const recipient = externalRecipient(99);
-      assertValidationError(
+      assertTxIntentMismatchError(
         () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [recipient])),
-        new AggregateValidationError([missingOutputError(recipient), implicitOutputError(psbt.txOutputs[0])])
+        'missing outputs (count=1), unexpected implicit external outputs (count=1)',
+        [
+          { address: recipient.address, amount: recipient.value.toString() },
+          { address: psbt.txOutputs[0].address, amount: psbt.txOutputs[0].value.toString() },
+        ]
       );
     });
 
     it('should throw expected error: missing internal recipient', function () {
       const recipient = internalRecipient(99);
-      assertValidationError(
+      assertTxIntentMismatchError(
         () => assertExpectedOutputDifference(getBaseParsedTransaction(psbt, [recipient])),
-        new AggregateValidationError([missingOutputError(recipient), implicitOutputError(psbt.txOutputs[0])])
+        'missing outputs (count=1), unexpected implicit external outputs (count=1)',
+        [
+          { address: recipient.address, amount: recipient.value.toString() },
+          { address: psbt.txOutputs[0].address, amount: psbt.txOutputs[0].value.toString() },
+        ]
       );
     });
   });