-
-
Notifications
You must be signed in to change notification settings - Fork 230
feat: Default addTransactionBatch to EIP7702 if supported, otherwise use sequential batch #5853
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
b46ad98
to
6f74606
Compare
6f74606
to
37d55f9
Compare
packages/transaction-controller/src/hooks/ExtraTransactionsPublishHook.ts
Show resolved
Hide resolved
8808f3d
to
96e2cdd
Compare
…e use sequential batch
96e2cdd
to
cb180c9
Compare
useHook: true, | ||
disable7702: true, | ||
disableHook: false, | ||
disableSequential: false, |
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 also has be true
as we only want to use smart transactions here, to trigger gas station, and ensure it's atomic.
* Defaults to false. | ||
*/ | ||
useHook?: boolean; | ||
/** Whether or not to disable batch transaction processing through an EIP 7702 upgraded account. */ |
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.
/** Whether or not to disable batch transaction processing through an EIP 7702 upgraded account. */ | |
/** Whether to disable batch transaction processing via an EIP-7702 upgraded account. */ |
/** Whether or not to disable batch transaction processing through an EIP 7702 upgraded account. */ | ||
disable7702?: boolean; | ||
|
||
/** Whether or not to disable batch transaction processing through a hook. */ |
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.
/** Whether or not to disable batch transaction processing through a hook. */ | |
/** Whether to disable batch transaction via the `publishBatch` hook. */ |
/** Whether or not to disable batch transaction processing through a hook. */ | ||
disableHook?: boolean; | ||
|
||
/** Whether or not to disable batch transaction through sequential batching. */ |
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.
/** Whether or not to disable batch transaction through sequential batching. */ | |
/** Whether to disable batch transaction via sequential transactions. */ |
if (useHook) { | ||
return await addTransactionBatchWithHook(request); | ||
if (!transactionBatchRequest.disable7702) { | ||
return await addTransactionBatchWith7702(request); |
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.
Does this not mean that if 7702 is enabled, but the chain isn't supported, it will throw and not fallback to anything else?
Do we need to use a try catch and only handle the specific chain not supported error?
(!transactionBatchRequest.disableSequential && | ||
sequentialPublishBatchHook.getHook()); | ||
if (!publishBatchHook) { | ||
log(`Can't process 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.
log(`Can't process batch`, { | |
log(`No supported batch methods found`, { |
sequentialPublishBatchHook.getHook()); | ||
if (!publishBatchHook) { | ||
log(`Can't process batch`, { | ||
disable7702: transactionBatchRequest.disable7702, |
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, could we destructure these above?
|
||
const chainId = getChainId(networkClientId); | ||
const batchId = generateBatchId(); | ||
const transactionCount = nestedTransactions.length; | ||
const collectHook = new CollectPublishHook(transactionCount); | ||
try { | ||
if (requireApproval && useHook) { | ||
if (requireApproval) { |
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 spot, if we've made it here, we're not doing the 7702 flow.
e9419d7
to
4748992
Compare
Co-authored-by: OGPoyraz <[email protected]>
Explanation
We previously relied on the
useHook
property in the request. Instead, the deciding factor is only whether or not EIP 7702 is supported on the chain of the batch transaction.References
Fixes https://github.com/MetaMask/MetaMask-planning/issues/4991
Changelog
Checklist