Skip to content

Conversation

mohammadalfaiyazbitgo
Copy link
Contributor

  • Add base SuspiciousTransactionError class extending BitGoJsError
  • Add SuspiciousTransactionParameterMismatchError for recipient mismatches
  • Add SuspiciousContractInteractionError for contract interaction issues
  • Add SuspiciousUnauthorizedTokenApproval for unauthorized token approvals
  • Add complete unit test suite with 7 test cases covering all error scenarios

TICKET: WP-6187

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo force-pushed the WP-6187/define-suspicious-transaction-error-classes branch from 66411ac to c9ae74b Compare October 2, 2025 19:04
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo changed the title feat(sdk-core): add suspicious transaction error classes feat(sdk-core): add transaction intent mismatch errors Oct 2, 2025
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo marked this pull request as ready for review October 2, 2025 19:41
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo requested review from a team as code owners October 2, 2025 19:41
mukeshsp
mukeshsp previously approved these changes Oct 3, 2025
mrdanish26
mrdanish26 previously approved these changes Oct 3, 2025
@pranavjain97 pranavjain97 requested a review from Copilot October 3, 2025 20:05
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a comprehensive error handling system for detecting transaction intent mismatches in the BitGo SDK, specifically focusing on identifying potentially malicious or unintended transaction modifications. The changes add specialized error classes and corresponding unit tests for various types of transaction anomalies.

  • Adds base TxIntentMismatchError class and three specialized error types for recipient mismatches, contract interaction issues, and unauthorized token approvals
  • Introduces comprehensive type definitions for error payloads including TokenApproval, MismatchedRecipient, and ContractDataPayload interfaces
  • Includes complete unit test coverage with 7 test cases validating error creation, inheritance, and property assignment

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
modules/sdk-core/src/bitgo/errors.ts Adds new error classes and interfaces for transaction intent mismatch detection
modules/sdk-core/test/unit/bitgo/errors.ts Provides comprehensive unit tests for all new error classes and their inheritance hierarchy

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}

export class ApiResponseError<ResponseBodyType = any> extends BitGoJsError {
export class ApiResponseError<ResponseBodyType = unknown> extends BitGoJsError {
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

This change from any to unknown is a good practice for type safety. However, this appears to be an unrelated change to the main PR purpose. Consider separating type safety improvements into a separate commit or PR.

Suggested change
export class ApiResponseError<ResponseBodyType = unknown> extends BitGoJsError {
export class ApiResponseError<ResponseBodyType = any> extends BitGoJsError {

Copilot uses AI. Check for mistakes.

Comment on lines 205 to 216
* Interface for token approval information used in suspicious transaction detection
*
* @interface TokenApproval
* @property {string} [tokenName] - Optional human-readable name of the token
* @property {string} tokenAddress - The contract address of the token being approved
* @property {number | 'unlimited'} authorizingAmount - The amount being authorized for spending, or 'unlimited' for infinite approval
* @property {string} authorizingAddress - The address being authorized to spend the tokens
*/
export interface TokenApproval {
tokenName?: string;
tokenAddress: string;
authorizingAmount: number | 'unlimited';
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

The authorizingAmount property has inconsistent types (number | 'unlimited') which could lead to confusion. Consider using a more structured approach like { type: 'limited', amount: number } | { type: 'unlimited' } or standardizing on string representation for both cases.

Suggested change
* Interface for token approval information used in suspicious transaction detection
*
* @interface TokenApproval
* @property {string} [tokenName] - Optional human-readable name of the token
* @property {string} tokenAddress - The contract address of the token being approved
* @property {number | 'unlimited'} authorizingAmount - The amount being authorized for spending, or 'unlimited' for infinite approval
* @property {string} authorizingAddress - The address being authorized to spend the tokens
*/
export interface TokenApproval {
tokenName?: string;
tokenAddress: string;
authorizingAmount: number | 'unlimited';
* Type for authorizing amount in token approval, using a discriminated union for clarity and type safety
*/
export type AuthorizingAmount =
| { type: 'limited'; amount: number }
| { type: 'unlimited' };
/**
* Interface for token approval information used in suspicious transaction detection
*
* @interface TokenApproval
* @property {string} [tokenName] - Optional human-readable name of the token
* @property {string} tokenAddress - The contract address of the token being approved
* @property {AuthorizingAmount} authorizingAmount - The amount being authorized for spending, or unlimited for infinite approval
* @property {string} authorizingAddress - The address being authorized to spend the tokens
*/
export interface TokenApproval {
tokenName?: string;
tokenAddress: string;
authorizingAmount: AuthorizingAmount;

Copilot uses AI. Check for mistakes.

Comment on lines 226 to 235
* @property {string | TokenTransferRecipientParams} [data] - Optional transaction data or token transfer parameters
* @property {string} [tokenName] - Optional name of the token being transferred
* @property {TokenTransferRecipientParams} [tokenData] - Optional structured token transfer data
*/
export interface MismatchedRecipient {
address: string;
amount: string;
data?: string | TokenTransferRecipientParams;
tokenName?: string;
tokenData?: TokenTransferRecipientParams;
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

The data property allows both string and TokenTransferRecipientParams types, but there's also a separate tokenData property for the same purpose. This creates ambiguity about which field to use. Consider consolidating these into a single, well-defined property.

Suggested change
* @property {string | TokenTransferRecipientParams} [data] - Optional transaction data or token transfer parameters
* @property {string} [tokenName] - Optional name of the token being transferred
* @property {TokenTransferRecipientParams} [tokenData] - Optional structured token transfer data
*/
export interface MismatchedRecipient {
address: string;
amount: string;
data?: string | TokenTransferRecipientParams;
tokenName?: string;
tokenData?: TokenTransferRecipientParams;
* @property {string | TokenTransferRecipientParams} [data] - Optional transaction data; if a token transfer, this may be a TokenTransferRecipientParams object
* @property {string} [tokenName] - Optional name of the token being transferred
*/
export interface MismatchedRecipient {
address: string;
amount: string;
data?: string | TokenTransferRecipientParams;
tokenName?: string;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworked to use base recipients type.

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo force-pushed the WP-6187/define-suspicious-transaction-error-classes branch 2 times, most recently from 6c5dacf to a38004c Compare October 3, 2025 20:40
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

export interface TokenApproval {
tokenName?: string;
tokenAddress: string;
authorizingAmount: { type: 'unlimited' } | { type: 'limited'; amount: number };
Copy link
Preview

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

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

The JSDoc comment on line 210 incorrectly states the type as number | 'unlimited' but the actual TypeScript type is a discriminated union with { type: 'unlimited' } or { type: 'limited'; amount: number }. The documentation should be updated to match the implementation.

Copilot uses AI. Check for mistakes.

- Add base TxIntentMismatchError class extending BitGoJsError
- Add TxIntentMismatchRecipientError for recipient intent mismatches
- Add TxIntentMismatchContractError for contract interaction intent mismatches
- Add TxIntentMismatchApprovalError for token approval intent mismatches

TICKET: WP-6187
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo force-pushed the WP-6187/define-suspicious-transaction-error-classes branch from a38004c to b4ce897 Compare October 3, 2025 20:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants