feat: add sequential batch support #5762
Conversation
vinistevam
force-pushed
the
feat/add-sequential-batch-hook
branch
from
0e732db to
8b9e6f7
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -332,7 +337,8 @@ async function getNestedTransactionMeta( | |||
| async function addTransactionBatchWithHook( | |||
| request: AddTransactionBatchRequest, | |||
| ): Promise<TransactionBatchResult> { | |||
| const { publishBatchHook, request: userRequest } = request; | |||
| const { publishBatchHook: initialPublishBatchHook, request: userRequest } = | |||
There was a problem hiding this comment.
Minor, would this be more explicit as requestPublishBatchHook?
There was a problem hiding this comment.
Applied suggestion.
| TransactionReceipt, | ||
| } from '../types'; | ||
|
|
||
| const TRANSACTION_CHECK_INTERVAL = 5000; // 5 seconds |
There was a problem hiding this comment.
Ideally this would also use the dynamic intervals from LaunchDarkly, so maybe it is cleaner to have a callback such as getPendingTransactionTrackerByChainId and then ask a tracker instance directly to poll our transaction?
There was a problem hiding this comment.
Agree. Passed a callback to our hook to give the responsibility to track the transaction.
|
|
||
| const log = createModuleLogger(projectLogger, 'sequential-publish-batch-hook'); | ||
|
|
||
| type SequentialPublishBatchHookParams = { |
There was a problem hiding this comment.
Minor, maybe SequentialPublishBatchHookOptions?
| * @returns The hook function. | ||
| */ | ||
| getHook() { | ||
| return async ({ |
There was a problem hiding this comment.
Rather than a function returning another function, should we use the pattern of having a normal #hook method that we bind here to keep it as readable as possible?
There was a problem hiding this comment.
Applied the pattern used in the rest of the hooks to bind the private hook.
| for (const transaction of transactions) { | ||
| try { | ||
| const transactionMeta = this.#getTransaction(String(transaction.id)); | ||
| const transactionHash = await this.#publishTransaction( |
There was a problem hiding this comment.
As the logic is so small here, I wonder if it's simpler for the hook to directly call sendRawTransaction via the EthQuery?
There was a problem hiding this comment.
I decided to use the #publishTransaction from txController because of the option to use #updateSubmitHistory if you don't see it as useful, I can call the sendRawTransaction as you suggested.
| ): Promise<boolean> { | ||
| let attempts = 0; | ||
|
|
||
| while (attempts < MAX_TRANSACTION_CHECK_ATTEMPTS) { |
There was a problem hiding this comment.
Is it out of our scope to define a timeout here, as it's so variable based on chain, and gas fees etc?
There was a problem hiding this comment.
Using the PendingTransactionTracker to handle it now.
| } | ||
|
|
||
| const waitPromise = new Promise((resolve) => | ||
| setTimeout(resolve, TRANSACTION_CHECK_INTERVAL), |
There was a problem hiding this comment.
Rather than iterating and waiting ourselves, could we add methods to the PendingTransactionTracker such as addTransaction to force a new ID into the queue? And then maybe emit an event we wait for here inside a new Promise?
The goal being to encapsulate as much as possible in the PendingTransactionTracker and just add a job to it from here.
|
|
||
| if (!isConfirmed) { | ||
| throw new Error( | ||
| `Transaction ${transactionHash} failed or was not confirmed.`, |
There was a problem hiding this comment.
Maybe Batch transaction failed - ${transactionHash} ?
Explanation
The
TransactionControllercurrently lacks support for sequential batch transactions, which are required for the stablecoin lending feature. Specifically, there is no mechanism to execute multiple transactions (e.g., approval + token deposit) sequentially while ensuring confirmation for each transaction. This limitation prevents efficient batch processing to support new features.What solution do these changes offer?
The
SequentialPublishBatchHookintroduces a default mechanism for handling batch transactions when no custompublishBatchHookis provided. It ensures transactions are published sequentially, waiting for confirmation before proceeding to the next. If any transaction fails to publish or confirm, the batch process halts and throws an error.Key Features:
References
Fixes https://github.com/MetaMask/MetaMask-planning/issues/4695
Changelog
Checklist