Skip to content

fix(transaction-pay-controller): use exact input for Relay deposits - #9954

Open
pedronfigueiredo wants to merge 9 commits into
mainfrom
pnf/conf-1782-relay-exact-input-deposits
Open

fix(transaction-pay-controller): use exact input for Relay deposits#9954
pedronfigueiredo wants to merge 9 commits into
mainfrom
pnf/conf-1782-relay-exact-input-deposits

Conversation

@pedronfigueiredo

@pedronfigueiredo pedronfigueiredo commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Explanation

Relay quote behavior is now determined after transaction processing rather than by transaction-type allowlists.

This change:

  • uses EXACT_INPUT with sourceTokenAmount when a Relay quote has no embedded txs, so the entered source amount remains the total amount paid;
  • preserves EXACT_OUTPUT with the target amount for quotes that embed transactions, including Money Account calls;
  • assigns the final Relay tradeType and amount once after transaction processing;
  • adds the optional, client-facing TransactionPayQuote.isInputBased and TransactionPayTotals.isInputBased flags;
  • has Relay, Across, Server, Fiat, and no-op strategies expose normalized input-based semantics without requiring totals code to inspect provider-specific quote internals;
  • marks totals as input-based only when every selected quote is input-based, preventing source-side fees from being counted twice; and
  • clamps negative Relay dust to zero when exact-input output is below the prior target minimum.

The Relay trade-type decision no longer depends on Perps or Predict transaction types. HyperCore requests without embedded transactions follow the same exact-input rule.

Consumer integration

The coordinated client PRs remain drafts until the Core package containing this change is published:

  • MetaMask Mobile #35249 consumes TransactionPayTotals.isInputBased, keeps input-based source amounts stable, and displays the authoritative target amount. It now wires both the v26 resolveSourceAmount callback and v27 getBalance callback through a shared resolver; final integration still requires aligning @metamask/sentinel-api-service and regenerating the lockfile.
  • MetaMask Extension #45753 consumes the same aggregate flag. After the Core release it must bump the package, remove its compatibility type, regenerate dependency artifacts including LavaMoat policies and attributions, and validate MV3 and MV2 builds.

Both clients treat only an explicit isInputBased: true as input-based. Mixed aggregates and embedded-transaction/output-based quotes retain the existing Total presentation. Input-based withdrawals use the controller's authoritative target amount; legacy or output-based withdrawals retain the existing fee-subtraction fallback.

Local absolute-path file: links in the client worktrees are integration-only and are not part of either client PR.

Validation

  • yarn workspace @metamask/transaction-pay-controller run test
    • 100% statement, line, and function coverage; 99.49% branch coverage
  • yarn workspace @metamask/transaction-pay-controller run jest --no-coverage src/utils/totals.test.ts src/strategy/relay/relay-quotes.test.ts src/strategy/across/across-quotes.test.ts src/strategy/server/server-quotes.test.ts src/strategy/fiat/fiat-direct-musd.test.ts src/utils/no-op-quote.test.ts
    • 6 suites and 288 tests passed
  • yarn workspace @metamask/transaction-pay-controller run build
  • yarn workspace @metamask/transaction-pay-controller run changelog:validate
  • Targeted ESLint and oxfmt --check for changed TypeScript files
  • git diff --check origin/main...HEAD

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Changes quote trade types, amounts, and confirmation totals for MetaMask Pay flows (including HyperCore deposits without embedded txs), with coordinated mobile/extension consumer PRs.

Overview
Relay quoting now picks tradeType and amount after transaction embedding instead of from transaction-type rules (including the removed HyperCore EXACT_OUTPUT path). Quotes without embedded txs use EXACT_INPUT and sourceTokenAmount; quotes with embedded calls (delegation, Money Account post-quote, etc.) stay EXACT_OUTPUT with the target or override amount.

The PR adds optional TransactionPayQuote.isInputBased and TransactionPayTotals.isInputBased, set across Relay, Across, Server, Fiat, and no-op normalization. calculateTotals no longer keys off isMaxAmount; it treats totals as input-based only when every selected quote is input-based, uses that for the paid amount in totals, and avoids double-counting fees already baked into the source side. Relay dust is clamped to zero when exact-input output falls below the prior minimum.

Embedded-transaction handlers no longer mutate tradeType / amount on the draft body; processMoneyAccountPostQuote returns the raw amount for the final request instead.

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

@matthewwalsh0
matthewwalsh0 self-requested a review August 26, 2026 13:39
@pedronfigueiredo
pedronfigueiredo force-pushed the pnf/conf-1782-relay-exact-input-deposits branch from eea585b to 63f9f32 Compare August 26, 2026 13:52
// start as EXPECTED_OUTPUT until transaction processing determines whether
// the request embeds transactions.
const useExactInput =
(isMaxAmount ?? false) || (request.isPostQuote ?? false);

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.

Minor, could also just Boolean(isMaxAmount || request.isPostQuote) if more readable?

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.

Agreed, Boolean(...) is clearer here. I’ll simplify it.

Addressed in 5f0d8b805e (commit).

recipient: effectiveRequest.recipient ?? from,
slippageTolerance,
tradeType: getTradeType(useExactInput, useExactOutput),
tradeType: useExactInput ? 'EXACT_INPUT' : 'EXPECTED_OUTPUT',

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.

Minor, was the getTradeType function useful if we pass it the request and isMaxAmount it owns the full decision.

}

if (!body.txs?.length) {
body.tradeType = 'EXACT_INPUT';

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.

Minor, is it simpler to assign tradeType once after we've set the transactions?

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’ll derive tradeType once from whether transactions were added after processing, instead of setting and overriding it in multiple places.

Addressed in b32aea78f3 (commit).

recipient: effectiveRequest.recipient ?? from,
slippageTolerance,
tradeType: getTradeType(useExactInput, useExactOutput),
tradeType: useExactInput ? 'EXACT_INPUT' : 'EXPECTED_OUTPUT',

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.

This EXPECTED_OUTPUT branch is never hit as we override it regardless if transactions or not?


if (!body.txs?.length) {
body.tradeType = 'EXACT_INPUT';
body.amount = sourceTokenAmount;

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.

Also here, is it more readable to just assign amount once after transactions are processed?

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.

Agreed. I’ll defer amount too and set it once after transaction processing, preserving the Money Account transaction amount when present.

Addressed in d0eb8912b4 (commit).

/** Total fees for the target transaction and all quotes. */
fees: TransactionPayFees;

/** Whether all selected quotes are driven by the source input amount. */

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.

Is this a bit ambiguous?

Maybe Whether the quote(s) subtract fees from the destination amount meaning the input amount is static

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.

Agreed—the fee behavior is more concrete. I’ll update the documentation to clarify that fees are subtracted from the destination amount while the input amount stays static.

Addressed in c848146ce2 (commit).

isMaxAmount: boolean,
): boolean {
if (quote.strategy === TransactionPayStrategy.Relay) {
const relayQuote = quote.original as RelayQuote;

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.

This breaks the abstraction provided by the TransactionPayStrategy and couples this code to the internals of each one.

The purpose of the TransactionPayQuote type is to abstract what a quote can return, so it would also need a isInputBased property?

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.

Agreed. I’ll expose isInputBased on TransactionPayQuote, have each strategy populate it, and make totals consume only the normalized property.

Addressed in ccb7854521 (commit).

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.

2 participants