Skip to content

feat: add tx explanation for TxIntentMismatch errors #7580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+284 −18
Open

feat: add tx explanation for TxIntentMismatch errors #7580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions modules/abstract-eth/src/abstractEthLikeNewCoins.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2798,6 +2798,34 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
return txPrebuild.coin === nativeCoin;
}

/**
* Generate transaction explanation for error reporting
* @param txPrebuild - Transaction prebuild containing txHex and fee info
* @returns Stringified JSON explanation
*/
private async getTxExplanation(txPrebuild?: TransactionPrebuild): Promise<string | undefined> {
if (!txPrebuild?.txHex || !txPrebuild?.gasPrice) {
return undefined;
}

try {
const explanation = await this.explainTransaction({
txHex: txPrebuild.txHex,
feeInfo: {
fee: txPrebuild.gasPrice.toString(),
},
});
return JSON.stringify(explanation, null, 2);
} catch (e) {
const errorDetails = {
error: 'Failed to parse transaction explanation',
txHex: txPrebuild.txHex,
details: e instanceof Error ? e.message : String(e),
};
return JSON.stringify(errorDetails, null, 2);
}
}

/**
* Verify if a tss transaction is valid
*
Expand All @@ -2811,9 +2839,18 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise<boolean> {
const { txParams, txPrebuild, wallet } = params;

const txExplanation = await this.getTxExplanation(txPrebuild);
Copy link
Contributor

@mullapudipruthvik mullapudipruthvik Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have this inside throwRecipientMismatch function ?
so that its only called when required

Copy link
Contributor

@mullapudipruthvik mullapudipruthvik Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and probably have the string conversion inside, instead of a new function


// Helper to throw TxIntentMismatchRecipientError with recipient details
const throwRecipientMismatch = (message: string, mismatchedRecipients: Recipient[]): never => {
throw new TxIntentMismatchRecipientError(message, undefined, [txParams], txPrebuild?.txHex, mismatchedRecipients);
throw new TxIntentMismatchRecipientError(
message,
undefined,
[txParams],
txPrebuild?.txHex,
mismatchedRecipients,
txExplanation
);
};

if (
Expand Down Expand Up @@ -2915,9 +2952,18 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
return this.verifyTssTransaction(params);
}

const txExplanation = await this.getTxExplanation(txPrebuild);

// Helper to throw TxIntentMismatchRecipientError with recipient details
const throwRecipientMismatch = (message: string, mismatchedRecipients: Recipient[]): never => {
throw new TxIntentMismatchRecipientError(message, undefined, [txParams], txPrebuild?.txHex, mismatchedRecipients);
throw new TxIntentMismatchRecipientError(
message,
undefined,
[txParams],
txPrebuild?.txHex,
mismatchedRecipients,
txExplanation
);
};

if (!txParams?.recipients || !txPrebuild?.recipients || !wallet) {
Expand Down Expand Up @@ -3019,7 +3065,8 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
'coin in txPrebuild did not match that in txParams supplied by client',
undefined,
[txParams],
txPrebuild?.txHex
txPrebuild?.txHex,
txExplanation
);
}
return true;
Expand Down
17 changes: 14 additions & 3 deletions modules/abstract-utxo/src/abstractUtxoCoin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {
assertValidTransactionRecipient,
explainTx,
fromExtendedAddressFormat,
getTxExplanation,
isScriptRecipient,
parseTransaction,
verifyTransaction,
Expand Down Expand Up @@ -143,7 +144,8 @@ function convertValidationErrorToTxIntentMismatch(
error: AggregateValidationError,
reqId: string | IRequestTracer | undefined,
txParams: BaseTransactionParams,
txHex: string | undefined
txHex: string | undefined,
txExplanation?: string
): TxIntentMismatchRecipientError {
const mismatchedRecipients: MismatchedRecipient[] = [];

Expand All @@ -170,7 +172,8 @@ function convertValidationErrorToTxIntentMismatch(
reqId,
[txParams],
txHex,
mismatchedRecipients
mismatchedRecipients,
txExplanation
);
// Preserve the original structured error as the cause for debugging
// See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
Expand Down Expand Up @@ -615,11 +618,19 @@ export abstract class AbstractUtxoCoin extends BaseCoin {
async verifyTransaction<TNumber extends number | bigint = number>(
params: VerifyTransactionOptions<TNumber>
): Promise<boolean> {
const txExplanation = await getTxExplanation(this, params.txPrebuild);
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty wasteful if we only use it in the catch block - move it there


try {
return await verifyTransaction(this, this.bitgo, params);
} catch (error) {
if (error instanceof AggregateValidationError) {
throw convertValidationErrorToTxIntentMismatch(error, params.reqId, params.txParams, params.txPrebuild.txHex);
throw convertValidationErrorToTxIntentMismatch(
error,
params.reqId,
params.txParams,
params.txPrebuild.txHex,
txExplanation
);
}
throw error;
}
Expand Down
6 changes: 5 additions & 1 deletion modules/abstract-utxo/src/transaction/descriptor/verifyTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { DescriptorMap } from '@bitgo/utxo-core/descriptor';

import { AbstractUtxoCoin, VerifyTransactionOptions } from '../../abstractUtxoCoin';
import { BaseOutput, BaseParsedTransactionOutputs } from '../types';
import { getTxExplanation } from '../txExplanation';

import { toBaseParsedTransactionOutputsFromPsbt } from './parse';

Expand Down Expand Up @@ -75,13 +76,16 @@ export async function verifyTransaction<TNumber extends number | bigint>(
params: VerifyTransactionOptions<TNumber>,
descriptorMap: DescriptorMap
): Promise<boolean> {
const txExplanation = await getTxExplanation(coin, params.txPrebuild);
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same


const tx = coin.decodeTransactionFromPrebuild(params.txPrebuild);
if (!(tx instanceof utxolib.bitgo.UtxoPsbt)) {
throw new TxIntentMismatchError(
'unexpected transaction type',
params.reqId,
[params.txParams],
params.txPrebuild.txHex
params.txPrebuild.txHex,
txExplanation
);
}

Expand Down
5 changes: 4 additions & 1 deletion modules/abstract-utxo/src/transaction/fixedScript/verifyTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { AbstractUtxoCoin, VerifyTransactionOptions } from '../../abstractUtxoCo
import { Output, ParsedTransaction } from '../types';
import { verifyCustomChangeKeySignatures, verifyKeySignature, verifyUserPublicKey } from '../../verifyKey';
import { getPsbtTxInputs, getTxInputs } from '../fetchInputs';
import { getTxExplanation } from '../txExplanation';

const debug = buildDebug('bitgo:abstract-utxo:verifyTransaction');

Expand Down Expand Up @@ -50,9 +51,11 @@ export async function verifyTransaction<TNumber extends bigint | number>(
): Promise<boolean> {
const { txParams, txPrebuild, wallet, verification = {}, reqId } = params;

const txExplanation = await getTxExplanation(coin, txPrebuild);

// Helper to throw TxIntentMismatchError with consistent context
const throwTxMismatch = (message: string): never => {
throw new TxIntentMismatchError(message, reqId, [txParams], txPrebuild.txHex);
throw new TxIntentMismatchError(message, reqId, [txParams], txPrebuild.txHex, txExplanation);
};

if (!_.isUndefined(verification.disableNetworking) && !_.isBoolean(verification.disableNetworking)) {
Expand Down
1 change: 1 addition & 0 deletions modules/abstract-utxo/src/transaction/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ export * from './recipient';
export { explainTx } from './explainTransaction';
export { parseTransaction } from './parseTransaction';
export { verifyTransaction } from './verifyTransaction';
export { getTxExplanation } from './txExplanation';
export * from './fetchInputs';
export * as bip322 from './bip322';
31 changes: 31 additions & 0 deletions modules/abstract-utxo/src/transaction/txExplanation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { AbstractUtxoCoin, TransactionPrebuild } from '../abstractUtxoCoin';

/**
* Generate a stringified transaction explanation for error reporting
* @param coin - The UTXO coin instance
* @param txPrebuild - Transaction prebuild containing txHex and txInfo
* @returns Stringified JSON explanation
*/
export async function getTxExplanation<TNumber extends number | bigint>(
coin: AbstractUtxoCoin,
txPrebuild: TransactionPrebuild<TNumber>
): Promise<string | undefined> {
if (!txPrebuild.txHex) {
return undefined;
}

try {
const explanation = await coin.explainTransaction({
txHex: txPrebuild.txHex,
txInfo: txPrebuild.txInfo,
});
return JSON.stringify(explanation, null, 2);
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will not work with bigint values

Copy link
Contributor Author

@danielpeng1 danielpeng1 Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do a bigint-safe JSON serialization. Can I create a test file modules/abstract-utxo/test/unit/txExplanation.ts to test this

Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to a return a string here anyway? we already have src/transaction/explainTransaction.ts already as well, so why are we adding another file here?

why can't the caller of this func (whoever that is) deal with the string conversion?

Copy link
Contributor Author

@danielpeng1 danielpeng1 Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the alternative to return the object (explanation)? Then the parent error class TxIntentMismatchError's constructor would handle the JSON.stringify for example?

regardless I can move this function to src/transaction/explainTransaction.ts

Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the alternative to return the object (explanation)? Then the parent error class TxIntentMismatchError's constructor would handle the JSON.stringify for example?

yes

regardless I can move this function to src/transaction/explainTransaction.ts

I don't think we need a new function

Copy link
Contributor Author

@danielpeng1 danielpeng1 Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then we would need to duplicate/in-line this try catch logic in each call site? In descriptor/verifyTransaction.ts, fixedScript/verifyTransaction.ts, abstractUtxoCoin.ts

Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about https://github.com/BitGo/BitGoJS/pull/7580/files/418878dd16a114c8dae0b233019abc5ad4bdf41f#r2546538479

Copy link
Contributor Author

@danielpeng1 danielpeng1 Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this is returning a string again though? (instead of the explanation object)

Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but at least it is relegated to the error class, so I'm not as concerned about the interface

you can implement it returning unknown or string | undefined, your call

} catch (e) {
const errorDetails = {
error: 'Failed to parse transaction explanation',
txHex: txPrebuild.txHex,
details: e instanceof Error ? e.message : String(e),
};
return JSON.stringify(errorDetails, null, 2);
}
}
25 changes: 18 additions & 7 deletions modules/sdk-core/src/bitgo/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,13 @@ export interface ContractDataPayload {
* @property {string | IRequestTracer | undefined} id - Transaction ID or request tracer for tracking
* @property {TransactionParams[]} txParams - Array of transaction parameters that were analyzed
* @property {string | undefined} txHex - The raw transaction in hexadecimal format
* @property {string | undefined} txExplanation - Stringified transaction explanation
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better make this unknown

*/
export class TxIntentMismatchError extends BitGoJsError {
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a static function here

static tryGetTxExplanation(coin: AbstractCoin, tx): Promise<string | undefined> {
   try {
      return bigintSafeStringify(coin.getExplanation(tx));
   } catch (e) {
     return String(e); // with tx etc
   }
 }

public readonly id: string | IRequestTracer | undefined;
public readonly txParams: TransactionParams[];
public readonly txHex: string | undefined;
public readonly txExplanation: string | undefined;

/**
* Creates an instance of TxIntentMismatchError
Expand All @@ -269,17 +271,20 @@ export class TxIntentMismatchError extends BitGoJsError {
* @param {string | IRequestTracer | undefined} id - Transaction ID or request tracer
* @param {TransactionParams[]} txParams - Transaction parameters that were analyzed
* @param {string | undefined} txHex - Raw transaction hex string
* @param {string | undefined} txExplanation - Stringified transaction explanation
*/
public constructor(
message: string,
id: string | IRequestTracer | undefined,
txParams: TransactionParams[],
txHex: string | undefined
txHex: string | undefined,
txExplanation?: string
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unknown here

) {
super(message);
this.id = id;
this.txParams = txParams;
this.txHex = txHex;
this.txExplanation = txExplanation;
Copy link
Contributor

@OttoAllmendinger OttoAllmendinger Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do some sort of safe stringification here (JSON.stringify with bigint substitution)

}
}

Expand All @@ -304,15 +309,17 @@ export class TxIntentMismatchRecipientError extends TxIntentMismatchError {
* @param {TransactionParams[]} txParams - Transaction parameters that were analyzed
* @param {string | undefined} txHex - Raw transaction hex string
* @param {MismatchedRecipient[]} mismatchedRecipients - Array of recipients that don't match user intent
* @param {string | undefined} txExplanation - Stringified transaction explanation
*/
public constructor(
message: string,
id: string | IRequestTracer | undefined,
txParams: TransactionParams[],
txHex: string | undefined,
mismatchedRecipients: MismatchedRecipient[]
mismatchedRecipients: MismatchedRecipient[],
txExplanation?: string
) {
super(message, id, txParams, txHex);
super(message, id, txParams, txHex, txExplanation);
this.mismatchedRecipients = mismatchedRecipients;
}
}
Expand All @@ -338,15 +345,17 @@ export class TxIntentMismatchContractError extends TxIntentMismatchError {
* @param {TransactionParams[]} txParams - Transaction parameters that were analyzed
* @param {string | undefined} txHex - Raw transaction hex string
* @param {ContractDataPayload} mismatchedDataPayload - The contract interaction data that doesn't match user intent
* @param {string | undefined} txExplanation - Stringified transaction explanation
*/
public constructor(
message: string,
id: string | IRequestTracer | undefined,
txParams: TransactionParams[],
txHex: string | undefined,
mismatchedDataPayload: ContractDataPayload
mismatchedDataPayload: ContractDataPayload,
txExplanation?: string
) {
super(message, id, txParams, txHex);
super(message, id, txParams, txHex, txExplanation);
this.mismatchedDataPayload = mismatchedDataPayload;
}
}
Expand All @@ -372,15 +381,17 @@ export class TxIntentMismatchApprovalError extends TxIntentMismatchError {
* @param {TransactionParams[]} txParams - Transaction parameters that were analyzed
* @param {string | undefined} txHex - Raw transaction hex string
* @param {TokenApproval} tokenApproval - Details of the token approval that doesn't match user intent
* @param {string | undefined} txExplanation - Stringified transaction explanation
*/
public constructor(
message: string,
id: string | IRequestTracer | undefined,
txParams: TransactionParams[],
txHex: string | undefined,
tokenApproval: TokenApproval
tokenApproval: TokenApproval,
txExplanation?: string
) {
super(message, id, txParams, txHex);
super(message, id, txParams, txHex, txExplanation);
this.tokenApproval = tokenApproval;
}
}
Loading