feat(money-account-utils): add money account transaction batch builders - #9680
Merged
Conversation
Jwhiles
force-pushed
the
feat/money-account-utils-package
branch
3 times, most recently
from
July 30, 2026 12:51
bc4b92b to
d90583a
Compare
Jwhiles
force-pushed
the
feat/add-hooks
branch
2 times, most recently
from
July 31, 2026 09:39
061c619 to
a42e8d0
Compare
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
ffmcgee725
reviewed
Jul 31, 2026
Comment on lines
+458
to
+480
| const shareAmount = | ||
| amount === 0n | ||
| ? 0n | ||
| : getSharesForWithdrawal( | ||
| amount, | ||
| await getVaultRate({ accountantAddress, provider }), | ||
| ); | ||
| // Allow 1-unit slippage on minimumAssets as defense-in-depth against | ||
| // rounding: the contract's mulDivDown can truncate assetsOut by up to | ||
| // 1 unit relative to the requested amount. This tolerance is safe | ||
| // because ceiling division in getSharesForWithdrawal already guarantees | ||
| // assetsOut >= amount; the 1-unit slack here is a second line of | ||
| // defense, not a standalone fix. The subsequent ERC-20 transfer uses | ||
| // the original `amount`, so the tolerance does not affect how much the | ||
| // user receives — it only prevents a spurious revert from the teller's | ||
| // MinimumAssetsNotMet check. | ||
| const minimumAssets = amount > 0n ? amount - 1n : 0n; | ||
| const withdrawData = buildWithdrawData( | ||
| musdAddress, | ||
| shareAmount, | ||
| minimumAssets, | ||
| moneyAccountAddress, | ||
| ); |
Member
There was a problem hiding this comment.
Zero-amount withdraw placeholders encode shareAmount = 0, which Veda Teller rejects with ZeroShares.
Unlike deposit placeholders (which use a dedicated builder that omits calldata), withdraw placeholders may fail during simulation or submission before re-encoding.
Would it be worth adding a buildMoneyAccountWithdrawPlaceholderBatch, mirroring the deposit placeholder builder, to solve this ?
Returning something like
withdrawTx: teller target/type, no data,
transferTx: mUSD target/type, no data,
No amount, provider, accountant, recipient, or money-account address required
Then make buildMoneyAccountWithdrawBatch reject amount === 0n, so it cannot produce known-reverting calldata.
Contributor
Author
There was a problem hiding this comment.
Yeah, that's a good idea - have added it and will re-integrate into my mobile PR
Jwhiles
added a commit
that referenced
this pull request
Aug 3, 2026
…zero amounts Review feedback on #9680: a zero-amount withdraw encodes `withdraw(mUSD, 0, 0, moneyAccount)`, which the teller rejects for redeeming no shares. Unlike the deposit path — which got a dedicated placeholder builder that omits calldata — the withdraw path had no way to express "targets resolved, no amount yet" other than the `amount === 0n` sentinel. Adds `buildMoneyAccountWithdrawPlaceholderBatch({ chainId, tellerAddress })`, mirroring the deposit placeholder: synchronous, no provider, no accountant address, no recipient, no money account address. Both encoding builders now throw on a zero amount, before any vault read, so neither can produce calldata that is valid and submittable but cannot succeed. That removes the sentinel, the two dead parameters it implied, and the zero-amount branches in both builders. Note on the reported symptom: this does not change whether a placeholder batch simulates cleanly. `generateEIP7702BatchTransaction` maps absent calldata to `'0x'`, so the deposit placeholder encodes empty calls to the token and teller, and with an atomic batch those revert too. The value here is that the illegal state becomes unrepresentable, not that a simulation starts passing. Consumers must guard a zero amount before calling the encoding builders — for a cleared amount field there is nothing to re-encode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@metamaskbot publish-preview |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
ffmcgee725
previously approved these changes
Aug 3, 2026
Ports the Money Account deposit and withdrawal batch builders from metamask-mobile (`app/components/UI/Money/utils/moneyAccountTransactions.ts`) so both clients encode the vault calls identically. The client-resident wrappers around these builders stay in mobile: they read vault config, provider and recipient from Redux/Engine singletons, which do not belong in a shared package. Everything below that — ABIs, calldata encoding, slippage and share arithmetic, and the two `previewDeposit`/`getRate` reads — moves here, keeping the existing signatures so call sites only change their import path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The description predated the rescope: activity parsing and classification were never extracted, and the vault transaction builders now are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address review feedback on the ported vault builders: - Replace the `initialiseWithoutData` flag with a dedicated `buildMoneyAccountDepositPlaceholderBatch`. The flag path still awaited `previewDeposit` and discarded the result, and required five vault addresses it never used; the new builder is synchronous, takes only `chainId` and `tellerAddress`, and performs no vault reads. - Make `MoneyAccountTxParams.params.data` required, with the no-calldata case as `MoneyAccountPlaceholderTxParams`, so callers of the encoding builders no longer have to narrow an optional field. - Return `CaipAssetType | undefined` from `getMoneyAccountDepositAssetId` rather than silently defaulting to Monad, so an unsupported chain stays distinguishable. Clients apply their own default at the call site. - Type `MUSD_TOKEN_ASSET_ID_BY_CHAIN` values as `CaipAssetType` and the deposit options' addresses as `Hex`, removing the internal casts. - Fix withdraw comments describing the redeemed asset as USDC; the builders encode mUSD throughout. Also reconcile the changelog with the released 1.0.0 of the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…zero amounts Review feedback on #9680: a zero-amount withdraw encodes `withdraw(mUSD, 0, 0, moneyAccount)`, which the teller rejects for redeeming no shares. Unlike the deposit path — which got a dedicated placeholder builder that omits calldata — the withdraw path had no way to express "targets resolved, no amount yet" other than the `amount === 0n` sentinel. Adds `buildMoneyAccountWithdrawPlaceholderBatch({ chainId, tellerAddress })`, mirroring the deposit placeholder: synchronous, no provider, no accountant address, no recipient, no money account address. Both encoding builders now throw on a zero amount, before any vault read, so neither can produce calldata that is valid and submittable but cannot succeed. That removes the sentinel, the two dead parameters it implied, and the zero-amount branches in both builders. Note on the reported symptom: this does not change whether a placeholder batch simulates cleanly. `generateEIP7702BatchTransaction` maps absent calldata to `'0x'`, so the deposit placeholder encodes empty calls to the token and teller, and with an atomic batch those revert too. The value here is that the illegal state becomes unrepresentable, not that a simulation starts passing. Consumers must guard a zero amount before calling the encoding builders — for a cleared amount field there is nothing to re-encode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ffmcgee725
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Ports the Money Account deposit and withdrawal transaction batch builders from metamask-mobile (
app/components/UI/Money/utils/moneyAccountTransactions.ts) into this package, so mobile and extension encode the vault calls identically rather than each keeping its own copy.What moved: the ABIs, the calldata encoders, the slippage and share arithmetic, the mUSD deposit-asset resolution, and the two contract reads (
previewDepositon the lens,getRateon the accountant).What deliberately did not move: the four client wrappers (
updateMoneyAccountDepositTokenAmount,updateMoneyAccountWithdrawTokenAmount,getMoneyAccountDepositTransactionsData,getMoneyAccountWithdrawTransactionsData). Those read vault config, provider and recipient from mobile's Redux/Engine singletons and return a Confirmations-owned type, so they stay in the client and delegate to these builders.Signatures are unchanged from mobile, including the injected
provider, so client call sites only change their import path.@ethersproject/{abi,abstract-provider,contracts}v5 is used directly rather than theethersumbrella, matching the convention already established bymoney-account-balance-service(which reads the same lens and accountant contracts).References
N/A
Checklist
Note
Medium Risk
Changes on-chain calldata and share/slippage math for user fund movements; logic is well-tested (including the prior floor-division withdrawal bug) but mistakes could cause failed or mis-sized vault txs.
Overview
Ports Money Account vault deposit/withdraw batch encoding from MetaMask Mobile into
@metamask/money-account-utils, so clients share one implementation for approve+deposit and withdraw+transfer flows.Deposit:
buildMoneyAccountDepositBatchreadspreviewDepositon the lens, setsminimumMintvia 0.2% slippage (applySlippage), and returns typed approve + teller deposit txs. Withdraw:buildMoneyAccountWithdrawBatchreadsgetRate, converts assets to shares with ceiling division (getSharesForWithdrawal) to avoidMinimumAssetsNotMetreverts, then encodes teller withdraw (to the money account) plus mUSDtransferto the recipient. Both full builders reject zero amounts and require an@ethersprojectProvider.Adds placeholder variants (
buildMoneyAccountDepositPlaceholderBatch/buildMoneyAccountWithdrawPlaceholderBatch) that only resolveto,value, andTransactionType(no calldata, no RPC). Also exportsgetMoneyAccountDepositAssetId/getMoneyAccountDepositAssetAddress,TELLER_ABI, and related types; typesMUSD_TOKEN_ASSET_ID_BY_CHAINasCaipAssetType; adds@ethersproject/*deps and a large Jest suite.Reviewed by Cursor Bugbot for commit 832c497. Bugbot is set up for automated code reviews on this repo. Configure here.