Skip to content

fix: money account withdraw update transaction config when recipient changes#28803

Closed
jpuri wants to merge 32 commits into
mainfrom
mmpay_recipient_update
Closed

fix: money account withdraw update transaction config when recipient changes#28803
jpuri wants to merge 32 commits into
mainfrom
mmpay_recipient_update

Conversation

@jpuri

@jpuri jpuri commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Description

For money account withdraw update transaction config when recipient changes.

Changelog

CHANGELOG entry:

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/CONF-1187

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Screen.Recording.2026-04-16.at.11.48.49.PM.mov
Screen.Recording.2026-04-16.at.11.39.13.PM.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Medium Risk
Touches money-account confirmation flows and pay-token selection/disablement logic; regressions could block users from selecting pay tokens or confirming transactions if accountOverride state isn’t set/propagated correctly.

Overview
Money Account confirmations now use a TransactionPayController-backed accountOverride instead of editing txParams directly. This introduces PayAccountSelector plus a new useTransactionAccountOverride hook/selector to read/write the chosen account via TransactionPayController.setTransactionConfig.

CustomAmountInfo gains supportAccountSelection (enabled for money-account deposit/withdraw) and disables PayTokenAmount and the confirm button until an override is chosen. PayWithRow is updated to respect a new useMoneyAccountPayToken hook: it blocks pay-token editing while awaiting account selection, shows a placeholder label (confirm.label.payment_method), and prioritizes a money-account display token/fallback for withdraw.

Also updates developer money-account test transactions (recipient/token address and transfer amount) and includes new/updated unit tests around the new hooks and components, plus a small AccountSelector padding tweak.

Reviewed by Cursor Bugbot for commit 32172a6. Bugbot is set up for automated code reviews on this repo. Configure here.

@jpuri jpuri requested a review from a team as a code owner April 14, 2026 11:22
@jpuri jpuri added team-confirmations Push issues to confirmations team no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed labels Apr 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions Bot added size-S risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 14, 2026
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 14, 2026
@jpuri jpuri enabled auto-merge April 14, 2026 11:44
Engine.context.TransactionPayController.setTransactionConfig(
transactionId,
(config) => {
config.refundTo = address as Hex;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I thought we already had a recipient property too but I was wrong.

Will need to add that to the pay controller so it doesn't always match the from.

@matthewwalsh0 matthewwalsh0 Apr 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But if this is just for the withdraw flow, then the from should become the selected / gas account who we want to be the recipient anyway? So maybe we don't need to change anything?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR is updated to use field accountOverride in config. I still need to work on core PR to add this field.

@@ -142,7 +142,12 @@ export const CustomAmountInfo: React.FC<CustomAmountInfoProps> = memo(
const handleRecipientAccountSelected = useCallback(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Outside the scope of this PR, but first time I'm seeing this code.

Could we modularise this a bit more and encapsulate this logic within the AccountSelector?

Or if you don't want to couple it to MetaMask Pay, we could wrap it and make a PayAccountSelector?

Just to make the logic a bit more readable and easier to test, plus stops this info component becoming a monolith.

I see we're only using the state for an alert, but if we're storing it on the pay controller, we could use a standard alert hook that reads the controller state?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point I updated PR to address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This point is not clear to me:

I see we're only using the state for an alert, but if we're storing it on the pay controller, we could use a standard alert hook that reads the controller state?

@github-actions github-actions Bot added size-M risk-medium Moderate testing recommended · Possible bug introduction risk and removed size-S risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 14, 2026
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 14, 2026
@jpuri jpuri requested a review from matthewwalsh0 April 14, 2026 13:29
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 16, 2026
@github-actions github-actions Bot removed the size-M label Apr 16, 2026
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 16, 2026
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 17, 2026
@github-actions github-actions Bot added risk-medium Moderate testing recommended · Possible bug introduction risk and removed risk-medium Moderate testing recommended · Possible bug introduction risk labels Apr 17, 2026
@jpuri jpuri requested review from a team as code owners April 21, 2026 12:20
@github-actions

Copy link
Copy Markdown
Contributor

E2E Fixture Validation — Schema is up to date
12 value mismatches detected (expected — fixture represents an existing user).
View details

@github-actions

Copy link
Copy Markdown
Contributor

AI PR Analysis

🚫 Merge safe: false | 🟠 Risk: high

Merge decision: AI analysis did not complete — manual review required before merging.

AI analysis did not complete. Manual review recommended.

View run

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeConfirmations, SmokeIdentity, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeTrade, SmokeWalletPlatform, SmokeCard, SmokePerps, SmokeRamps, SmokeMultiChainAPI, SmokePredictions, SmokeSeedlessOnboarding, SmokeBrowser, FlaskBuildTests
  • Selected Performance tags: @PerformanceAccountList, @PerformanceOnboarding, @PerformanceLogin, @PerformanceSwaps, @PerformanceLaunch, @PerformanceAssetLoading, @PerformancePredict, @PerformancePreps
  • Risk Level: high
  • AI Confidence: %
click to see 🤖 AI reasoning details

E2E Test Selection:
Fallback: AI analysis did not complete successfully. Running all tests.

Performance Test Selection:
Fallback: AI analysis did not complete successfully. Running all performance tests.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

DO-NOT-MERGE Pull requests that should not be merged no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed risk:high AI analysis: high risk risk-medium Moderate testing recommended · Possible bug introduction risk size-XL team-confirmations Push issues to confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants