-
-
Notifications
You must be signed in to change notification settings - Fork 239
Adds estimate gas limit for transaction batch using simulation API #5852
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
Conversation
…a publish batch hook
Co-authored-by: Matthew Walsh <[email protected]>
…:MetaMask/core into feat/add-batch-transaction-approval-type
…-estimate-gas-batch-transaction
…-estimate-gas-batch-transaction
…-estimate-gas-batch-transaction
8154080
to
097124d
Compare
const response = await simulateTransactions(chainId, { | ||
transactions: transactions.map((transaction) => ({ | ||
...transaction.params, | ||
from, | ||
})), | ||
}); |
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.
What happens if simulateTransactions
throws SimulationError
, do we not want to handle that case?
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.
If anything other than success happens, it will throw, and I could also handle it and log the simulation error.
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.
The best we could do is throw a cleaner error such as Cannot estimate transaction batch total gas as simulation failed
?
Co-authored-by: OGPoyraz <[email protected]>
); | ||
|
||
return { | ||
transactions: transactionsWithGasLimit, |
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.
What I meant was can we simplify this function and only return the gasLimit
as that's all we need currently?
const response = await simulateTransactions(chainId, { | ||
transactions: transactions.map((transaction) => ({ | ||
...transaction.params, | ||
from, | ||
})), | ||
}); |
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.
The best we could do is throw a cleaner error such as Cannot estimate transaction batch total gas as simulation failed
?
update: UpdateStateCallback; | ||
}): Promise<TransactionBatchMeta> { | ||
if (!isSimulationEnabled()) { | ||
throw new Error('Simulation is not enabled'); |
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.
throw new Error('Simulation is not enabled'); | |
throw new Error('Cannot create transaction batch as simulation not supported'); |
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.
This is something we'll need to consider in any client features that use a transaction batch, as we currently require simulation.
In future we could skip simulation if gas limits are provided on each transaction, or we could just estimate as we go normally and display no gas fee.
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
## [Unreleased] | |||
|
|||
### Added | |||
|
|||
- Add `gas` property to `TransactionBatchMeta`, populated using simulation API ([#5852](https://github.com/MetaMask/core/pull/5852)) |
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.
My fault, but could we also move the entry on line 33 to unreleased please, missed it in my previous PR.
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 think that entry is already in the version 56.3.0
, otherwise, I can change it in my next PR.
Explanation
This PR adds
simulateGasBatch
support to the Transaction Controller, enabling sequential gas estimation fornestedTransactions
within a batch. When bothuseHook
andrequireApproval
aretrue
, the method simulates each transaction, calculates the total and individual gas limits, and stores the result in state for further use in the UI or execution logic.Changes
Transaction Controller
simulateGasBatch
to estimate gas for each transaction in a batch sequentially.simulateGasBatch
into theaddTransactionBatch
flow, triggered whenuseHook
andrequireApproval
aretrue
.References
Changelog
Checklist