-
-
Notifications
You must be signed in to change notification settings - Fork 228
Adds transactionBatches
into transaction controller state
#5793
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
c3f8a6b
to
49e9a89
Compare
31069c4
to
e62592d
Compare
@@ -169,6 +169,7 @@ export enum ApprovalType { | |||
WalletConnect = 'wallet_connect', | |||
WalletRequestPermissions = 'wallet_requestPermissions', | |||
WatchAsset = 'wallet_watchAsset', | |||
TransactionBatch = 'transaction_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, alphabetical?
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.
Done
@@ -2596,6 +2606,15 @@ export class TransactionController extends BaseController< | |||
}); | |||
} | |||
|
|||
#addBatchMetadata(transactionBatchMeta: TransactionBatchMeta) { |
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.
In the interest of modularity, could we provide the update
callback directly so the state is encapsulated within the batch
module?
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.
Removed #addBatchMetadata
as we agreed to not persist the transactionBatches
for now.
* @param transactions - The transactions to be applied to the state. | ||
* @returns The trimmed list of transactions. | ||
*/ | ||
#trimBatchTransactionsForState( |
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.
Can this also go in batch.ts
file for the sake for modularity?
Plus can we simplify this to just add at the end, and delete from the start once we exceed some limit?
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.
Removed #trimBatchTransactionsForState
, becasue we are not saving the transactionBatches
yet
@@ -490,6 +490,39 @@ export type TransactionMeta = { | |||
}; | |||
}; | |||
|
|||
export type TransactionBatchMeta = { |
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.
JSDoc for type itself?
id: string; | ||
|
||
/** | ||
* Data for any nested transactions. |
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.
Technically not "nested" as that is EIP-7702 only.
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.
Done
const batchId = generateBatchId(); | ||
|
||
if (requireApproval) { | ||
const txBatchMeta: TransactionBatchMeta = { |
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.
Assuming this will grow, should we create a newBatchMetadata
function to just return the 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.
Created newBatchMetadata
function.
async function requestApproval( | ||
txBatchMeta: TransactionBatchMeta, | ||
messenger: TransactionControllerMessenger, | ||
{ shouldShowRequest }: { shouldShowRequest: boolean }, |
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.
As we only have one usage, should we remove this property and hardcode below?
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.
Done
id, | ||
origin: origin || ORIGIN_METAMASK, | ||
requestData, | ||
expectsResult: true, |
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 we're setting expectsResult
to true
, it means the client will wait until we call one of the result callbacks.
Meaning we need to invoke success
or error
returned from this function once the batch is complete or failed.
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, should I save the resultCallbacks
and call success when tx is confirmed or error when tx fails, similar to the pattern of collectHook
?
Applied fixes.
@@ -490,6 +490,39 @@ export type TransactionMeta = { | |||
}; | |||
}; | |||
|
|||
export type TransactionBatchMeta = { |
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.
Should we export this in index.ts
?
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.
Done, type exported.
const batchId = generateBatchId(); | ||
|
||
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.
Should we also check useHook
is true
at this stage as EIP-7702 uses the standard confirmation?
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.
Done.
Co-authored-by: Matthew Walsh <[email protected]>
…:MetaMask/core into feat/add-batch-transaction-approval-type
transactionBatches
into transaction controller statetransactionBatches
into transaction controller state
Explanation
This PR introduces support for batch transactions in the Transaction Controller by adding a new
ApprovalType
and extending the state to handleTransactionBatches
. These changes enable enhanced metadata management for sequential batch flows, including UI updates for gas estimation and future automation capabilities.Changes
Controller Utils
ApprovalType
to the enum:TransactionBatch
, to support batch transactions.Transaction Controller
transactionBatches
, to store metadata for transaction batches.#addBatchMetadata
, responsible for populating batch-specific metadata.TransactionBatchMeta
, to manage metadata for transaction batches.#trimBatchTransactionsForState
method to handleTransactionBatchMeta
.Rationale
The introduction of
TransactionBatchMeta
allows for a clean separation of metadata for batch transactions, which conceptually differ from individual transactions. This ensures:TransactionMeta
for individual transactions may not yet exist.These changes lay the groundwork for improved handling of batch transactions and pave the way for future enhancements.
References
Changelog
Checklist