feat: support atomic batch transactions #5306
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create prepare utils.
Add initial unit tests.
Add feature flags util.
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@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. |
|
@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. |
|
@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. |
jpuri
reviewed
Feb 14, 2025
| log('Retrieved feature flags', featureFlags); | ||
|
|
||
| return featureFlags as TransactionControllerFeatureFlags; | ||
| } |
There was a problem hiding this comment.
Not blocking for this PR, but I have a doubt if it is ok to use launchdarkly to dynamically get data like this. May be we make our own api for the purpose.
Also, is there a plan at what stage we get rid of using feature flags for this functionality.
There was a problem hiding this comment.
It's not dynamically getting data, it's still reliant on the polling that is controlled by the client, so it won't fire a request for each usage.
Both of these usages are also still conceptually "feature flags" as the supported chains are a means to enable the feature progressively as each chain adds support.
Similarly, the contract addresses are a means to "release" new contracts for the user to upgrade to.
Even if they were purely for remote configuration, I think we've been doing this for even longer than feature flags through swaps and smart transactions. Why would we create our own service and infrastructure, and client logic, to achieve what is already possible with one platform?
There is no plans to remove this functionality in future since it's a key part of the management, and security model.
pedronfigueiredo
approved these changes
Feb 14, 2025
jpuri
reviewed
Feb 14, 2025
| import { | ||
| getEIP7702ContractAddresses, | ||
| getEIP7702SupportedChains, | ||
| } from './feature-flags'; |
jpuri
reviewed
Feb 14, 2025
| return { | ||
| batchId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This does not look like utility function, may be it should have been part of controller itself. Also, does not look a good practice to pass functions, messenger to utility function like above.
There was a problem hiding this comment.
The main TransactionController file is almost 4000 lines long so this is a fundamental part of our controller architecture to modularise functionality in order to keep it readable, maintainable, and testable.
The practice of passing callbacks and the messenger itself is well established as it's the same as what we do with the controllers themselves in the client, it provides a clean abstraction to provide logic in a decoupled way without duplication or unnecessarily coupling distinct pieces of logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Support atomic batch transactions via EIP-7702, and ERC-7821.
Specifically:
addTransactionBatchmethod withTransactionBatchRequestandTransactionBatchResulttypes.executecall using ERC-7821 ABI.setCodetransaction if needed.isAtomicBatchSupportedmethod to identify which chains support atomic batch for a given account.batchTransactionType.batchutils to encapsulate all batch-related logic.feature-flagsutils to encapsulate retrieval and fallback of LaunchDarkly configuration.toof external transaction is not an internal account unlesstransactionTypeisbatch.References
Fixes #4096
Changelog
See
CHANGELOG.md.Checklist