Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: disable wallet buttons for accounts that cannot sign transactions #11330

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

k-g-j
Copy link
Contributor

@k-g-j k-g-j commented Sep 19, 2024

Description

This PR disables certain buttons in the WalletActions component when the selected account cannot sign transactions. This change improves the user experience by preventing attempts to perform actions that require transaction signing when the account cannot do so.

  1. The reason for the change is to prevent users from attempting actions that their current account cannot perform.
  2. The improvement is that Buy, Sell, Send, Swap, and Bridge buttons are now disabled when the selected account cannot sign transactions, providing clear visual feedback to the user about available actions.

Related issues

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

Manual testing steps

  1. Go to the SSK and create an account
  2. Update the account using this JSON, replacing the address and ID fields with the new account information (this update step removes eth_signTransaction from the account's methods)
{
    "id": "your new account ID",
    "address": "you new account address",
    "options": {},
    "methods": [
        "personal_sign",
        "eth_sign",
        "eth_signTypedData_v1",
        "eth_signTypedData_v3",
        "eth_signTypedData_v4"
    ],
    "type": "eip155:eoa"
}
  1. Switch to the account that cannot sign transactions if not already there
  2. Verify that the Buy, Sell, Send, Swap, and Bridge buttons are visually disabled with a message and not clickable
  3. Click on the asset details view
  4. Verify that the Buy, Sell, Send, Swap, and Bridge buttons are visually disabled and not clickable
  5. Switch back to an account that can sign transactions
  6. Verify that the buttons are now enabled and functional in wallet view and asset details view

Screenshots/Recordings

Before

Wallet view

Asset details view
simulator_screenshot_5FB98682-1B94-440A-9E12-BDE6D8B2B1C3

After

Wallet view

Asset details view
simulator_screenshot_6BD320C5-1B8E-4C8B-B437-CCC7416A8B3D

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

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

@k-g-j k-g-j changed the base branch from main to feat/display-snap-accounts-list September 19, 2024 20:56
@k-g-j k-g-j added area-UI needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-accounts labels Sep 19, 2024
@k-g-j k-g-j changed the base branch from feat/display-snap-accounts-list to main September 23, 2024 15:09
@k-g-j k-g-j marked this pull request as ready for review September 23, 2024 16:34
@k-g-j k-g-j requested a review from a team as a code owner September 23, 2024 16:34
@k-g-j k-g-j added the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 23, 2024
Copy link
Contributor

github-actions bot commented Sep 23, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 0f8068e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5ef1ab5f-a183-4616-bbb1-1086411a722d

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@k-g-j k-g-j added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 23, 2024
Copy link
Contributor

github-actions bot commented Sep 23, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: ed95ed3
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/3efa2677-f1bb-4979-8a75-c2aaa4555cfe

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall, left some minor comments

app/components/UI/WalletAction/WalletAction.types.ts Outdated Show resolved Hide resolved
app/components/UI/WalletAction/WalletAction.types.ts Outdated Show resolved Hide resolved
app/components/Views/WalletActions/WalletActions.test.tsx Outdated Show resolved Hide resolved
@k-g-j k-g-j removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 24, 2024
@@ -1237,6 +1238,14 @@
"info": "Info",
"swap": "Swap",
"bridge": "Bridge",
"disabled_button": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexJupiter @eriknson what do you think of this copy?

@k-g-j k-g-j force-pushed the feat/disable-signing-buttons branch from 2613b17 to 7a0a91a Compare September 24, 2024 22:13
@k-g-j k-g-j added the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 26, 2024
Copy link
Contributor

github-actions bot commented Sep 26, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: f5d01a6
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4b8e980b-bc7a-4554-b6b7-b155a1c30f8d

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@k-g-j k-g-j removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 27, 2024
owencraston
owencraston previously approved these changes Sep 27, 2024
@k-g-j k-g-j added the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 27, 2024
Copy link
Contributor

github-actions bot commented Sep 27, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: abda0b1
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/56bd3f1b-5482-4883-a2e9-cfe49cb8cc4f

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@k-g-j k-g-j removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Sep 30, 2024
…d label from account overview"

This reverts commit 54ac028.
@k-g-j k-g-j force-pushed the feat/disable-signing-buttons branch from d958792 to ab171e4 Compare October 18, 2024 16:42
Copy link

sonarcloud bot commented Oct 18, 2024

@plasmacorral plasmacorral added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 18, 2024
Copy link
Contributor

github-actions bot commented Oct 18, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: ab171e4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ec23b2a7-71bd-4803-82ad-722de18d238b

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@plasmacorral
Copy link
Contributor

plasmacorral commented Oct 18, 2024

Having an issue with local builds of flask on this branch in commit ab171e4

ERROR  Error: createSelector expects all input-selectors to be functions, but received the following types: [undefined, function memoized(), function memoized()], js engine: hermes
 ERROR  TypeError: Cannot read property 'store' of undefined, js engine: hermes
 ERROR  Error: createSelector expects all input-selectors to be functions, but received the following types: [undefined, function memoized(), function memoized()], js engine: hermes
 ERROR  TypeError: Cannot read property 'store' of undefined, js engine: hermes

Not observed with a flask build of main commit 076fa31

@plasmacorral plasmacorral added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA in Progress QA has started on the feature. labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-UI needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

6 participants