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

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Nov 1, 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.

The reason for the change is to prevent users from attempting actions that their current account cannot perform.
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.

This work is a continuation of Kate's PR. Since her departure it was easier to start with a fresh branch than rebase her work. Her pr already went through QA and reviews.

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

After

Wallet view

Asset details view

QA

This PR is a direct copy of #11330. Kates change went through several rounds of QA with Mike. His first round found a few issues which were outlined here. Those issues were addressed in subsequent commits and finally QA passed here. All of the code in this PR is a copy of the previous except that I addressed @brianacnguyen's comments. The extra styles that I removed have no effect on the UX since the opacity of 0.5 (correctly applied) has the desired effect on the buttons and makes them appear disabled. This came from a discussion with Brian during a code review.

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.

@owencraston owencraston requested review from a team as code owners November 1, 2024 00:01
Copy link
Contributor

github-actions bot commented Nov 1, 2024

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.

@owencraston owencraston added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 1, 2024
Copy link
Contributor

github-actions bot commented Nov 1, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 7ad62d7
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a4094cc7-579d-4f51-90da-e74dbf833718

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

@owencraston owencraston force-pushed the feat/conditional-wallet-actions branch from c0268df to 22bd9cd Compare November 5, 2024 01:53
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 5, 2024
Copy link
Contributor

github-actions bot commented Nov 5, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 22bd9cd
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/bda03e5f-a91e-420f-b5a3-b2f2c5cb6790

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

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, just left two small comments

app/components/UI/WalletAction/WalletAction.types.ts Outdated Show resolved Hide resolved
app/selectors/accountsController.ts Show resolved Hide resolved
@owencraston owencraston force-pushed the feat/conditional-wallet-actions branch from 22bd9cd to 285dfa8 Compare November 6, 2024 02:58
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 6, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 285dfa8
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/70e8fdc8-e5dd-43f9-a8ab-e4c3d945aa89

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

@owencraston owencraston force-pushed the feat/conditional-wallet-actions branch from 285dfa8 to abc306a Compare November 6, 2024 20:46
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 6, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: abc306a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e019ea07-c646-4a2b-8023-bcc2c51ff4c0

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

Copy link

sonarcloud bot commented Nov 6, 2024

@owencraston
Copy link
Contributor Author

E2E Failing due a known (not related with this PR) issue which is being handled by QA Team.

@owencraston owencraston added this pull request to the merge queue Nov 6, 2024
Merged via the queue into main with commit a473989 Nov 6, 2024
42 of 43 checks passed
@owencraston owencraston deleted the feat/conditional-wallet-actions branch November 6, 2024 21:44
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2024
@gauthierpetetin gauthierpetetin added the release-7.36.0 Issue or pull request that will be included in release 7.36.0 label Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.36.0 Issue or pull request that will be included in release 7.36.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants