From bcfaa3e09f1eca53c71b86713a1d8a861e748d3c Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 25 Sep 2024 13:32:59 +0200 Subject: [PATCH 1/6] feat: improve account syncing performance (#27330) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR adds several performance improvements and bug fixes for [account syncing](https://www.figma.com/board/sx26FtZcQc0zs0z5hpdqBh/Account-Syncing-Solution?t=5Oy00gbHLnoPqyYP-0). [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27330?quickstart=1) ## **Related issues** Fixes: - https://consensyssoftware.atlassian.net/browse/NOTIFY-1157 - https://consensyssoftware.atlassian.net/browse/NOTIFY-1158 - https://consensyssoftware.atlassian.net/browse/NOTIFY-1163 ## **Manual testing steps** 1. Log in with SRP 2. Add new accounts and rename others 3. Using another browser, log in with SRP 4. Watch the magic happen! ## **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. --------- Co-authored-by: MetaMask Bot Co-authored-by: OGPoyraz --- app/scripts/metamask-controller.js | 2 +- lavamoat/browserify/beta/policy.json | 4 ++++ lavamoat/browserify/flask/policy.json | 4 ++++ lavamoat/browserify/main/policy.json | 4 ++++ lavamoat/browserify/mmi/policy.json | 4 ++++ package.json | 2 +- .../useProfileSyncing.test.tsx | 2 ++ .../useProfileSyncing.ts | 24 +++++++++++++++---- yarn.lock | 24 ++++++++++--------- 9 files changed, 52 insertions(+), 18 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 8ad88c088b46..577184ed8ede 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1531,7 +1531,7 @@ export default class MetamaskController extends EventEmitter { }, }, env: { - isAccountSyncingEnabled: true, + isAccountSyncingEnabled: isManifestV3, }, messenger: this.controllerMessenger.getRestricted({ name: 'UserStorageController', diff --git a/lavamoat/browserify/beta/policy.json b/lavamoat/browserify/beta/policy.json index 6fe02c83f1ad..911a6ff8b04a 100644 --- a/lavamoat/browserify/beta/policy.json +++ b/lavamoat/browserify/beta/policy.json @@ -2069,14 +2069,18 @@ "URL": true, "URLSearchParams": true, "addEventListener": true, + "clearInterval": true, "console.error": true, "dispatchEvent": true, "fetch": true, "removeEventListener": true, + "setInterval": true, "setTimeout": true }, "packages": { "@metamask/base-controller": true, + "@metamask/keyring-api": true, + "@metamask/keyring-controller": true, "@metamask/message-signing-snap>@noble/ciphers": true, "@metamask/profile-sync-controller>siwe": true, "@noble/hashes": true, diff --git a/lavamoat/browserify/flask/policy.json b/lavamoat/browserify/flask/policy.json index 6fe02c83f1ad..911a6ff8b04a 100644 --- a/lavamoat/browserify/flask/policy.json +++ b/lavamoat/browserify/flask/policy.json @@ -2069,14 +2069,18 @@ "URL": true, "URLSearchParams": true, "addEventListener": true, + "clearInterval": true, "console.error": true, "dispatchEvent": true, "fetch": true, "removeEventListener": true, + "setInterval": true, "setTimeout": true }, "packages": { "@metamask/base-controller": true, + "@metamask/keyring-api": true, + "@metamask/keyring-controller": true, "@metamask/message-signing-snap>@noble/ciphers": true, "@metamask/profile-sync-controller>siwe": true, "@noble/hashes": true, diff --git a/lavamoat/browserify/main/policy.json b/lavamoat/browserify/main/policy.json index 6fe02c83f1ad..911a6ff8b04a 100644 --- a/lavamoat/browserify/main/policy.json +++ b/lavamoat/browserify/main/policy.json @@ -2069,14 +2069,18 @@ "URL": true, "URLSearchParams": true, "addEventListener": true, + "clearInterval": true, "console.error": true, "dispatchEvent": true, "fetch": true, "removeEventListener": true, + "setInterval": true, "setTimeout": true }, "packages": { "@metamask/base-controller": true, + "@metamask/keyring-api": true, + "@metamask/keyring-controller": true, "@metamask/message-signing-snap>@noble/ciphers": true, "@metamask/profile-sync-controller>siwe": true, "@noble/hashes": true, diff --git a/lavamoat/browserify/mmi/policy.json b/lavamoat/browserify/mmi/policy.json index 3cd466b03042..2fa339f5201e 100644 --- a/lavamoat/browserify/mmi/policy.json +++ b/lavamoat/browserify/mmi/policy.json @@ -2161,14 +2161,18 @@ "URL": true, "URLSearchParams": true, "addEventListener": true, + "clearInterval": true, "console.error": true, "dispatchEvent": true, "fetch": true, "removeEventListener": true, + "setInterval": true, "setTimeout": true }, "packages": { "@metamask/base-controller": true, + "@metamask/keyring-api": true, + "@metamask/keyring-controller": true, "@metamask/message-signing-snap>@noble/ciphers": true, "@metamask/profile-sync-controller>siwe": true, "@noble/hashes": true, diff --git a/package.json b/package.json index 12d1cd30d956..89712d8b0bd6 100644 --- a/package.json +++ b/package.json @@ -344,7 +344,7 @@ "@metamask/post-message-stream": "^8.0.0", "@metamask/ppom-validator": "0.34.0", "@metamask/preinstalled-example-snap": "^0.1.0", - "@metamask/profile-sync-controller": "^0.8.0", + "@metamask/profile-sync-controller": "^0.9.1", "@metamask/providers": "^14.0.2", "@metamask/queued-request-controller": "^2.0.0", "@metamask/rate-limit-controller": "^6.0.0", diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx b/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx index 178c646d0dda..481ad5deec9f 100644 --- a/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx +++ b/ui/hooks/metamask-notifications/useProfileSyncing.test.tsx @@ -29,6 +29,7 @@ type ArrangeMocksMetamaskStateOverrides = { isProfileSyncingEnabled?: boolean; isUnlocked?: boolean; useExternalServices?: boolean; + completedOnboarding?: boolean; }; const initialMetamaskState: ArrangeMocksMetamaskStateOverrides = { @@ -36,6 +37,7 @@ const initialMetamaskState: ArrangeMocksMetamaskStateOverrides = { isProfileSyncingEnabled: false, isUnlocked: true, useExternalServices: true, + completedOnboarding: true, }; const arrangeMocks = ( diff --git a/ui/hooks/metamask-notifications/useProfileSyncing.ts b/ui/hooks/metamask-notifications/useProfileSyncing.ts index c951e174d485..1306e160cb5e 100644 --- a/ui/hooks/metamask-notifications/useProfileSyncing.ts +++ b/ui/hooks/metamask-notifications/useProfileSyncing.ts @@ -13,7 +13,10 @@ import { import { selectIsSignedIn } from '../../selectors/metamask-notifications/authentication'; import { selectIsProfileSyncingEnabled } from '../../selectors/metamask-notifications/profile-syncing'; import { getUseExternalServices } from '../../selectors'; -import { getIsUnlocked } from '../../ducks/metamask/metamask'; +import { + getIsUnlocked, + getCompletedOnboarding, +} from '../../ducks/metamask/metamask'; // Define KeyringType interface export type KeyringType = { @@ -134,21 +137,32 @@ export const useAccountSyncing = () => { const basicFunctionality = useSelector(getUseExternalServices); const isUnlocked = useSelector(getIsUnlocked); const isSignedIn = useSelector(selectIsSignedIn); + const completedOnboarding = useSelector(getCompletedOnboarding); const shouldDispatchAccountSyncing = useMemo( () => - basicFunctionality && isProfileSyncingEnabled && isUnlocked && isSignedIn, - [basicFunctionality, isProfileSyncingEnabled, isUnlocked, isSignedIn], + basicFunctionality && + isProfileSyncingEnabled && + isUnlocked && + isSignedIn && + completedOnboarding, + [ + basicFunctionality, + isProfileSyncingEnabled, + isUnlocked, + isSignedIn, + completedOnboarding, + ], ); - const dispatchAccountSyncing = useCallback(async () => { + const dispatchAccountSyncing = useCallback(() => { setError(null); try { if (!shouldDispatchAccountSyncing) { return; } - await dispatch(syncInternalAccountsWithUserStorage()); + dispatch(syncInternalAccountsWithUserStorage()); } catch (e) { log.error(e); setError(e instanceof Error ? e.message : 'An unexpected error occurred'); diff --git a/yarn.lock b/yarn.lock index e6329f1b9a03..351040f272f5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5641,19 +5641,19 @@ __metadata: languageName: node linkType: hard -"@metamask/keyring-api@npm:^8.0.0, @metamask/keyring-api@npm:^8.1.0": - version: 8.1.0 - resolution: "@metamask/keyring-api@npm:8.1.0" +"@metamask/keyring-api@npm:^8.0.0, @metamask/keyring-api@npm:^8.1.0, @metamask/keyring-api@npm:^8.1.2": + version: 8.1.2 + resolution: "@metamask/keyring-api@npm:8.1.2" dependencies: - "@metamask/snaps-sdk": "npm:^6.1.0" + "@metamask/snaps-sdk": "npm:^6.5.1" "@metamask/superstruct": "npm:^3.1.0" - "@metamask/utils": "npm:^9.1.0" + "@metamask/utils": "npm:^9.2.1" "@types/uuid": "npm:^9.0.8" bech32: "npm:^2.0.0" uuid: "npm:^9.0.1" peerDependencies: "@metamask/providers": ">=15 <18" - checksum: 10/15711ddaa0007794cc23f9c02f6cfbee85aa1cf79a46468a0398404c295eef1511555ce6bd60a691081d33864d288ea8b309ee9ac9c4d6f277ab22e4d97cb76e + checksum: 10/bddb6d8c86f39b9afef0b484b473cfff5e33c99037de6ded2bc5d04d935971bf232237121c0e1297e43f266006f9c8a0cf48aa7d5588a7c4796fc803acdcfd00 languageName: node linkType: hard @@ -6053,11 +6053,13 @@ __metadata: languageName: node linkType: hard -"@metamask/profile-sync-controller@npm:^0.8.0": - version: 0.8.0 - resolution: "@metamask/profile-sync-controller@npm:0.8.0" +"@metamask/profile-sync-controller@npm:^0.9.1": + version: 0.9.1 + resolution: "@metamask/profile-sync-controller@npm:0.9.1" dependencies: "@metamask/base-controller": "npm:^7.0.1" + "@metamask/keyring-api": "npm:^8.1.2" + "@metamask/keyring-controller": "npm:^17.2.1" "@metamask/snaps-sdk": "npm:^6.5.0" "@metamask/snaps-utils": "npm:^8.1.1" "@noble/ciphers": "npm:^0.5.2" @@ -6069,7 +6071,7 @@ __metadata: "@metamask/accounts-controller": ^18.1.1 "@metamask/keyring-controller": ^17.2.0 "@metamask/snaps-controllers": ^9.7.0 - checksum: 10/613e2e87615f1db4a1a4559895a74c2b89c15720e831440eec0916aa96305f3db9b3e0926ac4a5d7532c462baaa18a117e2b9aa107949777163488c203cfea05 + checksum: 10/9f0c22e136a526527ec3a147084024b6d5a20219894166f00fc6b22d1554f61f19b357ccbdd32973745e03ab0481d1908295309870f723e7d3bf06f720569d8a languageName: node linkType: hard @@ -26136,7 +26138,7 @@ __metadata: "@metamask/post-message-stream": "npm:^8.0.0" "@metamask/ppom-validator": "npm:0.34.0" "@metamask/preinstalled-example-snap": "npm:^0.1.0" - "@metamask/profile-sync-controller": "npm:^0.8.0" + "@metamask/profile-sync-controller": "npm:^0.9.1" "@metamask/providers": "npm:^14.0.2" "@metamask/queued-request-controller": "npm:^2.0.0" "@metamask/rate-limit-controller": "npm:^6.0.0" From 76adc3701852e0baa5a87da7fe540827ce91ca1e Mon Sep 17 00:00:00 2001 From: weizman Date: Wed, 25 Sep 2024 15:02:01 +0300 Subject: [PATCH 2/6] chore: update snow dep version (#27386) --- package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 89712d8b0bd6..d54962b53b24 100644 --- a/package.json +++ b/package.json @@ -285,7 +285,7 @@ "@keystonehq/bc-ur-registry-eth": "^0.19.1", "@keystonehq/metamask-airgapped-keyring": "^0.13.1", "@lavamoat/lavadome-react": "0.0.17", - "@lavamoat/snow": "^2.0.1", + "@lavamoat/snow": "^2.0.2", "@material-ui/core": "^4.11.0", "@metamask-institutional/custody-controller": "^0.2.31", "@metamask-institutional/custody-keyring": "^2.0.3", diff --git a/yarn.lock b/yarn.lock index 351040f272f5..4a0bbee20818 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4347,10 +4347,10 @@ __metadata: languageName: node linkType: hard -"@lavamoat/snow@npm:^2.0.1": - version: 2.0.1 - resolution: "@lavamoat/snow@npm:2.0.1" - checksum: 10/f48aa87a4691748a48c0b90606119a07bba85f5315245ad3a076b4093eab0c74c0211d34a9b0b5029cb51b16b20704fdca18c1cbe182a82f0052374d61aa2c81 +"@lavamoat/snow@npm:^2.0.2": + version: 2.0.2 + resolution: "@lavamoat/snow@npm:2.0.2" + checksum: 10/613d4b7f42a80fb0c447f124ef0b7497afc7e5a5f81edd961975e14f35e9cfcbde04f649155f5d2fdcc334d5a190246ae90e23d53ca4e126e672e229c26895ae languageName: node linkType: hard @@ -26066,7 +26066,7 @@ __metadata: "@lavamoat/lavadome-core": "npm:0.0.10" "@lavamoat/lavadome-react": "npm:0.0.17" "@lavamoat/lavapack": "npm:^6.1.0" - "@lavamoat/snow": "npm:^2.0.1" + "@lavamoat/snow": "npm:^2.0.2" "@lgbot/madge": "npm:^6.2.0" "@lydell/node-pty": "npm:^1.0.1" "@material-ui/core": "npm:^4.11.0" From 470420f17f281c0c9f58ff7ea2cec86175ab6fd4 Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Wed, 25 Sep 2024 13:07:03 +0100 Subject: [PATCH 3/6] feat: Redesigned Revoke setApprovalForAll confirmation (#27111) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Includes modifiers to the setApprovalForAll screen to express revocation, including a new transaction simulation component. Includes e2e tests. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27111?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3005 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** Screenshot 2024-09-20 at 12 10 15 ## **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. --- app/_locales/en/messages.json | 9 ++ sonar-project.properties | 1 - ...proval-for-all-transaction-confirmation.ts | 10 ++ test/e2e/page-objects/pages/test-dapp.ts | 14 ++ ...55-revoke-set-approval-for-all-redesign.ts | 73 +++++++++ ...1155-set-approval-for-all-redesign.spec.ts | 1 - ...21-revoke-set-approval-for-all-redesign.ts | 96 ++++++++++++ .../__snapshots__/approve.test.tsx.snap | 2 +- .../approve-static-simulation.test.tsx.snap | 2 +- .../approve-static-simulation.tsx | 44 +++--- .../confirm/info/approve/hooks/use-is-nft.ts | 2 +- ...al-for-all-static-simulation.test.tsx.snap | 138 ++++++++++++++++++ ...oval-for-all-static-simulation.stories.tsx | 34 +++++ ...pproval-for-all-static-simulation.test.tsx | 25 ++++ ...set-approval-for-all-static-simulation.tsx | 65 +++++++++ .../set-approval-for-all-info.stories.tsx | 27 ++++ .../set-approval-for-all-info.tsx | 17 ++- ...set-approval-for-all-static-simulation.tsx | 55 +++---- .../static-simulation/static-simulation.tsx | 16 +- .../permit-simulation/permit-simulation.tsx | 51 +++++-- .../components/confirm/info/utils.test.ts | 38 +++++ .../components/confirm/info/utils.ts | 11 ++ .../components/confirm/title/title.tsx | 48 +++++- .../__snapshots__/confirm.test.tsx.snap | 16 +- 24 files changed, 700 insertions(+), 95 deletions(-) create mode 100644 test/e2e/tests/confirmations/transactions/erc1155-revoke-set-approval-for-all-redesign.ts create mode 100644 test/e2e/tests/confirmations/transactions/erc721-revoke-set-approval-for-all-redesign.ts create mode 100644 ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/__snapshots__/revoke-set-approval-for-all-static-simulation.test.tsx.snap create mode 100644 ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.stories.tsx create mode 100644 ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.test.tsx create mode 100644 ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.tsx create mode 100644 ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.stories.tsx create mode 100644 ui/pages/confirmations/components/confirm/info/utils.test.ts create mode 100644 ui/pages/confirmations/components/confirm/info/utils.ts diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 18236e3b109d..e312be4794e5 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -1071,6 +1071,9 @@ "confirmTitleSIWESignature": { "message": "Sign-in request" }, + "confirmTitleSetApprovalForAllRevokeTransaction": { + "message": "Remove permission" + }, "confirmTitleSignature": { "message": "Signature request" }, @@ -3778,6 +3781,9 @@ "permissionFor": { "message": "Permission for" }, + "permissionFrom": { + "message": "Permission from" + }, "permissionRequest": { "message": "Permission request" }, @@ -4863,6 +4869,9 @@ "simulationDetailsOutgoingHeading": { "message": "You send" }, + "simulationDetailsRevokeSetApprovalForAllDesc": { + "message": "You're removing someone else's permission to withdraw NFTs from your account." + }, "simulationDetailsSetApprovalForAllDesc": { "message": "You're giving permission for someone else to withdraw NFTs from your account." }, diff --git a/sonar-project.properties b/sonar-project.properties index 0ee619db70ff..a965dab30d7e 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -5,7 +5,6 @@ sonar.organization=consensys sonar.sources=app,development,offscreen,shared,types,ui sonar.exclusions=**/*.test.**,**/*.spec.**,app/images,test/e2e/page-objects,test/data - # Tests sonar.tests=app,development,offscreen,shared,test,types,ui sonar.test.inclusions=**/*.test.**,**/*.spec.** diff --git a/test/e2e/page-objects/pages/set-approval-for-all-transaction-confirmation.ts b/test/e2e/page-objects/pages/set-approval-for-all-transaction-confirmation.ts index ddd6ccb5e2b8..5259e0a51dcd 100644 --- a/test/e2e/page-objects/pages/set-approval-for-all-transaction-confirmation.ts +++ b/test/e2e/page-objects/pages/set-approval-for-all-transaction-confirmation.ts @@ -8,6 +8,8 @@ class SetApprovalForAllTransactionConfirmation extends TransactionConfirmation { private setApprovalForAllSubHeadingElement: RawLocator; + private revokeSetApprovalForAllTitleElement: RawLocator; + constructor(driver: Driver) { super(driver); @@ -21,6 +23,10 @@ class SetApprovalForAllTransactionConfirmation extends TransactionConfirmation { css: 'p', text: tEn('confirmTitleDescApproveTransaction') as string, }; + this.revokeSetApprovalForAllTitleElement = { + css: 'h2', + text: tEn('confirmTitleSetApprovalForAllRevokeTransaction') as string, + }; } async check_setApprovalForAllTitle() { @@ -30,6 +36,10 @@ class SetApprovalForAllTransactionConfirmation extends TransactionConfirmation { async check_setApprovalForAllSubHeading() { await this.driver.waitForSelector(this.setApprovalForAllSubHeadingElement); } + + async check_revokeSetApprovalForAllTitle() { + await this.driver.waitForSelector(this.revokeSetApprovalForAllTitleElement); + } } export default SetApprovalForAllTransactionConfirmation; diff --git a/test/e2e/page-objects/pages/test-dapp.ts b/test/e2e/page-objects/pages/test-dapp.ts index 29ff1f1e16b7..89ee6bc9cbd3 100644 --- a/test/e2e/page-objects/pages/test-dapp.ts +++ b/test/e2e/page-objects/pages/test-dapp.ts @@ -11,11 +11,17 @@ class TestDapp { private erc1155SetApprovalForAllButton: RawLocator; + private erc721RevokeSetApprovalForAllButton: RawLocator; + + private erc1155RevokeSetApprovalForAllButton: RawLocator; + constructor(driver: Driver) { this.driver = driver; this.erc721SetApprovalForAllButton = '#setApprovalForAllButton'; this.erc1155SetApprovalForAllButton = '#setApprovalForAllERC1155Button'; + this.erc721RevokeSetApprovalForAllButton = '#revokeButton'; + this.erc1155RevokeSetApprovalForAllButton = '#revokeERC1155Button'; } async open({ @@ -48,6 +54,14 @@ class TestDapp { async clickERC1155SetApprovalForAllButton() { await this.driver.clickElement(this.erc1155SetApprovalForAllButton); } + + public async clickERC721RevokeSetApprovalForAllButton() { + await this.driver.clickElement(this.erc721RevokeSetApprovalForAllButton); + } + + public async clickERC1155RevokeSetApprovalForAllButton() { + await this.driver.clickElement(this.erc1155RevokeSetApprovalForAllButton); + } } export default TestDapp; diff --git a/test/e2e/tests/confirmations/transactions/erc1155-revoke-set-approval-for-all-redesign.ts b/test/e2e/tests/confirmations/transactions/erc1155-revoke-set-approval-for-all-redesign.ts new file mode 100644 index 000000000000..7f26e02a572c --- /dev/null +++ b/test/e2e/tests/confirmations/transactions/erc1155-revoke-set-approval-for-all-redesign.ts @@ -0,0 +1,73 @@ +/* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ +import { TransactionEnvelopeType } from '@metamask/transaction-controller'; +import { DAPP_URL } from '../../../constants'; +import { unlockWallet, WINDOW_TITLES } from '../../../helpers'; +import { Mockttp } from '../../../mock-e2e'; +import SetApprovalForAllTransactionConfirmation from '../../../page-objects/pages/set-approval-for-all-transaction-confirmation'; +import TestDapp from '../../../page-objects/pages/test-dapp'; +import GanacheContractAddressRegistry from '../../../seeder/ganache-contract-address-registry'; +import { Driver } from '../../../webdriver/driver'; +import { withRedesignConfirmationFixtures } from '../helpers'; +import { mocked4BytesSetApprovalForAll } from './erc721-revoke-set-approval-for-all-redesign'; +import { TestSuiteArguments } from './shared'; + +const { SMART_CONTRACTS } = require('../../../seeder/smart-contracts'); + +describe('Confirmation Redesign ERC1155 Revoke setApprovalForAll', function () { + describe('Submit an revoke transaction @no-mmi', function () { + it('Sends a type 0 transaction (Legacy)', async function () { + await withRedesignConfirmationFixtures( + this.test?.fullTitle(), + TransactionEnvelopeType.legacy, + async ({ driver, contractRegistry }: TestSuiteArguments) => { + await createTransactionAndAssertDetails(driver, contractRegistry); + }, + mocks, + SMART_CONTRACTS.NFTS, + ); + }); + + it('Sends a type 2 transaction (EIP1559)', async function () { + await withRedesignConfirmationFixtures( + this.test?.fullTitle(), + TransactionEnvelopeType.feeMarket, + async ({ driver, contractRegistry }: TestSuiteArguments) => { + await createTransactionAndAssertDetails(driver, contractRegistry); + }, + mocks, + SMART_CONTRACTS.NFTS, + ); + }); + }); +}); + +async function mocks(server: Mockttp) { + return [await mocked4BytesSetApprovalForAll(server)]; +} + +async function createTransactionAndAssertDetails( + driver: Driver, + contractRegistry?: GanacheContractAddressRegistry, +) { + await unlockWallet(driver); + + const contractAddress = await ( + contractRegistry as GanacheContractAddressRegistry + ).getContractAddress(SMART_CONTRACTS.NFTS); + + const testDapp = new TestDapp(driver); + + await testDapp.open({ contractAddress, url: DAPP_URL }); + + await testDapp.clickERC1155RevokeSetApprovalForAllButton(); + + await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); + + const setApprovalForAllConfirmation = + new SetApprovalForAllTransactionConfirmation(driver); + + await setApprovalForAllConfirmation.check_revokeSetApprovalForAllTitle(); + + await setApprovalForAllConfirmation.clickScrollToBottomButton(); + await setApprovalForAllConfirmation.clickFooterConfirmButton(); +} diff --git a/test/e2e/tests/confirmations/transactions/erc1155-set-approval-for-all-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/erc1155-set-approval-for-all-redesign.spec.ts index 7d87cab123b3..438b3e979d0a 100644 --- a/test/e2e/tests/confirmations/transactions/erc1155-set-approval-for-all-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/erc1155-set-approval-for-all-redesign.spec.ts @@ -89,7 +89,6 @@ async function createTransactionAssertDetailsAndConfirm( await testDapp.clickERC1155SetApprovalForAllButton(); await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); - const setApprovalForAllConfirmation = new SetApprovalForAllTransactionConfirmation(driver); diff --git a/test/e2e/tests/confirmations/transactions/erc721-revoke-set-approval-for-all-redesign.ts b/test/e2e/tests/confirmations/transactions/erc721-revoke-set-approval-for-all-redesign.ts new file mode 100644 index 000000000000..5a8dcd3768f7 --- /dev/null +++ b/test/e2e/tests/confirmations/transactions/erc721-revoke-set-approval-for-all-redesign.ts @@ -0,0 +1,96 @@ +/* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */ +import { TransactionEnvelopeType } from '@metamask/transaction-controller'; +import { DAPP_URL } from '../../../constants'; +import { unlockWallet, WINDOW_TITLES } from '../../../helpers'; +import { Mockttp } from '../../../mock-e2e'; +import SetApprovalForAllTransactionConfirmation from '../../../page-objects/pages/set-approval-for-all-transaction-confirmation'; +import TestDapp from '../../../page-objects/pages/test-dapp'; +import GanacheContractAddressRegistry from '../../../seeder/ganache-contract-address-registry'; +import { Driver } from '../../../webdriver/driver'; +import { withRedesignConfirmationFixtures } from '../helpers'; +import { TestSuiteArguments } from './shared'; + +const { SMART_CONTRACTS } = require('../../../seeder/smart-contracts'); + +describe('Confirmation Redesign ERC721 Revoke setApprovalForAll', function () { + describe('Submit an revoke transaction @no-mmi', function () { + it('Sends a type 0 transaction (Legacy)', async function () { + await withRedesignConfirmationFixtures( + this.test?.fullTitle(), + TransactionEnvelopeType.legacy, + async ({ driver, contractRegistry }: TestSuiteArguments) => { + await createTransactionAndAssertDetails(driver, contractRegistry); + }, + mocks, + SMART_CONTRACTS.NFTS, + ); + }); + + it('Sends a type 2 transaction (EIP1559)', async function () { + await withRedesignConfirmationFixtures( + this.test?.fullTitle(), + TransactionEnvelopeType.feeMarket, + async ({ driver, contractRegistry }: TestSuiteArguments) => { + await createTransactionAndAssertDetails(driver, contractRegistry); + }, + mocks, + SMART_CONTRACTS.NFTS, + ); + }); + }); +}); + +async function mocks(server: Mockttp) { + return [await mocked4BytesSetApprovalForAll(server)]; +} + +export async function mocked4BytesSetApprovalForAll(mockServer: Mockttp) { + return await mockServer + .forGet('https://www.4byte.directory/api/v1/signatures/') + .withQuery({ hex_signature: '0xa22cb465' }) + .always() + .thenCallback(() => ({ + statusCode: 200, + json: { + count: 1, + next: null, + previous: null, + results: [ + { + bytes_signature: '¢,´e', + created_at: '2018-04-11T21:47:39.980645Z', + hex_signature: '0xa22cb465', + id: 29659, + text_signature: 'setApprovalForAll(address,bool)', + }, + ], + }, + })); +} + +async function createTransactionAndAssertDetails( + driver: Driver, + contractRegistry?: GanacheContractAddressRegistry, +) { + await unlockWallet(driver); + + const contractAddress = await ( + contractRegistry as GanacheContractAddressRegistry + ).getContractAddress(SMART_CONTRACTS.NFTS); + + const testDapp = new TestDapp(driver); + + await testDapp.open({ contractAddress, url: DAPP_URL }); + + await testDapp.clickERC721RevokeSetApprovalForAllButton(); + + await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); + + const setApprovalForAllConfirmation = + new SetApprovalForAllTransactionConfirmation(driver); + + await setApprovalForAllConfirmation.check_revokeSetApprovalForAllTitle(); + + await setApprovalForAllConfirmation.clickScrollToBottomButton(); + await setApprovalForAllConfirmation.clickFooterConfirmButton(); +} diff --git a/ui/pages/confirmations/components/confirm/info/approve/__snapshots__/approve.test.tsx.snap b/ui/pages/confirmations/components/confirm/info/approve/__snapshots__/approve.test.tsx.snap index d508fb0d4eca..648ecff92c1a 100644 --- a/ui/pages/confirmations/components/confirm/info/approve/__snapshots__/approve.test.tsx.snap +++ b/ui/pages/confirmations/components/confirm/info/approve/__snapshots__/approve.test.tsx.snap @@ -71,7 +71,7 @@ exports[` renders component for approve request 1`] = ` style="margin-left: auto; max-width: 100%;" >
renders component 1`] = ` style="margin-left: auto; max-width: 100%;" >
{ ); const simulationElements = ( - - - {spendingCap === SPENDING_CAP_UNLIMITED_MSG ? ( - {formattedTokenText} - ) : ( - formattedTokenText - )} + + + + + {spendingCap === SPENDING_CAP_UNLIMITED_MSG ? ( + + {formattedTokenText} + + ) : ( + formattedTokenText + )} + + + - - + ); return ( @@ -93,9 +102,6 @@ export const ApproveStaticSimulation = () => { ? 'simulationDetailsApproveDesc' : 'simulationDetailsERC20ApproveDesc', )} - simulationHeading={ - isNFT ? t('simulationApproveHeading') : t('spendingCap') - } simulationElements={simulationElements} /> ); diff --git a/ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts b/ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts index 9f60ef255acb..f0fcd713653a 100644 --- a/ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts +++ b/ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts @@ -10,7 +10,7 @@ export const useIsNFT = ( return await getTokenStandardAndDetails( transactionMeta?.txParams?.to as string, ); - }, [transactionMeta]); + }, [transactionMeta?.txParams?.to]); const isNFT = value?.standard !== TokenStandard.ERC20; diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/__snapshots__/revoke-set-approval-for-all-static-simulation.test.tsx.snap b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/__snapshots__/revoke-set-approval-for-all-static-simulation.test.tsx.snap new file mode 100644 index 000000000000..37e785040b0f --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/__snapshots__/revoke-set-approval-for-all-static-simulation.test.tsx.snap @@ -0,0 +1,138 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` renders component for setApprovalForAll request 1`] = ` +
+
+
+
+
+

+ Estimated changes +

+
+
+ +
+
+
+
+
+

+ You're removing someone else's permission to withdraw NFTs from your account. +

+
+
+
+
+
+

+ NFTs +

+
+
+
+
+
+
+ +

+ 0x07614...3ad68 +

+
+
+
+
+
+
+
+
+

+ Permission from +

+
+
+
+
+
+
+ +

+ 0x9bc5b...AfEF4 +

+
+
+
+
+
+
+
+`; diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.stories.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.stories.tsx new file mode 100644 index 000000000000..519c5e3cc6b9 --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.stories.tsx @@ -0,0 +1,34 @@ +import { Meta } from '@storybook/react'; +import React from 'react'; +import { Provider } from 'react-redux'; +import { getMockApproveConfirmState } from '../../../../../../../../test/data/confirmations/helper'; +import configureStore from '../../../../../../../store/store'; +import { ConfirmContextProvider } from '../../../../../context/confirm'; +import { RevokeSetApprovalForAllStaticSimulation } from './revoke-set-approval-for-all-static-simulation'; + +const store = configureStore(getMockApproveConfirmState()); + +const Story = { + title: + 'Pages/Confirmations/Components/Confirm/Info/SetApprovalForAll/RevokeSetApprovalForAllStaticSimulation', + component: RevokeSetApprovalForAllStaticSimulation, + decorators: [ + (story: () => Meta) => ( + + {story()} + + ), + ], +}; + +export default Story; + +export const DefaultStory = (args) => ( + +); + +DefaultStory.storyName = 'Default'; + +DefaultStory.args = { + spender: '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4', +}; diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.test.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.test.tsx new file mode 100644 index 000000000000..565ff5f8d16d --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.test.tsx @@ -0,0 +1,25 @@ +import React from 'react'; +import configureMockStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { getMockSetApprovalForAllConfirmState } from '../../../../../../../../test/data/confirmations/helper'; +import { renderWithConfirmContextProvider } from '../../../../../../../../test/lib/confirmations/render-helpers'; +import { RevokeSetApprovalForAllStaticSimulation } from './revoke-set-approval-for-all-static-simulation'; + +describe('', () => { + const middleware = [thunk]; + + it('renders component for setApprovalForAll request', async () => { + const state = getMockSetApprovalForAllConfirmState(); + + const mockStore = configureMockStore(middleware)(state); + + const testSpender = '0x9bc5baF874d2DA8D216aE9f137804184EE5AfEF4'; + + const { container } = renderWithConfirmContextProvider( + , + mockStore, + ); + + expect(container).toMatchSnapshot(); + }); +}); diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.tsx new file mode 100644 index 000000000000..7cc141fb64e5 --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation.tsx @@ -0,0 +1,65 @@ +import { NameType } from '@metamask/name-controller'; +import { TransactionMeta } from '@metamask/transaction-controller'; +import React from 'react'; +import { ConfirmInfoRow } from '../../../../../../../components/app/confirm/info/row'; +import Name from '../../../../../../../components/app/name'; +import { Box } from '../../../../../../../components/component-library'; +import { + AlignItems, + Display, +} from '../../../../../../../helpers/constants/design-system'; +import { useI18nContext } from '../../../../../../../hooks/useI18nContext'; +import { useConfirmContext } from '../../../../../context/confirm'; +import StaticSimulation from '../../shared/static-simulation/static-simulation'; + +export const RevokeSetApprovalForAllStaticSimulation = ({ + spender, +}: { + spender: string; +}) => { + const t = useI18nContext(); + + const { currentConfirmation: transactionMeta } = + useConfirmContext(); + + const nftsRow = ( + + + + + + + + ); + + const permissionFromRow = ( + + + + + + + + ); + + const RevokeSetApprovalForAllRows = ( + <> + {nftsRow} + {permissionFromRow} + + ); + + const simulationElements = RevokeSetApprovalForAllRows; + + return ( + + ); +}; diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.stories.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.stories.tsx new file mode 100644 index 000000000000..9a870efc4d59 --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.stories.tsx @@ -0,0 +1,27 @@ +import { Meta } from '@storybook/react'; +import React from 'react'; +import { Provider } from 'react-redux'; +import { getMockApproveConfirmState } from '../../../../../../../test/data/confirmations/helper'; +import configureStore from '../../../../../../store/store'; +import { ConfirmContextProvider } from '../../../../context/confirm'; +import SetApprovalForAll from './set-approval-for-all-info'; + +const store = configureStore(getMockApproveConfirmState()); + +const Story = { + title: 'Components/App/Confirm/info/SetApprovalForAll', + component: SetApprovalForAll, + decorators: [ + (story: () => Meta) => ( + + {story()} + + ), + ], +}; + +export default Story; + +export const DefaultStory = () => ; + +DefaultStory.storyName = 'Default'; diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.tsx index 9ed773e5a350..6a2c98f224e2 100644 --- a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.tsx +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-info.tsx @@ -4,8 +4,11 @@ import { useSelector } from 'react-redux'; import { useConfirmContext } from '../../../../context/confirm'; import { selectConfirmationAdvancedDetailsOpen } from '../../../../selectors/preferences'; import { ApproveDetails } from '../approve/approve-details/approve-details'; +import { useDecodedTransactionData } from '../hooks/useDecodedTransactionData'; import { AdvancedDetails } from '../shared/advanced-details/advanced-details'; import { GasFeesSection } from '../shared/gas-fees-section/gas-fees-section'; +import { getIsRevokeSetApprovalForAll } from '../utils'; +import { RevokeSetApprovalForAllStaticSimulation } from './revoke-set-approval-for-all-static-simulation/revoke-set-approval-for-all-static-simulation'; import { SetApprovalForAllStaticSimulation } from './set-approval-for-all-static-simulation/set-approval-for-all-static-simulation'; const SetApprovalForAllInfo = () => { @@ -16,13 +19,25 @@ const SetApprovalForAllInfo = () => { selectConfirmationAdvancedDetailsOpen, ); + const decodedResponse = useDecodedTransactionData(); + + const { value } = decodedResponse; + + const isRevokeSetApprovalForAll = getIsRevokeSetApprovalForAll(value); + + const spender = value?.data[0].params[0].value; + if (!transactionMeta?.txParams) { return null; } return ( <> - + {isRevokeSetApprovalForAll ? ( + + ) : ( + + )} {showAdvancedDetails && } diff --git a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-static-simulation/set-approval-for-all-static-simulation.tsx b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-static-simulation/set-approval-for-all-static-simulation.tsx index 89686f4faa7d..c50d10094486 100644 --- a/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-static-simulation/set-approval-for-all-static-simulation.tsx +++ b/ui/pages/confirmations/components/confirm/info/set-approval-for-all-info/set-approval-for-all-static-simulation/set-approval-for-all-static-simulation.tsx @@ -1,6 +1,7 @@ import { NameType } from '@metamask/name-controller'; import { TransactionMeta } from '@metamask/transaction-controller'; import React from 'react'; +import { ConfirmInfoRow } from '../../../../../../../components/app/confirm/info/row'; import Name from '../../../../../../../components/app/name'; import { Box, Text } from '../../../../../../../components/component-library'; import { @@ -18,33 +19,38 @@ import StaticSimulation from '../../shared/static-simulation/static-simulation'; export const SetApprovalForAllStaticSimulation = () => { const t = useI18nContext(); - const { currentConfirmation: transactionMeta } = useConfirmContext() as { - currentConfirmation: TransactionMeta; - }; + const { currentConfirmation: transactionMeta } = + useConfirmContext() as { + currentConfirmation: TransactionMeta; + }; const SetApprovalForAllRow = ( - - - - {t('all')} - + + + + + + {t('all')} + + + + - - + ); const simulationElements = SetApprovalForAllRow; @@ -54,7 +60,6 @@ export const SetApprovalForAllStaticSimulation = () => { title={t('simulationDetailsTitle')} titleTooltip={t('simulationDetailsTitleTooltip')} description={t('simulationDetailsSetApprovalForAllDesc')} - simulationHeading={t('withdrawing')} simulationElements={simulationElements} /> ); diff --git a/ui/pages/confirmations/components/confirm/info/shared/static-simulation/static-simulation.tsx b/ui/pages/confirmations/components/confirm/info/shared/static-simulation/static-simulation.tsx index 3414a1aee94a..7901854b91d1 100644 --- a/ui/pages/confirmations/components/confirm/info/shared/static-simulation/static-simulation.tsx +++ b/ui/pages/confirmations/components/confirm/info/shared/static-simulation/static-simulation.tsx @@ -4,31 +4,19 @@ import { ConfirmInfoRowText, } from '../../../../../../../components/app/confirm/info/row'; import { ConfirmInfoSection } from '../../../../../../../components/app/confirm/info/row/section'; -import { Box } from '../../../../../../../components/component-library'; const StaticSimulation: React.FC<{ title: string; titleTooltip: string; description: string; - simulationHeading: string; simulationElements: React.ReactNode; -}> = ({ - title, - titleTooltip, - description, - simulationHeading, - simulationElements, -}) => { +}> = ({ title, titleTooltip, description, simulationElements }) => { return ( - - - {simulationElements} - - + {simulationElements} ); }; diff --git a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx index 7953b23a43da..231997d18547 100644 --- a/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx +++ b/ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx @@ -1,6 +1,8 @@ +import { Hex } from '@metamask/utils'; import React from 'react'; import { PrimaryType } from '../../../../../../../../shared/constants/signatures'; import { parseTypedDataMessage } from '../../../../../../../../shared/modules/transaction.utils'; +import { ConfirmInfoRow } from '../../../../../../../components/app/confirm/info/row'; import { Box } from '../../../../../../../components/component-library'; import { Display, @@ -48,14 +50,27 @@ const PermitSimulation: React.FC = () => { const tokenDetails = extractTokenDetailsByPrimaryType(message, primaryType); - return ( - ( + + ); + + const SpendingCapRow = ( + + + {Array.isArray(tokenDetails) ? ( = () => { { token, amount }: { token: string; amount: string }, i: number, ) => ( - + ), )} @@ -80,8 +90,17 @@ const PermitSimulation: React.FC = () => { tokenContract={verifyingContract} value={message.value} /> - ) - } + )} + + + ); + + return ( + ); }; diff --git a/ui/pages/confirmations/components/confirm/info/utils.test.ts b/ui/pages/confirmations/components/confirm/info/utils.test.ts new file mode 100644 index 000000000000..e78d06d87622 --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/utils.test.ts @@ -0,0 +1,38 @@ +import { DecodedTransactionDataSource } from '../../../../../../shared/types/transaction-decode'; +import { getIsRevokeSetApprovalForAll } from './utils'; + +describe('getIsRevokeSetApprovalForAll', () => { + it('returns false if no data is passed as an argument', () => { + const testValue = { + data: [], + source: DecodedTransactionDataSource.FourByte, + }; + const actual = getIsRevokeSetApprovalForAll(testValue); + + expect(actual).toEqual(false); + }); + + it('returns true if no setApprovalForAll decoded tx is passed as an argument', () => { + const testValue = { + data: [ + { + name: 'setApprovalForAll', + params: [ + { + type: 'address', + value: '0x2e0D7E8c45221FcA00d74a3609A0f7097035d09B', + }, + { + type: 'boolean', + value: false, + }, + ], + }, + ], + source: DecodedTransactionDataSource.FourByte, + }; + const actual = getIsRevokeSetApprovalForAll(testValue); + + expect(actual).toEqual(true); + }); +}); diff --git a/ui/pages/confirmations/components/confirm/info/utils.ts b/ui/pages/confirmations/components/confirm/info/utils.ts new file mode 100644 index 000000000000..a80d604fed83 --- /dev/null +++ b/ui/pages/confirmations/components/confirm/info/utils.ts @@ -0,0 +1,11 @@ +import { DecodedTransactionDataResponse } from '../../../../../../shared/types/transaction-decode'; + +export function getIsRevokeSetApprovalForAll( + value: DecodedTransactionDataResponse | undefined, +): boolean { + const isRevokeSetApprovalForAll = + value?.data?.[0]?.name === 'setApprovalForAll' && + value?.data?.[0]?.params?.[1]?.value === false; + + return isRevokeSetApprovalForAll; +} diff --git a/ui/pages/confirmations/components/confirm/title/title.tsx b/ui/pages/confirmations/components/confirm/title/title.tsx index 94acee877900..4fa3119c4802 100644 --- a/ui/pages/confirmations/components/confirm/title/title.tsx +++ b/ui/pages/confirmations/components/confirm/title/title.tsx @@ -4,6 +4,8 @@ import { } from '@metamask/transaction-controller'; import React, { memo, useMemo } from 'react'; +import GeneralAlert from '../../../../../components/app/alert-system/general-alert/general-alert'; +import { getHighestSeverity } from '../../../../../components/app/alert-system/utils'; import { Box, Text } from '../../../../../components/component-library'; import { TextAlign, @@ -12,15 +14,15 @@ import { } from '../../../../../helpers/constants/design-system'; import useAlerts from '../../../../../hooks/useAlerts'; import { useI18nContext } from '../../../../../hooks/useI18nContext'; -import { getHighestSeverity } from '../../../../../components/app/alert-system/utils'; -import GeneralAlert from '../../../../../components/app/alert-system/general-alert/general-alert'; +import { useConfirmContext } from '../../../context/confirm'; import { Confirmation, SignatureRequestType } from '../../../types/confirm'; import { isPermitSignatureRequest, isSIWESignatureRequest, } from '../../../utils'; -import { useConfirmContext } from '../../../context/confirm'; import { useIsNFT } from '../info/approve/hooks/use-is-nft'; +import { useDecodedTransactionData } from '../info/hooks/useDecodedTransactionData'; +import { getIsRevokeSetApprovalForAll } from '../info/utils'; function ConfirmBannerAlert({ ownerId }: { ownerId: string }) { const t = useI18nContext(); @@ -64,6 +66,7 @@ const getTitle = ( t: IntlFunction, confirmation?: Confirmation, isNFT?: boolean, + isRevokeSetApprovalForAll?: boolean, ) => { switch (confirmation?.type) { case TransactionType.contractInteraction: @@ -87,6 +90,9 @@ const getTitle = ( case TransactionType.tokenMethodIncreaseAllowance: return t('confirmTitlePermitTokens'); case TransactionType.tokenMethodSetApprovalForAll: + if (isRevokeSetApprovalForAll) { + return t('confirmTitleSetApprovalForAllRevokeTransaction'); + } return t('setApprovalForAllRedesignedTitle'); default: return ''; @@ -97,6 +103,7 @@ const getDescription = ( t: IntlFunction, confirmation?: Confirmation, isNFT?: boolean, + isRevokeSetApprovalForAll?: boolean, ) => { switch (confirmation?.type) { case TransactionType.contractInteraction: @@ -120,7 +127,11 @@ const getDescription = ( case TransactionType.tokenMethodIncreaseAllowance: return t('confirmTitleDescPermitSignature'); case TransactionType.tokenMethodSetApprovalForAll: + if (isRevokeSetApprovalForAll) { + return ''; + } return t('confirmTitleDescApproveTransaction'); + default: return ''; } @@ -132,14 +143,37 @@ const ConfirmTitle: React.FC = memo(() => { const { isNFT } = useIsNFT(currentConfirmation as TransactionMeta); + let isRevokeSetApprovalForAll = false; + if ( + currentConfirmation?.type === TransactionType.tokenMethodSetApprovalForAll + ) { + const decodedResponse = useDecodedTransactionData(); + + isRevokeSetApprovalForAll = getIsRevokeSetApprovalForAll( + decodedResponse.value, + ); + } + const title = useMemo( - () => getTitle(t as IntlFunction, currentConfirmation, isNFT), - [currentConfirmation, isNFT], + () => + getTitle( + t as IntlFunction, + currentConfirmation, + isNFT, + isRevokeSetApprovalForAll, + ), + [currentConfirmation, isNFT, isRevokeSetApprovalForAll], ); const description = useMemo( - () => getDescription(t as IntlFunction, currentConfirmation, isNFT), - [currentConfirmation, isNFT], + () => + getDescription( + t as IntlFunction, + currentConfirmation, + isNFT, + isRevokeSetApprovalForAll, + ), + [currentConfirmation, isNFT, isRevokeSetApprovalForAll], ); if (!currentConfirmation) { diff --git a/ui/pages/confirmations/confirm/__snapshots__/confirm.test.tsx.snap b/ui/pages/confirmations/confirm/__snapshots__/confirm.test.tsx.snap index 460f5097af1f..30a1b6ad118c 100644 --- a/ui/pages/confirmations/confirm/__snapshots__/confirm.test.tsx.snap +++ b/ui/pages/confirmations/confirm/__snapshots__/confirm.test.tsx.snap @@ -295,7 +295,7 @@ exports[`Confirm should match snapshot for signature - typed sign - V4 - PermitB >
Date: Wed, 25 Sep 2024 13:59:14 +0100 Subject: [PATCH 4/6] fix: incorrect method name parsed from transaction data (#27363) Method data derived for the metrics is only persisted if a result is found. Queries to 4Byte are skipped if the transaction data is `0x` or less than four bytes. Transaction data is not rendered in the legacy or redesigned confirmations if it is `0x`. The `getKnownMethodData` selector only returns a value if transaction data is at least four bytes. The unused `useKnownMethodDataInTransaction` hook was removed. --- app/scripts/lib/util.test.js | 24 +++++- app/scripts/lib/util.ts | 2 +- shared/lib/four-byte.test.ts | 19 +++++ shared/lib/four-byte.ts | 12 ++- shared/modules/transaction.utils.test.js | 17 ++++ shared/modules/transaction.utils.ts | 7 ++ .../confirm-hexdata/confirm-hexdata.js | 3 +- .../confirm-hexdata/confirm-hexdata.test.js | 33 ++++---- .../hooks/useDecodedTransactionData.test.ts | 34 ++++---- .../info/hooks/useDecodedTransactionData.ts | 3 +- .../confirm/info/hooks/useFourByte.test.ts | 4 +- .../useKnownMethodDataInTransaction.test.ts | 80 ------------------- .../hooks/useKnownMethodDataInTransaction.ts | 21 ----- .../transaction-data/transaction-data.tsx | 3 +- .../confirm-transaction-base.component.js | 3 +- .../token-allowance/token-allowance.js | 4 +- ui/selectors/selectors.js | 15 +++- 17 files changed, 139 insertions(+), 145 deletions(-) delete mode 100644 ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts delete mode 100644 ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts diff --git a/app/scripts/lib/util.test.js b/app/scripts/lib/util.test.js index d428c3cd7ae1..7ae7e7b36a88 100644 --- a/app/scripts/lib/util.test.js +++ b/app/scripts/lib/util.test.js @@ -369,21 +369,25 @@ describe('app utils', () => { name: 'Approve Tokens', }, }; + it('return null if use4ByteResolution is not true', async () => { expect( await getMethodDataName(knownMethodData, false, '0x60806040'), ).toStrictEqual(null); }); + it('return null if prefixedData is not defined', async () => { expect( await getMethodDataName(knownMethodData, true, undefined), ).toStrictEqual(null); }); + it('return details from knownMethodData if defined', async () => { expect( await getMethodDataName(knownMethodData, true, '0x60806040'), ).toStrictEqual(knownMethodData['0x60806040']); }); + it('invoke getMethodDataAsync if details not available in knownMethodData', async () => { const DUMMY_METHOD_NAME = { name: 'Dummy Method Name', @@ -392,9 +396,10 @@ describe('app utils', () => { .spyOn(FourBiteUtils, 'getMethodDataAsync') .mockResolvedValue(DUMMY_METHOD_NAME); expect( - await getMethodDataName(knownMethodData, true, '0x123'), + await getMethodDataName(knownMethodData, true, '0x123', jest.fn()), ).toStrictEqual(DUMMY_METHOD_NAME); }); + it('invoke addKnownMethodData if details not available in knownMethodData', async () => { const DUMMY_METHOD_NAME = { name: 'Dummy Method Name', @@ -413,5 +418,22 @@ describe('app utils', () => { ).toStrictEqual(DUMMY_METHOD_NAME); expect(addKnownMethodData).toHaveBeenCalledTimes(1); }); + + it('does not invoke addKnownMethodData if no method data available', async () => { + const addKnownMethodData = jest.fn(); + + jest.spyOn(FourBiteUtils, 'getMethodDataAsync').mockResolvedValue({}); + + expect( + await getMethodDataName( + knownMethodData, + true, + '0x123', + addKnownMethodData, + ), + ).toStrictEqual({}); + + expect(addKnownMethodData).toHaveBeenCalledTimes(0); + }); }); }); diff --git a/app/scripts/lib/util.ts b/app/scripts/lib/util.ts index 2caade79b83e..e41b2a00b670 100644 --- a/app/scripts/lib/util.ts +++ b/app/scripts/lib/util.ts @@ -411,7 +411,7 @@ export const getMethodDataName = async ( provider, ); - if (addKnownMethodData) { + if (methodData?.name) { addKnownMethodData(fourBytePrefix, methodData as MethodData); } diff --git a/shared/lib/four-byte.test.ts b/shared/lib/four-byte.test.ts index e25235e6ae54..2867aa2e51b7 100644 --- a/shared/lib/four-byte.test.ts +++ b/shared/lib/four-byte.test.ts @@ -25,6 +25,25 @@ describe('Four Byte', () => { expect(result).toStrictEqual('someOtherFunction(address,uint256)'); }); + + // @ts-expect-error This is missing from the Mocha type definitions + it.each([undefined, null, '', '0x', '0X'])( + 'returns undefined if four byte prefix is %s', + async (prefix: string) => { + expect(await getMethodFrom4Byte(prefix)).toBeUndefined(); + }, + ); + + // @ts-expect-error This is missing from the Mocha type definitions + it.each([ + ['with hex prefix', '0x1234567'], + ['without hex prefix', '1234567'], + ])( + 'returns undefined if length of four byte prefix %s is less than 8', + async (_: string, prefix: string) => { + expect(await getMethodFrom4Byte(prefix)).toBeUndefined(); + }, + ); }); describe('getMethodDataAsync', () => { diff --git a/shared/lib/four-byte.ts b/shared/lib/four-byte.ts index c8d67af7b1f4..e28f4d4c0c5c 100644 --- a/shared/lib/four-byte.ts +++ b/shared/lib/four-byte.ts @@ -1,4 +1,7 @@ import { MethodRegistry } from 'eth-method-registry'; +import { Hex } from '@metamask/utils'; +import { hasTransactionData } from '../modules/transaction.utils'; +import { stripHexPrefix } from '../modules/hexstring-utils'; import fetchWithCache from './fetch-with-cache'; type FourByteResult = { @@ -12,7 +15,14 @@ type FourByteResponse = { export async function getMethodFrom4Byte( fourBytePrefix: string, -): Promise { +): Promise { + if ( + !hasTransactionData(fourBytePrefix as Hex) || + stripHexPrefix(fourBytePrefix)?.length < 8 + ) { + return undefined; + } + const fourByteResponse = (await fetchWithCache({ url: `https://www.4byte.directory/api/v1/signatures/?hex_signature=${fourBytePrefix}`, fetchOptions: { diff --git a/shared/modules/transaction.utils.test.js b/shared/modules/transaction.utils.test.js index 133f2de9141e..28bfaaa34ed7 100644 --- a/shared/modules/transaction.utils.test.js +++ b/shared/modules/transaction.utils.test.js @@ -4,6 +4,7 @@ import { TransactionType } from '@metamask/transaction-controller'; import { createTestProviderTools } from '../../test/stub/provider'; import { determineTransactionType, + hasTransactionData, isEIP1559Transaction, isLegacyTransaction, parseStandardTokenTransactionData, @@ -417,4 +418,20 @@ describe('Transaction.utils', function () { }); }); }); + + describe('hasTransactionData', () => { + it.each([ + ['has prefix', '0x1234'], + ['has no prefix', '1234'], + ])('returns true if data %s', (_, data) => { + expect(hasTransactionData(data)).toBe(true); + }); + + it.each([undefined, null, '', '0x', '0X'])( + 'returns false if data is %s', + (data) => { + expect(hasTransactionData(data)).toBe(false); + }, + ); + }); }); diff --git a/shared/modules/transaction.utils.ts b/shared/modules/transaction.utils.ts index e09680c6c4bd..910fe9b6d4be 100644 --- a/shared/modules/transaction.utils.ts +++ b/shared/modules/transaction.utils.ts @@ -14,6 +14,7 @@ import { } from '@metamask/transaction-controller'; import type { TransactionParams } from '@metamask/transaction-controller'; +import { Hex } from '@metamask/utils'; import { AssetType, TokenStandard } from '../constants/transaction'; import { readAddressAsContract } from './contract-utils'; import { isEqualCaseInsensitive } from './string-utils'; @@ -319,3 +320,9 @@ export const parseTypedDataMessage = (dataToParse: string) => { return result; }; + +export function hasTransactionData(transactionData?: Hex): boolean { + return Boolean( + transactionData?.length && transactionData?.toLowerCase?.() !== '0x', + ); +} diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js index 1309d7e7e618..8108cac67a0b 100644 --- a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js +++ b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.js @@ -15,6 +15,7 @@ import { import Box from '../../../../components/ui/box'; import { Text } from '../../../../components/component-library'; import CopyRawData from '../transaction-decoding/components/ui/copy-raw-data'; +import { hasTransactionData } from '../../../../../shared/modules/transaction.utils'; const ConfirmHexData = ({ txData, dataHexComponent }) => { const t = useI18nContext(); @@ -28,7 +29,7 @@ const ConfirmHexData = ({ txData, dataHexComponent }) => { return dataHexComponent; } - if (!txParams.data || !txParams.to) { + if (!hasTransactionData(txParams.data) || !txParams.to) { return null; } diff --git a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js index f9f41566c4c0..d31478775ba2 100644 --- a/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js +++ b/ui/pages/confirmations/components/confirm-hexdata/confirm-hexdata.test.js @@ -28,21 +28,24 @@ describe('ConfirmHexData', () => { expect(await findByText('Transfer')).toBeInTheDocument(); }); - it('should return null if transaction has no data', () => { - const { container } = renderWithProvider( - , - store, - ); - expect(container.firstChild).toStrictEqual(null); - }); + it.each([undefined, null, '', '0x', '0X'])( + 'should return null if transaction data is %s', + (data) => { + const { container } = renderWithProvider( + , + store, + ); + expect(container.firstChild).toStrictEqual(null); + }, + ); it('should return null if transaction has no to address', () => { const { container } = renderWithProvider( diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts index 19ccc1ac1976..35f2f42c4792 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.test.ts @@ -38,22 +38,26 @@ async function runHook(state: Record) { describe('useDecodedTransactionData', () => { const decodeTransactionDataMock = jest.mocked(decodeTransactionData); - it('returns undefined if no transaction data', async () => { - const result = await runHook( - getMockConfirmStateForTransaction({ - id: '123', - chainId: CHAIN_ID_MOCK, - type: TransactionType.contractInteraction, - status: TransactionStatus.unapproved, - txParams: { - data: '', - to: CONTRACT_ADDRESS_MOCK, - } as TransactionParams, - }), - ); + // @ts-expect-error This is missing from the Mocha type definitions + it.each([undefined, null, '', '0x', '0X'])( + 'returns undefined if transaction data is %s', + async (data: string) => { + const result = await runHook( + getMockConfirmStateForTransaction({ + id: '123', + chainId: CHAIN_ID_MOCK, + type: TransactionType.contractInteraction, + status: TransactionStatus.unapproved, + txParams: { + data, + to: CONTRACT_ADDRESS_MOCK, + } as TransactionParams, + }), + ); - expect(result).toStrictEqual({ pending: false, value: undefined }); - }); + expect(result).toStrictEqual({ pending: false, value: undefined }); + }, + ); it('returns the decoded data', async () => { decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts index 3515ad2503f5..b2d69df413d4 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useDecodedTransactionData.ts @@ -8,6 +8,7 @@ import { import { decodeTransactionData } from '../../../../../../store/actions'; import { DecodedTransactionDataResponse } from '../../../../../../../shared/types/transaction-decode'; import { useConfirmContext } from '../../../../context/confirm'; +import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils'; export function useDecodedTransactionData(): AsyncResult< DecodedTransactionDataResponse | undefined @@ -19,7 +20,7 @@ export function useDecodedTransactionData(): AsyncResult< const transactionData = currentConfirmation?.txParams?.data as Hex; return useAsyncResult(async () => { - if (!transactionData?.length) { + if (!hasTransactionData(transactionData)) { return undefined; } diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts index bdff31ac33e6..5d4a023c82bb 100644 --- a/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts +++ b/ui/pages/confirmations/components/confirm/info/hooks/useFourByte.test.ts @@ -54,7 +54,7 @@ describe('useFourByte', () => { }, ); - expect(result.current).toEqual({}); + expect(result.current).toBeNull(); }); it("returns undefined if it's not known even if resolution is enabled", () => { @@ -75,6 +75,6 @@ describe('useFourByte', () => { }, ); - expect(result.current).toEqual({}); + expect(result.current).toBeNull(); }); }); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts b/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts deleted file mode 100644 index bdff31ac33e6..000000000000 --- a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { TransactionMeta } from '@metamask/transaction-controller'; -import { - CONTRACT_INTERACTION_SENDER_ADDRESS, - genUnapprovedContractInteractionConfirmation, -} from '../../../../../../../test/data/confirmations/contract-interaction'; -import mockState from '../../../../../../../test/data/mock-state.json'; -import { renderHookWithProvider } from '../../../../../../../test/lib/render-helpers'; -import { useFourByte } from './useFourByte'; - -describe('useFourByte', () => { - const depositHexData = '0xd0e30db0'; - - it('returns the method name and params', () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: true, - knownMethodData: { - [depositHexData]: { name: 'Deposit', params: [] }, - }, - }, - }, - ); - - expect(result.current.name).toEqual('Deposit'); - expect(result.current.params).toEqual([]); - }); - - it('returns empty object if resolution is turned off', () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: false, - knownMethodData: { - [depositHexData]: { name: 'Deposit', params: [] }, - }, - }, - }, - ); - - expect(result.current).toEqual({}); - }); - - it("returns undefined if it's not known even if resolution is enabled", () => { - const currentConfirmation = genUnapprovedContractInteractionConfirmation({ - address: CONTRACT_INTERACTION_SENDER_ADDRESS, - txData: depositHexData, - }) as TransactionMeta; - - const { result } = renderHookWithProvider( - () => useFourByte(currentConfirmation), - { - ...mockState, - metamask: { - ...mockState.metamask, - use4ByteResolution: true, - knownMethodData: {}, - }, - }, - ); - - expect(result.current).toEqual({}); - }); -}); diff --git a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts b/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts deleted file mode 100644 index 821fbeaa9b5a..000000000000 --- a/ui/pages/confirmations/components/confirm/info/hooks/useKnownMethodDataInTransaction.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { TransactionMeta } from '@metamask/transaction-controller'; -import { useDispatch, useSelector } from 'react-redux'; -import { - getKnownMethodData, - use4ByteResolutionSelector, -} from '../../../../../../selectors'; -import { getContractMethodData } from '../../../../../../store/actions'; - -export const useKnownMethodDataInTransaction = ( - currentConfirmation: TransactionMeta, -) => { - const dispatch = useDispatch(); - const use4ByteResolution = useSelector(use4ByteResolutionSelector); - const transactionData = currentConfirmation?.txParams?.data; - if (use4ByteResolution && transactionData) { - dispatch(getContractMethodData(currentConfirmation.txParams.data)); - } - const knownMethodData = - useSelector((state) => getKnownMethodData(state, transactionData)) || {}; - return { knownMethodData }; -}; diff --git a/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx b/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx index b8c3a44078f1..4af44b5a310d 100644 --- a/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx +++ b/ui/pages/confirmations/components/confirm/info/shared/transaction-data/transaction-data.tsx @@ -29,6 +29,7 @@ import { // eslint-disable-next-line import/no-restricted-paths import { UniswapPathPool } from '../../../../../../../../app/scripts/lib/transaction/decode/uniswap'; import { useConfirmContext } from '../../../../../context/confirm'; +import { hasTransactionData } from '../../../../../../../../shared/modules/transaction.utils'; export const TransactionData = () => { const { currentConfirmation } = useConfirmContext(); @@ -42,7 +43,7 @@ export const TransactionData = () => { return ; } - if (!transactionData?.length) { + if (!hasTransactionData(transactionData)) { return null; } diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js index 2cdb9a419aa1..4bc313f6503b 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js @@ -73,6 +73,7 @@ import FeeDetailsComponent from '../components/fee-details-component/fee-details import { SimulationDetails } from '../components/simulation-details'; import { fetchSwapsFeatureFlags } from '../../swaps/swaps.util'; import { NetworkChangeToastLegacy } from '../components/confirm/network-change-toast'; +import { hasTransactionData } from '../../../../shared/modules/transaction.utils'; export default class ConfirmTransactionBase extends Component { static contextTypes = { @@ -640,7 +641,7 @@ export default class ConfirmTransactionBase extends Component { const { txParams: { data }, } = txData; - if (!data) { + if (!hasTransactionData(data)) { return null; } return ( diff --git a/ui/pages/confirmations/token-allowance/token-allowance.js b/ui/pages/confirmations/token-allowance/token-allowance.js index 123cd1354cc6..8ccd19ecb566 100644 --- a/ui/pages/confirmations/token-allowance/token-allowance.js +++ b/ui/pages/confirmations/token-allowance/token-allowance.js @@ -212,7 +212,9 @@ export default function TokenAllowance({ } const fee = useSelector((state) => transactionFeeSelector(state, fullTxData)); - const methodData = useSelector((state) => getKnownMethodData(state, data)); + const methodData = useSelector( + (state) => getKnownMethodData(state, data) ?? {}, + ); const { balanceError } = useGasFeeContext(); diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index 64346a655dfd..bdc1547b7246 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -107,6 +107,7 @@ import { MultichainNativeAssets } from '../../shared/constants/multichain/assets // TODO: Remove restricted import // eslint-disable-next-line import/no-restricted-paths import { BridgeFeatureFlagsKey } from '../../app/scripts/controllers/bridge/types'; +import { hasTransactionData } from '../../shared/modules/transaction.utils'; import { getAllUnapprovedTransactions, getCurrentNetworkTransactions, @@ -1164,14 +1165,20 @@ export function getRpcPrefsForCurrentProvider(state) { } export function getKnownMethodData(state, data) { - if (!data) { + const { knownMethodData, use4ByteResolution } = state.metamask; + + if (!use4ByteResolution || !hasTransactionData(data)) { return null; } + const prefixedData = addHexPrefix(data); const fourBytePrefix = prefixedData.slice(0, 10); - const { knownMethodData, use4ByteResolution } = state.metamask; - // If 4byte setting is off, we do not want to return the knownMethodData - return use4ByteResolution ? knownMethodData?.[fourBytePrefix] ?? {} : {}; + + if (fourBytePrefix.length < 10) { + return null; + } + + return knownMethodData?.[fourBytePrefix] ?? null; } export function getFeatureFlags(state) { From 7054c1615fbb3ff6fcbf33704e63bdc237f0f6fb Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 25 Sep 2024 15:33:06 +0200 Subject: [PATCH 5/6] test: [POM] Migrate Snap Account Settings to page object model (#27302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This pull request migrate snap account settings test to page object model(POM) pattern, enhancing code maintainability, and improving test reliability. Also improved the implementation of login methods so we query directly ganache, and there's no need to pass any balance manually. ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27155?quickstart=1) ## **Related issues** Fixes: #27343 ## **Manual testing steps** Check code readability, make sure tests pass. ## **Screenshots/Recordings** ### **Before** ### **After** ## **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. --------- Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Chloe Gao Co-authored-by: chloeYue <105063779+chloeYue@users.noreply.github.com> --- .../accounts/snap-account-settings.spec.ts | 62 --------- test/e2e/page-objects/flows/login.flow.ts | 38 ++++-- .../page-objects/pages/account-list-page.ts | 119 +++++++++++------- .../pages/experimental-settings.ts | 38 ++++++ test/e2e/page-objects/pages/header-navbar.ts | 13 +- test/e2e/page-objects/pages/homepage.ts | 55 ++++---- test/e2e/page-objects/pages/login-page.ts | 12 +- test/e2e/page-objects/pages/settings-page.ts | 37 ++++++ .../tests/account/account-custom-name.spec.ts | 3 +- .../tests/account/account-hide-unhide.spec.ts | 3 +- .../tests/account/account-pin-unpin.spec.ts | 4 +- .../e2e/tests/account/forgot-password.spec.ts | 17 ++- test/e2e/tests/account/lock-account.spec.ts | 13 +- .../tests/account/migrate-old-vault.spec.ts | 13 +- .../account/snap-account-settings.spec.ts | 46 +++++++ test/e2e/tests/transaction/ens.spec.ts | 14 +-- .../e2e/tests/transaction/simple-send.spec.ts | 11 +- .../stuck-approved-transaction.spec.ts | 11 +- 18 files changed, 333 insertions(+), 176 deletions(-) delete mode 100644 test/e2e/accounts/snap-account-settings.spec.ts create mode 100644 test/e2e/page-objects/pages/experimental-settings.ts create mode 100644 test/e2e/page-objects/pages/settings-page.ts create mode 100644 test/e2e/tests/account/snap-account-settings.spec.ts diff --git a/test/e2e/accounts/snap-account-settings.spec.ts b/test/e2e/accounts/snap-account-settings.spec.ts deleted file mode 100644 index 5a27f10f4764..000000000000 --- a/test/e2e/accounts/snap-account-settings.spec.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { Suite } from 'mocha'; -import { withFixtures } from '../helpers'; -import FixtureBuilder from '../fixture-builder'; -import { Driver } from '../webdriver/driver'; - -describe('Add snap account experimental settings', function (this: Suite) { - it('switch "Enable Add account snap" to on', async function () { - await withFixtures( - { - fixtures: new FixtureBuilder().build(), - title: this.test?.fullTitle(), - }, - async ({ driver }: { driver: Driver }) => { - await driver.navigate(); - await driver.fill('#password', 'correct horse battery staple'); - await driver.press('#password', driver.Key.ENTER); - - // Make sure the "Add snap account" button is not visible. - await driver.clickElement('[data-testid="account-menu-icon"]'); - await driver.clickElement( - '[data-testid="multichain-account-menu-popover-action-button"]', - ); - - await driver.assertElementNotPresent( - { - text: 'Add account Snap', - tag: 'button', - }, - { - findElementGuard: { - text: 'Add a new Ethereum account', - tag: 'button', - }, - }, // wait for the modal to appear - ); - await driver.clickElement('.mm-box button[aria-label="Close"]'); - - // Navigate to experimental settings. - await driver.clickElement( - '[data-testid="account-options-menu-button"]', - ); - await driver.clickElement({ text: 'Settings', tag: 'div' }); - await driver.clickElement({ text: 'Experimental', tag: 'div' }); - - // Switch "Enable Add account Snap" to on. - await driver.clickElement( - '[data-testid="add-account-snap-toggle-div"]', - ); - - // Make sure the "Add account Snap" button is visible. - await driver.clickElement('[data-testid="account-menu-icon"]'); - await driver.clickElement( - '[data-testid="multichain-account-menu-popover-action-button"]', - ); - await driver.findElement({ - text: 'Add account Snap', - tag: 'button', - }); - }, - ); - }); -}); diff --git a/test/e2e/page-objects/flows/login.flow.ts b/test/e2e/page-objects/flows/login.flow.ts index 039de3e12d98..2904b1b9bd38 100644 --- a/test/e2e/page-objects/flows/login.flow.ts +++ b/test/e2e/page-objects/flows/login.flow.ts @@ -1,30 +1,44 @@ import LoginPage from '../pages/login-page'; import HomePage from '../pages/homepage'; import { Driver } from '../../webdriver/driver'; -import { DEFAULT_GANACHE_ETH_BALANCE_DEC } from '../../constants'; -import { WALLET_PASSWORD } from '../../helpers'; +import { Ganache } from '../../seeder/ganache'; /** - * This method unlocks the wallet and verifies that the user lands on the homepage with the expected balance. It is designed to be the initial step in setting up a test environment. + * This method unlocks the wallet and lands the user on the homepage. * * @param driver - The webdriver instance. - * @param password - The password used to unlock the wallet. Defaults to WALLET_PASSWORD. - * @param expectedBalance - The expected balance to be displayed on the homepage after successful login. Defaults to DEFAULT_GANACHE_ETH_BALANCE_DEC, reflecting common usage in test setups. + * @param password - The password used to unlock the wallet. */ -export const loginWithBalanceValidation = async ( +export const loginWithoutBalanceValidation = async ( driver: Driver, - password: string = WALLET_PASSWORD, - expectedBalance: string = DEFAULT_GANACHE_ETH_BALANCE_DEC, + password?: string, ) => { - console.log('Navigate to unlock page and try to login with pasword'); + console.log('Navigate to unlock page and try to login with password'); await driver.navigate(); const loginPage = new LoginPage(driver); await loginPage.check_pageIsLoaded(); - await loginPage.fillPassword(password); - await loginPage.clickUnlockButton(); + await loginPage.loginToHomepage(password); // user should land on homepage after successfully logging in with password const homePage = new HomePage(driver); await homePage.check_pageIsLoaded(); - await homePage.check_expectedBalanceIsDisplayed(expectedBalance); +}; + +/** + * This method unlocks the wallet and verifies that the user lands on the homepage with the expected balance. It is designed to be the initial step in setting up a test environment. + * + * @param driver - The webdriver instance. + * @param ganacheServer - The ganache server instance + * @param password - The password used to unlock the wallet. + */ +export const loginWithBalanceValidation = async ( + driver: Driver, + ganacheServer?: Ganache, + password?: string, +) => { + await loginWithoutBalanceValidation(driver, password); + // Verify the expected balance on the homepage + if (ganacheServer) { + await new HomePage(driver).check_ganacheBalanceIsDisplayed(ganacheServer); + } }; diff --git a/test/e2e/page-objects/pages/account-list-page.ts b/test/e2e/page-objects/pages/account-list-page.ts index df245c961345..05c394444c36 100644 --- a/test/e2e/page-objects/pages/account-list-page.ts +++ b/test/e2e/page-objects/pages/account-list-page.ts @@ -5,60 +5,65 @@ class AccountListPage { private accountListItem: string; - private accountOptionsMenuButton: string; + private accountMenuButton: string; - private hideUnhideAccountButton: string; + private accountNameInput: string; - private hiddenAccountsList: string; + private accountOptionsMenuButton: string; - private hiddenAccountOptionsMenuButton: string; + private addAccountConfirmButton: string; - private pinnedIcon: string; + private addEthereumAccountButton: string; - private pinUnpinAccountButton: string; + private addSnapAccountButton: object; + + private closeAccountModalButton: string; private createAccountButton: string; - private addEthereumAccountButton: string; + private editableLabelButton: string; - private accountNameInput: string; + private editableLabelInput: string; - private addAccountConfirmButton: string; + private hideUnhideAccountButton: string; - private accountMenuButton: string; + private hiddenAccountOptionsMenuButton: string; - private editableLabelButton: string; + private hiddenAccountsList: string; - private editableLabelInput: string; + private pinUnpinAccountButton: string; - private saveAccountLabelButton: string; + private pinnedIcon: string; - private closeEditLabelButton: string; + private saveAccountLabelButton: string; constructor(driver: Driver) { this.driver = driver; + this.accountListItem = '.multichain-account-menu-popover__list--menu-item'; + this.accountMenuButton = '[data-testid="account-list-menu-details"]'; + this.accountNameInput = '#account-name'; this.accountOptionsMenuButton = '[data-testid="account-list-item-menu-button"]'; - this.hideUnhideAccountButton = '[data-testid="account-list-menu-hide"]'; - this.pinUnpinAccountButton = '[data-testid="account-list-menu-pin"]'; - this.hiddenAccountsList = '[data-testid="hidden-accounts-list"]'; - this.pinnedIcon = '[data-testid="account-pinned-icon"]'; - this.hiddenAccountOptionsMenuButton = - '.multichain-account-menu-popover__list--menu-item-hidden-account [data-testid="account-list-item-menu-button"]'; - this.createAccountButton = - '[data-testid="multichain-account-menu-popover-action-button"]'; - this.addEthereumAccountButton = - '[data-testid="multichain-account-menu-popover-add-account"]'; - this.accountNameInput = '#account-name'; this.addAccountConfirmButton = '[data-testid="submit-add-account-with-name"]'; - this.accountMenuButton = '[data-testid="account-list-menu-details"]'; + this.addEthereumAccountButton = + '[data-testid="multichain-account-menu-popover-add-account"]'; + this.addSnapAccountButton = { + text: 'Add account Snap', + tag: 'button', + }; + this.closeAccountModalButton = 'button[aria-label="Close"]'; + this.createAccountButton = + '[data-testid="multichain-account-menu-popover-action-button"]'; this.editableLabelButton = '[data-testid="editable-label-button"]'; this.editableLabelInput = '[data-testid="editable-input"] input'; + this.hideUnhideAccountButton = '[data-testid="account-list-menu-hide"]'; + this.hiddenAccountOptionsMenuButton = + '.multichain-account-menu-popover__list--menu-item-hidden-account [data-testid="account-list-item-menu-button"]'; + this.hiddenAccountsList = '[data-testid="hidden-accounts-list"]'; + this.pinUnpinAccountButton = '[data-testid="account-list-menu-pin"]'; + this.pinnedIcon = '[data-testid="account-pinned-icon"]'; this.saveAccountLabelButton = '[data-testid="save-account-label-input"]'; - this.closeEditLabelButton = 'button[aria-label="Close"]'; - // this selector needs to be used in combination with an account label text. - this.accountListItem = '.multichain-account-menu-popover__list--menu-item'; } async check_pageIsLoaded(): Promise { @@ -100,7 +105,14 @@ class AccountListPage { await this.driver.clickElement(this.editableLabelButton); await this.driver.fill(this.editableLabelInput, newLabel); await this.driver.clickElement(this.saveAccountLabelButton); - await this.driver.clickElement(this.closeEditLabelButton); + await this.driver.clickElement(this.closeAccountModalButton); + } + + async closeAccountModal(): Promise { + console.log(`Close account modal in account list`); + await this.driver.clickElementAndWaitToDisappear( + this.closeAccountModalButton, + ); } async hideAccount(): Promise { @@ -110,9 +122,16 @@ class AccountListPage { async openAccountOptionsMenu(): Promise { console.log(`Open account option menu`); + await this.driver.waitForSelector(this.accountListItem); await this.driver.clickElement(this.accountOptionsMenuButton); } + async openAddAccountModal(): Promise { + console.log(`Open add account modal in account list`); + await this.driver.clickElement(this.createAccountButton); + await this.driver.waitForSelector(this.addEthereumAccountButton); + } + async openHiddenAccountOptions(): Promise { console.log(`Open hidden accounts options menu`); await this.driver.clickElement(this.hiddenAccountOptionsMenuButton); @@ -128,16 +147,6 @@ class AccountListPage { await this.driver.clickElement(this.pinUnpinAccountButton); } - async unhideAccount(): Promise { - console.log(`Unhide account in account list`); - await this.driver.clickElement(this.hideUnhideAccountButton); - } - - async unpinAccount(): Promise { - console.log(`Unpin account in account list`); - await this.driver.clickElement(this.pinUnpinAccountButton); - } - async switchToAccount(expectedLabel: string): Promise { console.log( `Switch to account with label ${expectedLabel} in account list`, @@ -148,11 +157,16 @@ class AccountListPage { }); } - /** - * Check account is displayed in account list. - * - * @param expectedLabel - The expected account label to be displayed in accouunt list. - */ + async unhideAccount(): Promise { + console.log(`Unhide account in account list`); + await this.driver.clickElement(this.hideUnhideAccountButton); + } + + async unpinAccount(): Promise { + console.log(`Unpin account in account list`); + await this.driver.clickElement(this.pinUnpinAccountButton); + } + async check_accountDisplayedInAccountList( expectedLabel: string = 'Account', ): Promise { @@ -165,6 +179,11 @@ class AccountListPage { }); } + async check_accountIsDisplayed(): Promise { + console.log(`Check that account is displayed in account list`); + await this.driver.waitForSelector(this.accountListItem); + } + async check_accountIsPinned(): Promise { console.log(`Check that account is pinned`); await this.driver.waitForSelector(this.pinnedIcon); @@ -175,6 +194,16 @@ class AccountListPage { await this.driver.assertElementNotPresent(this.pinnedIcon); } + async check_addAccountSnapButtonIsDisplayed(): Promise { + console.log('Check add account snap button is displayed'); + await this.driver.waitForSelector(this.addSnapAccountButton); + } + + async check_addAccountSnapButtonNotPresent(): Promise { + console.log('Check add account snap button is not present'); + await this.driver.assertElementNotPresent(this.addSnapAccountButton); + } + async check_hiddenAccountsListExists(): Promise { console.log(`Check that hidden accounts list is displayed in account list`); await this.driver.waitForSelector(this.hiddenAccountsList); diff --git a/test/e2e/page-objects/pages/experimental-settings.ts b/test/e2e/page-objects/pages/experimental-settings.ts new file mode 100644 index 000000000000..8c7129b17555 --- /dev/null +++ b/test/e2e/page-objects/pages/experimental-settings.ts @@ -0,0 +1,38 @@ +import { Driver } from '../../webdriver/driver'; + +class ExperimentalSettings { + private readonly driver: Driver; + + // Locators + private readonly addAccountSnapToggle: string = + '[data-testid="add-account-snap-toggle-div"]'; + + private readonly experimentalPageTitle: object = { + text: 'Experimental', + css: '.h4', + }; + + constructor(driver: Driver) { + this.driver = driver; + } + + async check_pageIsLoaded(): Promise { + try { + await this.driver.waitForSelector(this.experimentalPageTitle); + } catch (e) { + console.log( + 'Timeout while waiting for Experimental Settings page to be loaded', + e, + ); + throw e; + } + console.log('Experimental Settings page is loaded'); + } + + async toggleAddAccountSnap(): Promise { + console.log('Toggle Add Account Snap on experimental setting page'); + await this.driver.clickElement(this.addAccountSnapToggle); + } +} + +export default ExperimentalSettings; diff --git a/test/e2e/page-objects/pages/header-navbar.ts b/test/e2e/page-objects/pages/header-navbar.ts index fd446fbd664b..742b37a5c48f 100644 --- a/test/e2e/page-objects/pages/header-navbar.ts +++ b/test/e2e/page-objects/pages/header-navbar.ts @@ -9,20 +9,29 @@ class HeaderNavbar { private lockMetaMaskButton: string; + private settingsButton: string; + constructor(driver: Driver) { this.driver = driver; this.accountMenuButton = '[data-testid="account-menu-icon"]'; this.accountOptionMenu = '[data-testid="account-options-menu-button"]'; this.lockMetaMaskButton = '[data-testid="global-menu-lock"]'; + this.settingsButton = '[data-testid="global-menu-settings"]'; + } + + async lockMetaMask(): Promise { + await this.driver.clickElement(this.accountOptionMenu); + await this.driver.clickElement(this.lockMetaMaskButton); } async openAccountMenu(): Promise { await this.driver.clickElement(this.accountMenuButton); } - async lockMetaMask(): Promise { + async openSettingsPage(): Promise { + console.log('Open settings page'); await this.driver.clickElement(this.accountOptionMenu); - await this.driver.clickElement(this.lockMetaMaskButton); + await this.driver.clickElement(this.settingsButton); } /** diff --git a/test/e2e/page-objects/pages/homepage.ts b/test/e2e/page-objects/pages/homepage.ts index 5e596021a449..59845138c8a2 100644 --- a/test/e2e/page-objects/pages/homepage.ts +++ b/test/e2e/page-objects/pages/homepage.ts @@ -1,6 +1,6 @@ import { strict as assert } from 'assert'; import { Driver } from '../../webdriver/driver'; -import { DEFAULT_GANACHE_ETH_BALANCE_DEC } from '../../constants'; +import { Ganache } from '../../seeder/ganache'; import HeaderNavbar from './header-navbar'; class HomePage { @@ -52,26 +52,6 @@ class HomePage { console.log('Home page is loaded'); } - async check_expectedBalanceIsDisplayed( - expectedBalance: string = DEFAULT_GANACHE_ETH_BALANCE_DEC, - ): Promise { - try { - await this.driver.waitForSelector({ - css: this.balance, - text: `${expectedBalance} ETH`, - }); - } catch (e) { - const balance = await this.driver.waitForSelector(this.balance); - const currentBalance = parseFloat(await balance.getText()); - const errorMessage = `Expected balance ${expectedBalance} ETH, got balance ${currentBalance} ETH`; - console.log(errorMessage, e); - throw e; - } - console.log( - `Expected balance ${expectedBalance} ETH is displayed on homepage`, - ); - } - async startSendFlow(): Promise { await this.driver.clickElement(this.sendButton); } @@ -129,6 +109,39 @@ class HomePage { ); } + async check_expectedBalanceIsDisplayed( + expectedBalance: string, + ): Promise { + try { + await this.driver.waitForSelector({ + css: this.balance, + text: `${expectedBalance} ETH`, + }); + } catch (e) { + const balance = await this.driver.waitForSelector(this.balance); + const currentBalance = parseFloat(await balance.getText()); + const errorMessage = `Expected balance ${expectedBalance} ETH, got balance ${currentBalance} ETH`; + console.log(errorMessage, e); + throw e; + } + console.log( + `Expected balance ${expectedBalance} ETH is displayed on homepage`, + ); + } + + async check_ganacheBalanceIsDisplayed( + ganacheServer?: Ganache, + address = null, + ): Promise { + let expectedBalance: string; + if (ganacheServer) { + expectedBalance = (await ganacheServer.getBalance(address)).toString(); + } else { + expectedBalance = '0'; + } + await this.check_expectedBalanceIsDisplayed(expectedBalance); + } + /** * This function checks if a specified transaction amount at the specified index matches the expected one. * diff --git a/test/e2e/page-objects/pages/login-page.ts b/test/e2e/page-objects/pages/login-page.ts index 079e8e091f04..ccbb1d1483b3 100644 --- a/test/e2e/page-objects/pages/login-page.ts +++ b/test/e2e/page-objects/pages/login-page.ts @@ -1,4 +1,5 @@ import { Driver } from '../../webdriver/driver'; +import { WALLET_PASSWORD } from '../../helpers'; class LoginPage { private driver: Driver; @@ -39,11 +40,14 @@ class LoginPage { console.log('Login page is loaded'); } - async fillPassword(password: string): Promise { + /** + * This method unlocks the wallet and lands user on the homepage. + * + * @param password - The password used to unlock the wallet. Defaults to WALLET_PASSWORD. + */ + async loginToHomepage(password: string = WALLET_PASSWORD): Promise { + console.log(`On login page, Login to homepage `); await this.driver.fill(this.passwordInput, password); - } - - async clickUnlockButton(): Promise { await this.driver.clickElement(this.unlockButton); } diff --git a/test/e2e/page-objects/pages/settings-page.ts b/test/e2e/page-objects/pages/settings-page.ts new file mode 100644 index 000000000000..89678c8712ac --- /dev/null +++ b/test/e2e/page-objects/pages/settings-page.ts @@ -0,0 +1,37 @@ +import { Driver } from '../../webdriver/driver'; + +class SettingsPage { + private readonly driver: Driver; + + // Locators + private readonly experimentalSettingsButton: object = { + text: 'Experimental', + css: '.tab-bar__tab__content__title', + }; + + private readonly settingsPageTitle: object = { + text: 'Settings', + css: 'h3', + }; + + constructor(driver: Driver) { + this.driver = driver; + } + + async check_pageIsLoaded(): Promise { + try { + await this.driver.waitForSelector(this.settingsPageTitle); + } catch (e) { + console.log('Timeout while waiting for Settings page to be loaded', e); + throw e; + } + console.log('Settings page is loaded'); + } + + async goToExperimentalSettings(): Promise { + console.log('Navigating to Experimental Settings'); + await this.driver.clickElement(this.experimentalSettingsButton); + } +} + +export default SettingsPage; diff --git a/test/e2e/tests/account/account-custom-name.spec.ts b/test/e2e/tests/account/account-custom-name.spec.ts index 336992a03a1d..138317b1dcf0 100644 --- a/test/e2e/tests/account/account-custom-name.spec.ts +++ b/test/e2e/tests/account/account-custom-name.spec.ts @@ -1,6 +1,6 @@ import { Suite } from 'mocha'; import { Driver } from '../../webdriver/driver'; -import { defaultGanacheOptions, withFixtures } from '../../helpers'; +import { withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; import AccountListPage from '../../page-objects/pages/account-list-page'; @@ -14,7 +14,6 @@ describe('Account Custom Name Persistence', function (this: Suite) { await withFixtures( { fixtures: new FixtureBuilder().build(), - ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, async ({ driver }: { driver: Driver }) => { diff --git a/test/e2e/tests/account/account-hide-unhide.spec.ts b/test/e2e/tests/account/account-hide-unhide.spec.ts index 309ad9339fc8..92429f4e0a74 100644 --- a/test/e2e/tests/account/account-hide-unhide.spec.ts +++ b/test/e2e/tests/account/account-hide-unhide.spec.ts @@ -1,6 +1,6 @@ import { Suite } from 'mocha'; import { Driver } from '../../webdriver/driver'; -import { withFixtures, defaultGanacheOptions } from '../../helpers'; +import { withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; import AccountListPage from '../../page-objects/pages/account-list-page'; @@ -11,7 +11,6 @@ describe('Account list - hide/unhide functionality', function (this: Suite) { await withFixtures( { fixtures: new FixtureBuilder().build(), - ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, async ({ driver }: { driver: Driver }) => { diff --git a/test/e2e/tests/account/account-pin-unpin.spec.ts b/test/e2e/tests/account/account-pin-unpin.spec.ts index 114a2e32c008..ea4b141e911a 100644 --- a/test/e2e/tests/account/account-pin-unpin.spec.ts +++ b/test/e2e/tests/account/account-pin-unpin.spec.ts @@ -1,6 +1,6 @@ import { Suite } from 'mocha'; import { Driver } from '../../webdriver/driver'; -import { withFixtures, defaultGanacheOptions } from '../../helpers'; +import { withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; import AccountListPage from '../../page-objects/pages/account-list-page'; @@ -11,7 +11,6 @@ describe('Account list - pin/unpin functionality', function (this: Suite) { await withFixtures( { fixtures: new FixtureBuilder().build(), - ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, async ({ driver }: { driver: Driver }) => { @@ -37,7 +36,6 @@ describe('Account list - pin/unpin functionality', function (this: Suite) { await withFixtures( { fixtures: new FixtureBuilder().build(), - ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, async ({ driver }: { driver: Driver }) => { diff --git a/test/e2e/tests/account/forgot-password.spec.ts b/test/e2e/tests/account/forgot-password.spec.ts index d5af9998520d..698efcbe0819 100644 --- a/test/e2e/tests/account/forgot-password.spec.ts +++ b/test/e2e/tests/account/forgot-password.spec.ts @@ -1,7 +1,8 @@ -import { withFixtures, defaultGanacheOptions } from '../../helpers'; +import { defaultGanacheOptions, withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; -import { E2E_SRP } from '../../default-fixture'; import { Driver } from '../../webdriver/driver'; +import { E2E_SRP } from '../../default-fixture'; +import { Ganache } from '../../seeder/ganache'; import HomePage from '../../page-objects/pages/homepage'; import LoginPage from '../../page-objects/pages/login-page'; import ResetPasswordPage from '../../page-objects/pages/reset-password-page'; @@ -17,8 +18,14 @@ describe('Forgot password', function () { ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, - async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver); + async ({ + driver, + ganacheServer, + }: { + driver: Driver; + ganacheServer?: Ganache; + }) => { + await loginWithBalanceValidation(driver, ganacheServer); // Lock Wallet const homePage = new HomePage(driver); @@ -35,7 +42,7 @@ describe('Forgot password', function () { await homePage.headerNavbar.lockMetaMask(); // Check user can log in with new password - await loginWithBalanceValidation(driver, newPassword); + await loginWithBalanceValidation(driver, ganacheServer, newPassword); }, ); }); diff --git a/test/e2e/tests/account/lock-account.spec.ts b/test/e2e/tests/account/lock-account.spec.ts index c4cc914978cb..b77170c23676 100644 --- a/test/e2e/tests/account/lock-account.spec.ts +++ b/test/e2e/tests/account/lock-account.spec.ts @@ -3,6 +3,7 @@ import { defaultGanacheOptions, withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import HomePage from '../../page-objects/pages/homepage'; import { Driver } from '../../webdriver/driver'; +import { Ganache } from '../../seeder/ganache'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; describe('Lock and unlock', function (this: Suite) { @@ -13,11 +14,17 @@ describe('Lock and unlock', function (this: Suite) { ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, - async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver); + async ({ + driver, + ganacheServer, + }: { + driver: Driver; + ganacheServer?: Ganache; + }) => { + await loginWithBalanceValidation(driver, ganacheServer); const homePage = new HomePage(driver); await homePage.headerNavbar.lockMetaMask(); - await loginWithBalanceValidation(driver); + await loginWithBalanceValidation(driver, ganacheServer); }, ); }); diff --git a/test/e2e/tests/account/migrate-old-vault.spec.ts b/test/e2e/tests/account/migrate-old-vault.spec.ts index eeacfbc24a09..702729bd6fe7 100644 --- a/test/e2e/tests/account/migrate-old-vault.spec.ts +++ b/test/e2e/tests/account/migrate-old-vault.spec.ts @@ -2,6 +2,7 @@ import { Suite } from 'mocha'; import { defaultGanacheOptions, withFixtures } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { Driver } from '../../webdriver/driver'; +import { Ganache } from '../../seeder/ganache'; import HomePage from '../../page-objects/pages/homepage'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; @@ -13,11 +14,17 @@ describe('Migrate vault with old encryption', function (this: Suite) { ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, - async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver); + async ({ + driver, + ganacheServer, + }: { + driver: Driver; + ganacheServer?: Ganache; + }) => { + await loginWithBalanceValidation(driver, ganacheServer); const homePage = new HomePage(driver); await homePage.headerNavbar.lockMetaMask(); - await loginWithBalanceValidation(driver); + await loginWithBalanceValidation(driver, ganacheServer); }, ); }); diff --git a/test/e2e/tests/account/snap-account-settings.spec.ts b/test/e2e/tests/account/snap-account-settings.spec.ts new file mode 100644 index 000000000000..cbd5f8814b7b --- /dev/null +++ b/test/e2e/tests/account/snap-account-settings.spec.ts @@ -0,0 +1,46 @@ +import { Suite } from 'mocha'; +import { withFixtures } from '../../helpers'; +import FixtureBuilder from '../../fixture-builder'; +import { Driver } from '../../webdriver/driver'; +import SettingsPage from '../../page-objects/pages/settings-page'; +import ExperimentalSettings from '../../page-objects/pages/experimental-settings'; +import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; +import HeaderNavbar from '../../page-objects/pages/header-navbar'; +import AccountListPage from '../../page-objects/pages/account-list-page'; + +describe('Add snap account experimental settings @no-mmi', function (this: Suite) { + it('switch "Enable Add account snap" to on', async function () { + await withFixtures( + { + fixtures: new FixtureBuilder().build(), + title: this.test?.fullTitle(), + }, + async ({ driver }: { driver: Driver }) => { + await loginWithBalanceValidation(driver); + + // Make sure the "Add snap account" button is not visible. + const headerNavbar = new HeaderNavbar(driver); + await headerNavbar.openAccountMenu(); + const accountListPage = new AccountListPage(driver); + await accountListPage.openAddAccountModal(); + await accountListPage.check_addAccountSnapButtonNotPresent(); + await accountListPage.closeAccountModal(); + + // Navigate to experimental settings and enable Add account Snap. + await headerNavbar.openSettingsPage(); + const settingsPage = new SettingsPage(driver); + await settingsPage.check_pageIsLoaded(); + await settingsPage.goToExperimentalSettings(); + + const experimentalSettings = new ExperimentalSettings(driver); + await settingsPage.check_pageIsLoaded(); + await experimentalSettings.toggleAddAccountSnap(); + + // Make sure the "Add account Snap" button is visible. + await headerNavbar.openAccountMenu(); + await accountListPage.openAddAccountModal(); + await accountListPage.check_addAccountSnapButtonIsDisplayed(); + }, + ); + }); +}); diff --git a/test/e2e/tests/transaction/ens.spec.ts b/test/e2e/tests/transaction/ens.spec.ts index 77d10d9b95ad..4700ab8fac3f 100644 --- a/test/e2e/tests/transaction/ens.spec.ts +++ b/test/e2e/tests/transaction/ens.spec.ts @@ -1,13 +1,9 @@ import { Suite } from 'mocha'; import { MockttpServer } from 'mockttp'; -import { - defaultGanacheOptions, - withFixtures, - WALLET_PASSWORD, -} from '../../helpers'; +import { defaultGanacheOptions, withFixtures } from '../../helpers'; import { Driver } from '../../webdriver/driver'; import FixtureBuilder from '../../fixture-builder'; -import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; +import { loginWithoutBalanceValidation } from '../../page-objects/flows/login.flow'; import HomePage from '../../page-objects/pages/homepage'; import SendTokenPage from '../../page-objects/pages/send/send-token-page'; import { mockServerJsonRpc } from '../ppom/mocks/mock-server-json-rpc'; @@ -112,10 +108,12 @@ describe('ENS', function (this: Suite) { testSpecificMock: mockInfura, }, async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver, WALLET_PASSWORD, '<0.000001'); + await loginWithoutBalanceValidation(driver); // click send button on homepage to start send flow - await new HomePage(driver).startSendFlow(); + const homepage = new HomePage(driver); + await homepage.check_expectedBalanceIsDisplayed('<0.000001'); + await homepage.startSendFlow(); // fill ens address as recipient when user lands on send token screen const sendToPage = new SendTokenPage(driver); diff --git a/test/e2e/tests/transaction/simple-send.spec.ts b/test/e2e/tests/transaction/simple-send.spec.ts index 25a9497d6bb3..43b096bf4ec8 100644 --- a/test/e2e/tests/transaction/simple-send.spec.ts +++ b/test/e2e/tests/transaction/simple-send.spec.ts @@ -1,5 +1,6 @@ import { Suite } from 'mocha'; import { Driver } from '../../webdriver/driver'; +import { Ganache } from '../../seeder/ganache'; import { withFixtures, defaultGanacheOptions } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; @@ -14,8 +15,14 @@ describe('Simple send eth', function (this: Suite) { ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, - async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver); + async ({ + driver, + ganacheServer, + }: { + driver: Driver; + ganacheServer?: Ganache; + }) => { + await loginWithBalanceValidation(driver, ganacheServer); await sendTransaction( driver, '0x985c30949c92df7a0bd42e0f3e3d539ece98db24', diff --git a/test/e2e/tests/transaction/stuck-approved-transaction.spec.ts b/test/e2e/tests/transaction/stuck-approved-transaction.spec.ts index 20be72537277..7afedc3187dd 100644 --- a/test/e2e/tests/transaction/stuck-approved-transaction.spec.ts +++ b/test/e2e/tests/transaction/stuck-approved-transaction.spec.ts @@ -2,6 +2,7 @@ import { Suite } from 'mocha'; import { withFixtures, generateGanacheOptions } from '../../helpers'; import FixtureBuilder from '../../fixture-builder'; import { Driver } from '../../webdriver/driver'; +import { Ganache } from '../../seeder/ganache'; import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; import HomePage from '../../page-objects/pages/homepage'; @@ -15,8 +16,14 @@ describe('Editing Confirm Transaction', function (this: Suite) { ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), title: this.test?.fullTitle(), }, - async ({ driver }: { driver: Driver }) => { - await loginWithBalanceValidation(driver); + async ({ + driver, + ganacheServer, + }: { + driver: Driver; + ganacheServer?: Ganache; + }) => { + await loginWithBalanceValidation(driver, ganacheServer); const homePage = new HomePage(driver); await homePage.goToActivityList(); From 8c3e71f346246db85719688846db2dc8e154fc89 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Wed, 25 Sep 2024 15:43:02 +0200 Subject: [PATCH 6/6] fix: flaky test `Token Allowance increases token spending cap to allow other accounts to transfer tokens` (#27389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** There is a race condition with gas values which cause to send the increase allowance transaction with a lower gas value making this transaction to fail. Subsequently, when we try the transfer from action from the dapp, no popup is open, since we don't have enough allowance. This is the moment where the test fails (waiting for the popup) ![Screenshot from 2024-09-25 13-08-43](https://github.com/user-attachments/assets/13707906-7246-4870-bf97-6883f8423f45) The problem is that the gas values are changed like this: - `0.000062` - `0.000054` - `0.000062` Since the first value is already the expected one, we jump into the Confirm step, but right before clicking confirm, it can happen that we send the tx with the next gas value `0.000054` making the tx to fail (see video) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27389?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/27388 ## **Manual testing steps** 1. Check ci 2. Run test manually `ENABLE_MV3=false yarn test:e2e:single test/e2e/tests/tokens/increase-token-allowance.spec.js --browser=chrome --leave-running=true` ## **Screenshots/Recordings** See how gas is updated, first it has the correct value (which is the reason why we proceed) but right after, the gas is updated to a lower value, sending the tx with that low value. https://github.com/user-attachments/assets/efd1ae40-c582-4d38-8573-17b11da94ed3 ## **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. --- test/e2e/tests/tokens/increase-token-allowance.spec.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/e2e/tests/tokens/increase-token-allowance.spec.js b/test/e2e/tests/tokens/increase-token-allowance.spec.js index ef1ed8e19998..7df956e9df43 100644 --- a/test/e2e/tests/tokens/increase-token-allowance.spec.js +++ b/test/e2e/tests/tokens/increase-token-allowance.spec.js @@ -272,6 +272,15 @@ describe('Increase Token Allowance', function () { css: '.box--display-flex > h6', text: `10 TST`, }); + await driver.assertElementNotPresent( + { + tag: 'h6', + text: '0.000054 ETH', + }, + { + waitAtLeastGuard: 2000, + }, + ); await driver.waitForSelector({ tag: 'h6', text: '0.000062 ETH',