Skip to content
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

New e2e tests #28470

Closed
wants to merge 85 commits into from
Closed

Conversation

jonybur
Copy link
Contributor

@jonybur jonybur commented Nov 14, 2024

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • 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.

gambinish and others added 30 commits November 12, 2024 17:17
…bject Model (#28383)

## **Description**

- Migrate e2e tests `test/e2e/tests/tokens/nft/auto-detect-nft.spec.js`
to TS and Page Object Model, to reduce flakiness.
- Migrate e2e tests `test/e2e/tests/tokens/nft/import-nft.spec.js` to TS
and Page Object Model, to reduce flakiness.
- Create page classe functions for nft
- Deprecate/remove old functions in `helper.js`


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27155?quickstart=1)

## **Related issues**

Fixes: #28388

## **Manual testing steps**
Check code readability, make sure tests pass.

## **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/develop/.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/develop/.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.
<!--
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**
There is a race condition in our testing setup, which causes that the
expected fixtures state, not being there when we start the test. This
has been surfaced in [this
branch](#28277),
where the account tracker multi polling is being added.
The problem is that if we don't have the AccountTracker in state when
the `resetState` function is called (at the beginning of wallet loading)
the balance will remain loading until we refresh the wallet.

Edit: performing the load state early in the test setup fixes the issue.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28421?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3627

## **Manual testing steps**

1. Check all tests continue to pass
2. Check this changes fix this branch ENS test
https://github.com/MetaMask/metamask-extension/pull/28402/files#diff-1acb7898d60977530c97169551d22dbe477a4e3aeb74f1f14bf2eea0b4d75d35
. Alternatively, see videos below with before and after behaviours

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**




https://github.com/user-attachments/assets/8f50ec04-cf96-478e-9c3c-dce54254a628



### **After**


https://github.com/user-attachments/assets/0f109b1a-9289-48d9-b337-d51890c9d448




## **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/develop/.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/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
<!--
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**

<!--
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?
-->

This PR reintroduces the happy path send flow tests for BTC

## **Related issues**

Fixes: MetaMask/accounts-planning#668

## **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**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

---------

Co-authored-by: Charly Chevalier <[email protected]>
…#28403)

<!--
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**

Previously, some gas fee parameters defaulted to dApp suggested fee
values. This was problematic, because these values are always defined,
even when the user changed to a custom priority.

To fix the issue, we now only show the fees contained in txParams,
trusting they match the user selected priority level in the gas fees
modal.

<!--
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?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28403?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

See screen recording below.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->



https://github.com/user-attachments/assets/3874d3b6-ad96-4fd9-9f40-361b38a0253b



### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
<!--
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**

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28086?quickstart=1)

### Motivation

- Creating a Sentry custom span that would capture this performance
issue:
  - #27500
- It was determined that the issue is not captured by the existing "UI
Startup" custom span
(MetaMask/MetaMask-planning#3398).

### Approach

I've decided to widen the scope of this ticket so that it covers
performance issues with all accounts in general (not just those created
with the `account-watcher` snap).

To this end, I've defined three new custom spans: 

- `'Account Overview - Asset List Tab'`
- `'Account Overview - Nfts Tab'`
- `'Account Overview - Activity Tab'`

#### Behavior

- **Triggers** a) when an account list item is clicked in the account
list menu component, or b) when an account overview tab (e.g. "Tokens",
"NFTs", "Activity") is clicked, or c) (Only for `"Account Overview -
Asset List Tab"`) the "Import" button in the detected token selection
popover is clicked.
- **Terminates** when one of the following happens (race):
  - A different account is clicked in the account list menu.
  - An account overview tab is clicked.
- The newly selected tab component finishes loading -- including its
resources.
- If there is no data to display, the loading spinner/message
disappears, and the appropriate filler content renders.

#### Helpers

