-
Notifications
You must be signed in to change notification settings - Fork 300
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.
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
base: master
Are you sure you want to change the base?
Conversation
| txHex: txPrebuild.txHex, | ||
| txInfo: txPrebuild.txInfo, | ||
| }); | ||
| return JSON.stringify(explanation, null, 2); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
| async verifyTransaction<TNumber extends number | bigint = number>( | ||
| params: VerifyTransactionOptions<TNumber> | ||
| ): Promise<boolean> { | ||
| const txExplanation = await getTxExplanation(this, params.txPrebuild); |
There was a problem hiding this comment.
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
| params: VerifyTransactionOptions<TNumber>, | ||
| descriptorMap: DescriptorMap | ||
| ): Promise<boolean> { | ||
| const txExplanation = await getTxExplanation(coin, params.txPrebuild); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
OttoAllmendinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better deal with the string explanation -> conversion in the sdk-core errors
| txHex: txPrebuild.txHex, | ||
| txInfo: txPrebuild.txInfo, | ||
| }); | ||
| return JSON.stringify(explanation, null, 2); |
There was a problem hiding this comment.
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?
| * @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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better make this unknown
| txParams: TransactionParams[], | ||
| txHex: string | undefined | ||
| txHex: string | undefined, | ||
| txExplanation?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unknown here
| this.id = id; | ||
| this.txParams = txParams; | ||
| this.txHex = txHex; | ||
| this.txExplanation = txExplanation; |
There was a problem hiding this comment.
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)
| * @property {string | undefined} txHex - The raw transaction in hexadecimal format | ||
| * @property {string | undefined} txExplanation - Stringified transaction explanation | ||
| */ | ||
| export class TxIntentMismatchError extends BitGoJsError { |
There was a problem hiding this comment.
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
}
}
| txHex: txPrebuild.txHex, | ||
| txInfo: txPrebuild.txInfo, | ||
| }); | ||
| return JSON.stringify(explanation, null, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| async verifyTssTransaction(params: VerifyEthTransactionOptions): Promise<boolean> { | ||
| const { txParams, txPrebuild, wallet } = params; | ||
|
|
||
| const txExplanation = await this.getTxExplanation(txPrebuild); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Adding tx explanation for TxIntentMismatch errors for UTXO and ETH-like coins.
getTxExplanationmethod in bothAbstractEthLikeNewCoinsandAbstractUtxoCointo generate detailed transaction explanations for error reporting.txExplanation.tsto use inabstractUtxoCoin.tsand bothverifyTransactionsTxIntentMismatchErrorand its subclasses.Ticket: WP-6608