feat: add hook for updating MM pay transaction amount update for money account deposits#29561
Conversation
…y account deposits
|
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29561 +/- ##
===========================================
- Coverage 82.15% 61.84% -20.32%
===========================================
Files 5178 5185 +7
Lines 137450 137845 +395
Branches 31079 31194 +115
===========================================
- Hits 112924 85248 -27676
- Misses 16875 45938 +29063
+ Partials 7651 6659 -992 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (isMoneyAccountDeposit) { | ||
| updateCustomTokenAmount(amountHuman); | ||
| } else { | ||
| updateTokenAmountCallback(amountHuman); |
There was a problem hiding this comment.
Can we encapsulate this further and only call useUpdateTokenAmount inside useUpdateCustomTokenAmount if there is no type specific condition?
There was a problem hiding this comment.
PR is updated to address this.
| import { updateMoneyAccountDepositTokenAmount } from '../../../../UI/Money/utils/moneyAccountTransactions'; | ||
| import Logger from '../../../../../util/Logger'; | ||
|
|
||
| export function useUpdateCustomTokenAmount() { |
There was a problem hiding this comment.
Maybe useUpdateTransactionPayAmount and we could put it in the pay directory?
There was a problem hiding this comment.
Suggestion is implemented.
| return; | ||
| } | ||
|
|
||
| const updates = updateMoneyAccountDepositTokenAmount( |
There was a problem hiding this comment.
We'd need a money account type check here, so we can scale it with additional types in future, and fallback to the default useUpdateTokenAmount?
There was a problem hiding this comment.
PR is updated to address this.
| } | ||
|
|
||
| const updates = updateMoneyAccountDepositTokenAmount( | ||
| transactionMeta, |
There was a problem hiding this comment.
Nice, give them the full context to extract what they need 👍
| }; | ||
| } | ||
|
|
||
| export interface MoneyAccountDepositTokenAmountUpdate { |
There was a problem hiding this comment.
Should this be defined within our new hook as something like UpdateAmountCall?
There was a problem hiding this comment.
PR is updated to make this change
There was a problem hiding this comment.
I moved it to app/components/Views/confirmations/types/transactions.ts to avoid circular dependency issue.
| ); | ||
|
|
||
| for (const { nestedTransactionIndex, transactionData } of updates) { | ||
| updateAtomicBatchData({ |
There was a problem hiding this comment.
I've hit this before also, but ideally we'd have a bulkified version of this to update multiple calls at once for cleaner state updates, but not needed now.
Could also support calling updateEditableParams from here with the full data but only if needed in future.
| amountHuman, | ||
| ); | ||
|
|
||
| for (const { nestedTransactionIndex, transactionData } of updates) { |
There was a problem hiding this comment.
Minor, we could move this out of the money account specific callback to minimise duplication if we have other types in future.
…metamask-mobile into custom_amt_update_hook
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d22c539. Configure here.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: The changes in this PR are focused on the Money Account deposit/withdraw confirmation flow, specifically:
Tags selected:
The stub implementations return empty arrays so Money Account-specific behavior is unchanged functionally, but the routing logic and async error handling in Performance Test Selection: |
|
…
Description
Add code to update amount for money account deposit. The function for money account data updates is placeholder only.
Changelog
CHANGELOG entry:
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
NA
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
Medium Risk
Touches the confirmation custom-amount flow and transaction data mutation paths; while Money Account updaters are stubs, the new routing and async error-handling could affect how/when amounts are applied and errors surfaced.
Overview
Adds Money Account-aware amount updating for confirmations. Introduces
useUpdateTransactionPayAmount, which routes amount changes: formoneyAccountDeposit/moneyAccountWithdrawit prepares per-nested-call updates and applies them viaupdateAtomicBatchData, otherwise it falls back to the existinguseUpdateTokenAmountpath.Wires custom amount entry into the new updater and hardens Done handling.
useTransactionCustomAmountnow delegatesupdateTokenAmounttouseUpdateTransactionPayAmount, andCustomAmountInfo’s Done handler is made async withtry/catch/finallyto always flush UI state and log failures.Scaffolds Money Account re-encoding. Adds placeholder
updateMoneyAccountDepositTokenAmount/updateMoneyAccountWithdrawTokenAmount(returning[]) plus a sharedUpdateTransactionPayAmountCalltype and accompanying tests.Reviewed by Cursor Bugbot for commit 12f2ee6. Bugbot is set up for automated code reviews on this repo. Configure here.