feat: Release Chain Permissions #27561

NidhiKJha · 2024-10-02T10:59:59Z

This PR is to remove feature flags and add e2e for Chain Permissions

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2713

Manual testing steps

Run yarn start
Everything should work

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js

app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.test.js

jiexi · 2024-10-02T15:54:25Z

app/scripts/metamask-controller.js

+              ...(requestedPermissions[RestrictedMethods.eth_accounts] && {
+                [PermissionNames.permittedChains]: {},
+              }),


do we anticipate a user trying to request permittedChains by itself? if so we probably need to populate an empty eth_accounts object here too? @adonesky1

do we anticipate a user trying to request permittedChains by itself? if so we probably need to populate an empty eth_accounts object here too? @adonesky1

🤔 I think whether or not we encourage it we should include an accounts permission as part of the request to avoid confusion/frustration

app/scripts/metamask-controller.js

jiexi · 2024-10-02T16:01:59Z

I see a few more references to CHAIN_PERMISSIONS left when I search in code

NidhiKJha · 2024-10-02T16:03:17Z

I see a few more references to CHAIN_PERMISSIONS left when I search in code

Those are for confirmation page Copy Changes. We are not removing feature flag from these files

…ltichain-e2e-tests-v2 # Conflicts: # app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js

## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27718?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27725?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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.

seaona · 2024-10-09T16:01:45Z

QA Findings

Signed Transaction stuck after network switch

I've encountered a case where after switching networks using window.ethereum on a dapp and trying to send a transaction the tx status remains in Signed.

Then I cannot send transactions on other networks, as it says there's a tx pending.

Smart Transactions are disabled.

Repro:
I'm unsure if there's something in the wallet state that causes this, because just by doing these below, I encounter the issue:

Setup ganache
Connect to test dapp
Trigger tx
Confirm
Tx remains signed

signed-tx-local.mp4

Invalid v value

This is a failure that sometimes appears in our e2e tests. I think this needs to be investigated further as might be a bug in the wallet side.

I'm trying to find repro steps (no luck so far), but I find it deterministically by running this spec in the mmi build:

Switch Ethereum Chain for two dapps queues switchEthereumChain request from second dapp after send tx request

I've seen this in other tests too, but the tests didn't fail as they were not checking if the tx was successful or not. If that's confirmed, we'd need to add a new step to capture this deterministically

…sendTransaction calls (#27734)  ## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27734?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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.

sonarcloud · 2024-10-09T17:25:15Z

Quality Gate failed

Failed conditions
41.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

seaona · 2024-10-09T18:49:13Z

ci e2e webpack: Chrome errors in webpack makes test fail

We see there are some empty errors that make test fails sometimes in webpack. After checking why those are empty we've seen that it's because those RPC errors don't have any value, but a description field. With the below change we can see how the error message is then displayed and we can see how those are expected RPC errors due to the server being down (in the test we shut down the 2 ganache servers)

#getErrorFromEvent(event) {
    // Extract the values from the array
    const values = event.args.map((a) => {
      console.log("Argument:", a); // Added 
      return a.value ? a.value : a.description; // changed
  });

Current ci:

With logs added:

Remaining part: we should fix the ignore-all to make that these errors are also ignored.

metamaskbot · 2024-10-09T18:49:36Z

Builds ready [535b710]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1946 ± 161 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1605	2812	1936	322	154
		domContentLoaded	1563	2597	1878	263	127
		load	1601	2853	1946	335	161
		domInteractive	14	80	39	16	8

Bundle size diffs [🚀 Bundle size reduced!]

background: -297 Bytes (-0.01%)
ui: -3.13 KiB (-0.04%)
common: -200 Bytes (-0.00%)

NidhiKJha · 2024-10-09T18:56:00Z

ci e2e webpack: Chrome errors in webpack makes test fail

@seaona interesting catch. Following our slack conversation if the flakiness would be there, I will file a follow up PR and address it there. Thanks ❤️

EtherWizard33

Hey Nidhi, looks like one of the checks in not passing

vinnyhoward · 2024-10-09T19:57:39Z

test/e2e/tests/request-queuing/switchChain-sendTx.spec.js

-          text: 'Switching networks will cancel all pending confirmations',
-          tag: 'span',
-        });
+        // await driver.findElement({


We can probably remove this correct?

vinnyhoward

LGTM!✅

FrederikBolding · 2024-10-10T09:15:08Z

test/e2e/snaps/test-snap-txinsights.spec.js

        await driver.clickElement({
          text: 'Confirm',
          tag: 'button',
        });

-        await driver.waitForSelector({ text: 'OK' });


I believe these waitForSelector calls were added to try and combat flakiness, I don't think we should be removing them. This applies to multiple places and files in this PR.

cc @bowensanders

This is replaced with await driver.clickElementAndWaitForWindowToClose. We can keep it but here we have have added an extra condition that the windown close when this button is clicked. In other instances of file, we wanted to make sure that the window is closed once we click the button.

FrederikBolding · 2024-10-10T09:16:04Z

test/e2e/snaps/test-snap-txinsights-v2.spec.js

@@ -140,12 +127,6 @@ describe('Test Snap TxInsights-v2', function () {
          tag: 'button',
          text: 'Activity',
        });
-
-        // wait for transaction confirmation
-        await driver.waitForSelector({


How come this is removed?

Yeah, this one I can add back. Can I do it in a follow up PR?

FrederikBolding · 2024-10-10T09:28:55Z

ui/components/app/permissions-connect-permission-list/permissions-connect-permission-list.js

@@ -18,10 +17,6 @@ import { CaveatTypes } from '../../../../shared/constants/permissions';
 * @returns {JSX.Element} A permission description node.
 */
 function getDescriptionNode(permission, index, accounts) {
-  const permissionValue = permission?.permissionValue?.caveats?.find(


This seems to revert 4fa502b - is that intentional?

Yes, because caveats doesn't exist in permissionValue object

Hmm, any idea why this value changed since that fix then? 🤔

FrederikBolding · 2024-10-10T09:30:22Z

ui/components/app/permission-cell/permission-cell-status.js

@@ -49,7 +49,7 @@ export const PermissionCellStatus = ({

  const renderAccountsGroup = () => (
    <>
-      {process.env.CHAIN_PERMISSIONS ? (
+      {networks.length > 0 ? (


This confuses me, are we stopping rendering accounts? If so, can we remove the code for it?

Or are we keeping it for Snaps for now? If so, this is still confusing 😅

We are just keeping it for Snaps. networks is defined only in case of permittedChains.

Seems like it is in use for non-Snap connections too. I think this deserves a refactor outside of this PR. It feels confusing to nest the networks display logic inside the account group logic.

For initial connection screen, we need to show the current active account and all the enabled networks to be there , this screen should not show up for Snaps. This is connections screen, it doesn't use permission cell anyway

FrederikBolding

Looks like we have double padding on switchEthereumChain. Unclear if this is caused by this PR

FrederikBolding

Not blocking, but I have left some comments that should probably be addressed in a follow up.

NidhiKJha requested review from a team as code owners October 2, 2024 11:00

NidhiKJha added the team-wallet-ux label Oct 2, 2024

NidhiKJha force-pushed the multichain-e2e-tests-v2 branch from 084a4bc to 250ba94 Compare October 2, 2024 12:30

darkwing changed the title ~~feat: added e2e and removed feature flag~~ feat: Release Amon Hen v2 Oct 2, 2024

NidhiKJha added 10 commits October 2, 2024 15:59

added e2e and removed feature flag

031b373

reverted permssion page

6c1cc39

updated tests

34c8900

updated more spec

7159c3c

updated more spec

dbabded

updated ui.spec

e660639

updated prep build and removed unused spec

bdd7dcb

removed unused locale

d3161b6

updated spec

aa33dd5

updated permissions container

af5271b

NidhiKJha force-pushed the multichain-e2e-tests-v2 branch from 8b33f63 to af5271b Compare October 2, 2024 15:07

updated helper

30eec2c

jiexi reviewed Oct 2, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js Show resolved Hide resolved

jiexi reviewed Oct 2, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.test.js Outdated Show resolved Hide resolved

jiexi reviewed Oct 2, 2024

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

NidhiKJha and others added 7 commits October 2, 2024 17:16

updated helper

bec6302

remove getChainPermissionsFeatureFlag variable

4f9d8b7

Merge branch 'develop' into multichain-e2e-tests-v2

b59058f

Fix add/switch chain

6579bc2

updated permission page spec

e05fe5b

Merge remote-tracking branch 'origin/multichain-e2e-tests-v2' into mu…

c611dc0

…ltichain-e2e-tests-v2 # Conflicts: # app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js

fix add/switch

bd2886a

seaona and others added 3 commits October 9, 2024 14:51

Merge branch 'develop' into multichain-e2e-tests-v2

f72566f

EtherWizard33 self-requested a review October 9, 2024 19:18

EtherWizard33 reviewed Oct 9, 2024

View reviewed changes

darkwing approved these changes Oct 9, 2024

View reviewed changes

vinnyhoward reviewed Oct 9, 2024

View reviewed changes

vinnyhoward approved these changes Oct 9, 2024

View reviewed changes

FrederikBolding reviewed Oct 10, 2024

View reviewed changes

FrederikBolding approved these changes Oct 10, 2024

View reviewed changes

NidhiKJha added this pull request to the merge queue Oct 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

NidhiKJha added this pull request to the merge queue Oct 10, 2024

Merged via the queue into develop with commit 11b9bd4 Oct 10, 2024
77 of 78 checks passed

NidhiKJha deleted the multichain-e2e-tests-v2 branch October 10, 2024 11:33

github-actions bot locked and limited conversation to collaborators Oct 10, 2024

metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 10, 2024

gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Release Chain Permissions #27561

feat: Release Chain Permissions #27561

NidhiKJha commented Oct 2, 2024 •

edited

Loading

jiexi Oct 2, 2024

adonesky1 Oct 2, 2024

jiexi commented Oct 2, 2024

NidhiKJha commented Oct 2, 2024 •

edited

Loading

seaona commented Oct 9, 2024 •

edited

Loading

sonarcloud bot commented Oct 9, 2024

seaona commented Oct 9, 2024

metamaskbot commented Oct 9, 2024

NidhiKJha commented Oct 9, 2024

ci e2e webpack: Chrome errors in webpack makes test fail

EtherWizard33 left a comment •

edited

Loading

vinnyhoward Oct 9, 2024

vinnyhoward left a comment

FrederikBolding Oct 10, 2024 •

edited

Loading

NidhiKJha Oct 10, 2024

FrederikBolding Oct 10, 2024

NidhiKJha Oct 10, 2024

FrederikBolding Oct 10, 2024

NidhiKJha Oct 10, 2024

FrederikBolding Oct 10, 2024

FrederikBolding Oct 10, 2024 •

edited

Loading

NidhiKJha Oct 10, 2024

FrederikBolding Oct 10, 2024

NidhiKJha Oct 10, 2024 •

edited

Loading

FrederikBolding left a comment •

edited

Loading

FrederikBolding left a comment

feat: Release Chain Permissions #27561

feat: Release Chain Permissions #27561

Conversation

NidhiKJha commented Oct 2, 2024 • edited Loading

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiexi commented Oct 2, 2024

NidhiKJha commented Oct 2, 2024 • edited Loading

seaona commented Oct 9, 2024 • edited Loading

QA Findings

Signed Transaction stuck after network switch

Invalid v value

sonarcloud bot commented Oct 9, 2024

Quality Gate failed

seaona commented Oct 9, 2024

ci e2e webpack: Chrome errors in webpack makes test fail

metamaskbot commented Oct 9, 2024

NidhiKJha commented Oct 9, 2024

ci e2e webpack: Chrome errors in webpack makes test fail

EtherWizard33 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinnyhoward left a comment

Choose a reason for hiding this comment

FrederikBolding Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrederikBolding Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NidhiKJha Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

FrederikBolding left a comment • edited Loading

Choose a reason for hiding this comment

FrederikBolding left a comment

Choose a reason for hiding this comment

NidhiKJha commented Oct 2, 2024 •

edited

Loading

NidhiKJha commented Oct 2, 2024 •

edited

Loading

seaona commented Oct 9, 2024 •

edited

Loading

EtherWizard33 left a comment •

edited

Loading

FrederikBolding Oct 10, 2024 •

edited

Loading

FrederikBolding Oct 10, 2024 •

edited

Loading

NidhiKJha Oct 10, 2024 •

edited

Loading

FrederikBolding left a comment •

edited

Loading