-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
base: main
Are you sure you want to change the base?
Conversation
…a publish batch hook
Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
…:MetaMask/core into feat/add-batch-transaction-approval-type
…-estimate-gas-batch-transaction
…-estimate-gas-batch-transaction
…-estimate-gas-batch-transaction
transactionMeta: { | ||
...txBatchMeta, | ||
txParams: { | ||
...txBatchMeta.transactions?.[0], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
}) { | ||
this.#updateTransactionBatch(transactionBatchId, (batch) => { | ||
batch.gasFeeEstimates = gasFeeEstimates; | ||
return batch; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
9f29b3d
Explanation
This PR extends the
GasFeePoller
to track/update gas properties for unapprovedtransactionBatches
.Changes
The GasFeePoller now:
transactionBatches
via#getUnapprovedTransactionBatches
.DefaultGasFeeFlow
.transaction-batch-updated
events with updatedgasFeeEstimates
.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