- Added selector for the `defaultHomeActiveTabName` property of the
`metamask` slice.
- Added enum `AccountOverviewTabKey` and objects
`ACCOUNT_OVERVIEW_TAB_KEY_TO_METAMETRICS_EVENT_NAME_MAP`,
`ACCOUNT_OVERVIEW_TAB_KEY_TO_TRACE_NAME_MAP`.

### Results

- The custom spans function as expected.
- The spans successfully distinguish periods of inactivity with loading
time.
- The spans continue to capture delays while the UI toggles between
accounts and account overview tabs.
<img width="1677" alt="Screenshot 2024-11-12 at 8 52 58 AM"
src="https://github.com/user-attachments/assets/62eda693-f488-46f5-a808-6f25a1763d52">

- The custom spans capture the performance issues that motivated this
ticket
  
- #27500 (see 1.46m
long Nfts Tab span and 5.49s Asset List Tab span)
<img width="1214" alt="Screenshot 2024-11-13 at 1 24 17 AM"
src="https://github.com/user-attachments/assets/9c1f0108-f194-4074-bcc4-1a96f94781d5">
  
- #28130 (see 1.28m
long Nfts Tab span and Error message: "Cannot destructure property
'imageOriginal' ...")
<img width="1214" alt="Screenshot 2024-11-13 at 1 30 00 AM"
src="https://github.com/user-attachments/assets/9baaf40c-d4da-465b-aef5-cbd684d6f450">

## **Related issues**

- Closes MetaMask/MetaMask-planning#3516

## **Manual testing steps**

1. Click one or more of the account overview tabs ("Tokens", "NFTs",
"Activity").
2. Click Account Menu
3. Switch account and repeat.
  - Add accounts as necessary.
- For load testing, add whale account(s) using the experimental "Watch
Account" feature.


## **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/develop/.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/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
…bject Model (#28383)

## **Description**

- Migrate e2e tests `test/e2e/tests/tokens/nft/auto-detect-nft.spec.js`
to TS and Page Object Model, to reduce flakiness.
- Migrate e2e tests `test/e2e/tests/tokens/nft/import-nft.spec.js` to TS
and Page Object Model, to reduce flakiness.
- Create page classe functions for nft
- Deprecate/remove old functions in `helper.js`


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27155?quickstart=1)

## **Related issues**

Fixes: #28388

## **Manual testing steps**
Check code readability, make sure tests pass.

## **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/develop/.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/develop/.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.
<!--
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**
There is a race condition in our testing setup, which causes that the
expected fixtures state, not being there when we start the test. This
has been surfaced in [this
branch](#28277),
where the account tracker multi polling is being added.
The problem is that if we don't have the AccountTracker in state when
the `resetState` function is called (at the beginning of wallet loading)
the balance will remain loading until we refresh the wallet.

Edit: performing the load state early in the test setup fixes the issue.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28421?quickstart=1)

## **Related issues**

Fixes: MetaMask/MetaMask-planning#3627

## **Manual testing steps**

1. Check all tests continue to pass
2. Check this changes fix this branch ENS test
https://github.com/MetaMask/metamask-extension/pull/28402/files#diff-1acb7898d60977530c97169551d22dbe477a4e3aeb74f1f14bf2eea0b4d75d35
. Alternatively, see videos below with before and after behaviours

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**




https://github.com/user-attachments/assets/8f50ec04-cf96-478e-9c3c-dce54254a628



### **After**


https://github.com/user-attachments/assets/0f109b1a-9289-48d9-b337-d51890c9d448




## **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/develop/.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/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
<!--
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**

<!--
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?
-->

This PR reintroduces the happy path send flow tests for BTC

## **Related issues**

Fixes: https://github.com/MetaMask/accounts-planning/issues/668

## **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**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

---------

Co-authored-by: Charly Chevalier <[email protected]>
…#28403)

<!--
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**

Previously, some gas fee parameters defaulted to dApp suggested fee
values. This was problematic, because these values are always defined,
even when the user changed to a custom priority.

To fix the issue, we now only show the fees contained in txParams,
trusting they match the user selected priority level in the gas fees
modal.

<!--
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?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28403?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

See screen recording below.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->



https://github.com/user-attachments/assets/3874d3b6-ad96-4fd9-9f40-361b38a0253b



### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
NidhiKJha and others added 14 commits November 14, 2024 10:13
<!--
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**

<!--
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?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28443?quickstart=1)

## **Related issues**

Fixes: #28322
Fixes: #28339

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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/develop/.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/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

Co-authored-by: Vinicius Stevam <[email protected]>
<!--
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**

The current mock server setup injects all mocks at the beginning of the
test, which can lead to delays. Specifically, when we assert the account
details opened metric, only the relevant endpoint for that metric gets
triggered, while other mocks, like "signature approved," may not be
invoked yet. Although this doesn’t cause the test to fail, it results in
unnecessarily long wait times during the test, as the framework waits
for mocks that haven’t been triggered.

This PR addresses the issue by moving the metric assertions to the end
of the test. By doing so, we eliminate the idle waiting period,
streamlining the test flow and reducing overall test execution time.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27532?quickstart=1)

## **Related issues**

Fixes:
[#19823](#19823)

## **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**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.
## **Description**

Fix the `attribution:generate` command by ensuring that it is possible
to install just production dependencies.

Previously the command `yarn workspaces focus --production` (used to
discard development dependencies, keeping just production dependencies
installed) would fail because `rimraf` was not found. `rimraf` was a
development dependency used in the `postinstall` script. This was
resolved by replacing `rimraf` with a Node.js script that does the same
thing without needing any dependency.

Once that failure was resolved, another was revealed. The
`allow-scripts` step of the installation began failing because there was
a package detected that had an install script that was missing from our
configuration. This package was in our configuration already, but the
`allow-scripts` configuration is sensitive to changes in the directory
structure of `node_modules`, and that structure changed due to
differences in which packages were hoisted in the production-only
install.

That failure was resolved by updating `generate-attributions.sh` to
remove the `allow-scripts` plugin while generating attributions. We
don't need `postinstall` scripts to run in order to read licences from
disk.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28415?quickstart=1)

## **Related issues**

Fixes #28412

## **Manual testing steps**

1. Run `yarn attributions:generate`, and see that it completes
successfully
* Locally, it should also re-install the `allow-scripts` plugin and
development dependencies
* If this command is run with `CI=true` (e.g. `CI=true yarn
attributions:generate`), it will skip the step of re-installing the
`allow-scripts` plugin and development dependencies. This is what would
happen on CI, where the environment gets discarded after this is run so
there is no point in re-installing things.

## **Screenshots/Recordings**

N/A

## **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/develop/.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/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

---------

Co-authored-by: MetaMask Bot <[email protected]>
## **Description**

This PR adds transaction simulation metrics when the UI is not shown and
the transaction simulation settings is turned on

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28427?quickstart=1)

## **Related issues**

Fixes: #28369

## **Manual testing steps**
Turn on MetaMetrics settings

1. Go to wallet dashboard
2. Initiate send transaction directly through wallet
3. Reject or Cancel the transaction
4. Observe simulation metrics

## **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**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] 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.

---------

Co-authored-by: Vinicius Stevam <[email protected]>
Co-authored-by: Vinicius Stevam <[email protected]>
…-balances' into remove-portfolio-view-fallout
@jonybur jonybur requested review from a team and kumavis as code owners November 14, 2024 20:28
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] network +2 483 kB metamaskbot

🚮 Removed packages: npm/@babel/[email protected], npm/@babel/[email protected], npm/@metamask/[email protected]

View full report↗︎

Copy link

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Network access npm/@metamask/[email protected] 🚫

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@jonybur jonybur closed this Nov 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 2024
@jonybur jonybur reopened this Nov 15, 2024
@jonybur jonybur closed this Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.