Skip to content

Extends GasFeePoller to update gas properties for unapproved transaction batches #5950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 71 commits into
base: main
Choose a base branch
from

Conversation

vinistevam
Copy link
Contributor

@vinistevam vinistevam commented Jun 10, 2025

Explanation

This PR extends the GasFeePoller to track/update gas properties for unapproved transactionBatches.

Changes

The GasFeePoller now:

  • Detects unapproved transactionBatches via #getUnapprovedTransactionBatches.
  • Fetches gas estimates using DefaultGasFeeFlow.
  • Emits transaction-batch-updated events with updated gasFeeEstimates.

Updates are applied to the TransactionBatchMeta for each batch, mirroring the behaviour already in place for single transactions.
This enhancement ensures that unapproved transaction batches receive timely gas updates similar to individual transactions, improving consistency across transaction types.

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/5090

Changelog

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, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

vinistevam and others added 30 commits May 13, 2025 17:18
Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
…:MetaMask/core into feat/add-batch-transaction-approval-type
@vinistevam vinistevam marked this pull request as ready for review June 10, 2025 12:58
@vinistevam vinistevam requested review from a team as code owners June 10, 2025 12:58
Base automatically changed from feat/add-estimate-gas-fee-batch-tx to main June 11, 2025 13:25
transactionMeta: {
...txBatchMeta,
txParams: {
...txBatchMeta.transactions?.[0],
Copy link
Member

@OGPoyraz OGPoyraz Jun 21, 2025

Choose a reason for hiding this comment

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

I maybe missing the context - why we are estimating only the first transaction?

Shouldn't we estimate all nested transactions and return all gas estimates and then UI calculates them all and show as final network fee estimate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, for now, we ultimately need the gasFee on the batch to render in the confirmation. That gives us two alternatives for MVP estimate for the first transaction or estimate all transactions (it can be X txs) and pick the highest one. However, this can be tricky for Linea, for example, their RPC will fail if a transaction requires a previous transaction, like a token approval.

In the end, for the batch, we are using the simplest case, only getting a network level estimate (via the DefaultGasFeeFlow).

@vinistevam vinistevam requested a review from matthewwalsh0 June 24, 2025 06:32
OGPoyraz
OGPoyraz previously approved these changes Jun 24, 2025
matthewwalsh0
matthewwalsh0 previously approved these changes Jun 24, 2025
}) {
this.#updateTransactionBatch(transactionBatchId, (batch) => {
batch.gasFeeEstimates = gasFeeEstimates;
return batch;
Copy link
Member

Choose a reason for hiding this comment

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

Minor, you only need to do or the other, either update the draft directly, or return a new object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed to use only one approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants