Integrate Shield gateway in transaction controller #6281
Merged
Conversation
This file contains hidden or 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
matthewwalsh0
previously requested changes
Aug 11, 2025
| /** | ||
| * Optional parameters for the transaction shield. | ||
| */ | ||
| shieldParams?: ShieldParams; |
There was a problem hiding this comment.
Assuming we definitely want to go the client route, instead of a remote-only service / cache of some kind, then the TransactionController should not know about Shield at all.
It's a fundamental controller, so it's vital we abstract and decouple other features and domain for the sake of maintainability and coupling.
Could this instead be something like:
getSimulationConfig?: () => Promise<{url?: string; authorization?: string;}>;
There was a problem hiding this comment.
will adapt the PR accordingly
There was a problem hiding this comment.
updated to
export type GetSimulationConfig = () => Promise<{
baseUrl?: string;
authorization?: string;
}>;There was a problem hiding this comment.
realized that we need to provide the current URL as a request parameter included in the proxy URL.
update to
export type GetSimulationConfig = (url: string) => Promise<{
newUrl?: string;
authorization?: string;
}>;
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
3 times, most recently
from
55ab505 to
1bafbab
Compare
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
from
1bafbab to
d2c3e3b
Compare
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
2 times, most recently
from
ab4ffb1 to
83695c3
Compare
OGPoyraz
reviewed
Aug 20, 2025
OGPoyraz
reviewed
Aug 20, 2025
OGPoyraz
reviewed
Aug 20, 2025
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
from
5206c99 to
7b6467c
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. |
OGPoyraz
previously approved these changes
Aug 21, 2025
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
from
c567bb1 to
1273915
Compare
OGPoyraz
previously approved these changes
Aug 21, 2025
Comment on lines
756
to
-754
| update, | ||
| getEthQuery, | ||
| getChainId, |
|
@metamaskbot publish-preview |
matthiasgeihs
dismissed
matthewwalsh0’s stale review
Request has been addressed
- sort parameters alphabetically
- make `#getSimulationConfig` class property mandatory
- update coverage because we can't test the `getSimulationConfig`
fallback created in the constructor
- make `getSimulationConfig` function parameter mandatory
- fix parameter ordering
matthiasgeihs
force-pushed
the
mg/integrate-shield-gateway
branch
from
7749841 to
9706a20
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. |
OGPoyraz
approved these changes
Aug 25, 2025
micaelae
pushed a commit
that referenced
this pull request
Aug 25, 2025
## Explanation For MetaMask Transaction Shield we want to reroute all transaction simulation requests through a dedicated proxy. This PR adds the following functionality to the TransactionController to accomplish that. - Add optional `getSimulationConfig` parameter to constructor. - If `getSimulationConfig` parameter is present, call the function to retrieve a new simulation URL and an authorization header. Replace the simulation URL and add the authorization header to the simulation request. <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References [Transaction Shield Notion Page](https://www.notion.so/Shield-1c3f86d67d68801f8821dae7af4de4bc?source=copy_link) <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
7 tasks
github-merge-queue bot
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 27, 2025
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** For MetaMask Transaction Shield, we want to be able to route Security Alerts API requests through a dedicated proxy, if the user is subscribed to Shield. This PR adds a corresponding configuration option to the Security Alerts API, similar to how it was done for the Simulation API in the TransactionController (MetaMask/core#6281). This option is not used yet. It will later be used once the corresponding backend services are deployed and the new ShieldController and SubscriptionController are being integrated. <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/35396?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
](https://codespaces.new/MetaMask/metamask-extension/pull/35396?quickstart=1){kind=link}
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
For MetaMask Transaction Shield we want to reroute all transaction simulation requests through a dedicated proxy.
This PR adds the following functionality to the TransactionController to accomplish that.
getSimulationConfigparameter to constructor.getSimulationConfigparameter is present, call the function to retrieve a new simulation URL and an authorization header. Replace the simulation URL and add the authorization header to the simulation request.References
Transaction Shield Notion Page
Checklist