fix: update required assets correctly for money account deposits#29806
Conversation
jpuri
added
team-confirmations
|
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
Bot
added
pr-not-ready-for-e2e
|
jpuri
removed
the
pr-not-ready-for-e2e
| return { updateTransactionPayAmount }; | ||
| } | ||
|
|
||
| function syncMoneyAccountDepositRequiredAssets( |
There was a problem hiding this comment.
Minor, this is a more general updateRequiredAssetsAmount for any type to use if needed?
| ...transactionMeta, | ||
| requiredAssets: [{ ...existing[0], amount }, ...existing.slice(1)], | ||
| }, | ||
| 'Money Account deposit: sync requiredAssets amount', |
There was a problem hiding this comment.
Minor, Update requiredAssets amount as can be generic?
| }, | ||
| 'Money Account deposit: sync requiredAssets amount', | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
Minor, not sure how this would throw as it's a synchronous state update.
| } catch (error) { | ||
| Logger.error( | ||
| error as Error, | ||
| 'Failed to sync Money Account deposit requiredAssets amount', |
There was a problem hiding this comment.
Minor, also can be generic.
| ); | ||
| }); | ||
|
|
||
| describe('syncMoneyAccountDepositRequiredAssets', () => { |
There was a problem hiding this comment.
Minor, this isn't an exported function, so should be something like updates requires assets amount?
| return nestedCall.to; | ||
| } | ||
|
|
||
| const requiredAssetAddress = transactionMeta?.requiredAssets?.[0]?.address; |
There was a problem hiding this comment.
Minor, if we have requiredAssets set, that should probably take priority over a found token transfer call above?
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 5150e0f. Configure here.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
Tag selection rationale:
Performance Test Selection: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29806 +/- ##
=======================================
Coverage 81.50% 81.51%
=======================================
Files 5328 5328
Lines 141146 141158 +12
Branches 32157 32160 +3
=======================================
+ Hits 115046 115061 +15
+ Misses 18239 18236 -3
Partials 7861 7861 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
metamaskbotv2
Bot
added
the
release-7.78.0
4 participants
Description
Fix money account deposits by setting correst required asset in transaction.
Changelog
CHANGELOG entry:
Related issues
Ref: https://consensyssoftware.atlassian.net/browse/CONF-1324
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 transaction amount update flow for
moneyAccountDeposit, including writingrequiredAssetsviaupdateTransaction, which could affect what token/amount is used for deposits if decimals or asset ordering are wrong.Overview
Fixes Money Account deposit pay-amount updates by syncing
transactionMeta.requiredAssets[0].amountto the user-entered amount (converted to atomic units using required-token decimals, rounded up, and hex-encoded) before applying nested transaction updates.Also updates
getTokenAddressto preferrequiredAssets[0].addresswhen no ERC-20 transfer call is detected, and adds/expands unit tests covering the new required-asset sync and address selection behavior.Reviewed by Cursor Bugbot for commit c14d618. Bugbot is set up for automated code reviews on this repo. Configure here.