Skip to content

fix(sdk-core): add validation for unsigned txHex #5886

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

Merged

Conversation

mullapudipruthvik
Copy link
Contributor

@mullapudipruthvik mullapudipruthvik commented Apr 4, 2025

Ticket: COIN-3222

TICKET: COIN-3222

We want to verify the txHex with params provided by client before signing

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-3222-add-verifyTransaction-before-signing branch 3 times, most recently from 9dfec05 to 7a73c51 Compare April 17, 2025 13:25
@mullapudipruthvik mullapudipruthvik marked this pull request as ready for review April 21, 2025 04:50
@mullapudipruthvik mullapudipruthvik requested review from a team as code owners April 21, 2025 04:50
@dpkjnr dpkjnr requested a review from Copilot April 23, 2025 05:27
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 adds validation for unsigned txHex by introducing a new txParams field and corresponding verification steps before signing transactions. Key changes include:

  • Passing txParams from txPrebuild to the signTxRequest call.
  • Updating the TSS ECDSA utilities (both MPCv2 and classic) to verify transactions using the provided txParams.
  • Adjusting test cases and related type definitions to support these new validation steps.

Reviewed Changes

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

Show a summary per file
File Description
modules/sdk-core/src/bitgo/wallet/wallet.ts Passes new txParams from txPrebuild to the TSS signTxRequest method.
modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsaMPCv2.ts Introduces transaction verification by calling verifyTransaction with appropriate parameters.
modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts Adds transaction verification before signing in a similar manner as in the MPCv2 utility.
modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts Extends TSSParams with a new optional txParams property.
modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts Updates the wallet type from Wallet to IWallet to better abstract the dependency.
modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts Updates test cases to check for proper error messages when recipient or amount mismatches occur.
modules/abstract-eth/src/abstractEthLikeNewCoins.ts Changes in transaction type validation and adds async verification of TSS transactions.
modules/abstract-cosmos/src/cosmosCoin.ts Minor formatting change following transaction building.
Comments suppressed due to low confidence (2)

modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts:955

  • [nitpick] Confirm that the error message for a recipient address mismatch is consistent across all validation cases.
.should.be.rejectedWith('destination address does not match with the recipient address');

modules/bitgo/test/v2/unit/internal/tssUtils/ecdsa.ts:974

  • [nitpick] Ensure that the error message for an amount mismatch remains consistent with other areas of the codebase that implement similar validations.
.should.be.rejectedWith('the transaction amount in txPrebuild does not match the value given by client');

const { txParams, txPrebuild, wallet } = params;
if (
!txParams?.recipients &&
!(
txParams.prebuildTx?.consolidateId ||
(txParams.type && ['acceleration', 'fillNonce', 'transferToken'].includes(txParams.type))
(txParams.type && ['acceleration', 'fillNonce', 'transfer'].includes(txParams.type))
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

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

Review the allowed transaction types here since 'transferToken' was replaced with 'transfer'. Ensure this change is intentional and that it aligns with the client specifications.

Suggested change
(txParams.type && ['acceleration', 'fillNonce', 'transfer'].includes(txParams.type))
(txParams.type && ['acceleration', 'fillNonce', 'transfer', 'transferToken'].includes(txParams.type))

Copilot uses AI. Check for mistakes.

@mullapudipruthvik mullapudipruthvik force-pushed the COIN-3222-add-verifyTransaction-before-signing branch from 7a73c51 to 7251d82 Compare April 23, 2025 11:42
@mullapudipruthvik mullapudipruthvik force-pushed the COIN-3222-add-verifyTransaction-before-signing branch from 7251d82 to eff30e3 Compare April 23, 2025 12:27
@mullapudipruthvik mullapudipruthvik merged commit a4b9db6 into master Apr 28, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants