Skip to content

Conversation

@kamleshmugdiya
Copy link
Contributor

@kamleshmugdiya kamleshmugdiya commented May 21, 2025

Ticket: COIN-4071

@kamleshmugdiya kamleshmugdiya force-pushed the COIN-4071 branch 2 times, most recently from 2e4c831 to 40efbfd Compare May 27, 2025 09:40
@kamleshmugdiya kamleshmugdiya force-pushed the COIN-4071 branch 6 times, most recently from f95933e to edbe6f3 Compare July 11, 2025 10:08
@kamleshmugdiya kamleshmugdiya marked this pull request as ready for review July 11, 2025 10:13
@kamleshmugdiya kamleshmugdiya requested review from a team as code owners July 11, 2025 10:13
@kamleshmugdiya kamleshmugdiya force-pushed the COIN-4071 branch 2 times, most recently from bfe731d to 82c51de Compare July 11, 2025 10:53
@sampras-saha
Copy link
Contributor

@claude can you review?

Copy link
Contributor

@sampras-saha sampras-saha left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

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

Adds support for ERC20 token approvals within MPC flows and the Wallet API.

  • Introduces a new tokenApproval intent across TSS utils and MPC utilities.
  • Updates PopulatedIntent and AbstractEthLikeNewCoins to recognize and pass tokenName.
  • Adds unit tests covering populateIntent and approveErc20Token for Ethereum, Polygon, and BSC.

Reviewed Changes

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

Show a summary per file
File Description
modules/sdk-core/src/bitgo/wallet/wallet.ts Added tokenApproval case in the prebuild transaction switch.
modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts Extended PopulatedIntent interface with optional tokenName.
modules/sdk-core/src/bitgo/utils/mpcUtils.ts Supported tokenApproval in populateIntent and included tokenName.
modules/abstract-eth/src/abstractEthLikeNewCoins.ts Allowed tokenApproval in missing-params validation logic.
modules/bitgo/test/v2/unit/wallet.ts Added tests for populateIntent and the approveErc20Token method.
Comments suppressed due to low confidence (3)

modules/sdk-core/src/bitgo/wallet/wallet.ts:3358

  • Consider adding a unit test to verify that prebuildTxWithIntent is called with the correct intentType and tokenName for the tokenApproval case, ensuring this branch remains covered.
      case 'tokenApproval':

modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts:259

  • [nitpick] The new tokenName field should be documented in the interface JSDoc to explain its purpose and any constraints (e.g., valid tokens, format).
  tokenName?: string;

modules/bitgo/test/v2/unit/wallet.ts:5096

  • This test checks for a generic rejection but doesn't assert the specific error message. To strengthen validation, specify the expected error message when required parameters are missing.
      await ethWallet.approveErc20Token('', tokenName).should.be.rejectedWith();

case 'tokenApproval':
return {
...baseIntent,
tokenName: params.tokenName,
Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The tokenApproval branch re-adds tokenName to the returned intent even though it's already included in baseIntent. Consider merging feeOptions and tokenName into baseIntent or extracting common fields to reduce duplication.

Suggested change
tokenName: params.tokenName,

Copilot uses AI. Check for mistakes.
!(
txParams.prebuildTx?.consolidateId ||
(txParams.type && ['acceleration', 'fillNonce', 'transferToken'].includes(txParams.type))
(txParams.type && ['acceleration', 'fillNonce', 'transferToken', 'tokenApproval'].includes(txParams.type))
Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The list of supported transaction types is hard-coded in multiple places. Extract this array into a shared constant to ensure consistency and simplify future updates.

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

Copilot uses AI. Check for mistakes.
@kamleshmugdiya kamleshmugdiya merged commit 5f5a391 into master Jul 11, 2025
12 checks passed
@kamleshmugdiya kamleshmugdiya self-assigned this Jul 24, 2025
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.

3 participants