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

fix: fake native tokens need better UX error handling #8572

Merged
merged 11 commits into from
Mar 1, 2024
Merged

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Feb 13, 2024

Description

In response to recent scam incidents we're adding mitigations to help prevent users from adding malicious networks and being shown inaccurate token values.

Implementation of this ticket will:

Show N/A for a native token value, when the native token symbol does not match the chain ID as publicly known
Show a tooltip that when tapped shows a bottom sheet explainer
The explainer directs the user to update their Network details for the network the token balance is requested for
User story

As a user I want to be informed that a token balance might be inaccurate, so that I don't make decisions to transfer or buy assets based on this information
As a user I want to be informed when a custom network might be trying to scam me

Related issues

Fixes: #1354

Manual testing steps

  1. Go to edit network page and choose Polygon
    2.change the symbol from MATIC to ETH
  2. Go to the wallet page

Screenshots/Recordings

Before

before.mov

After

https://drive.google.com/drive/folders/1lcKOyC7U1r9OW2qt7BjsReGdG-02EysJ?usp=sharing

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@salimtb salimtb requested a review from a team as a code owner February 13, 2024 15:00
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.

@salimtb salimtb added team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead need-ux-ds-review A label to be used for design system and UX PR review team-assets labels Feb 13, 2024
@salimtb salimtb marked this pull request as draft February 13, 2024 15:14
@salimtb salimtb marked this pull request as ready for review February 13, 2024 15:23
@github-actions github-actions bot added the Run Smoke E2E Triggers smoke e2e on Bitrise label Feb 13, 2024
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5ea75708-64bf-49ba-b246-cd5de618b4b6
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 32.06107% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 41.39%. Comparing base (371cb5f) to head (bf5977c).

Files Patch % Lines
...Settings/NetworksSettings/NetworkSettings/index.js 0.00% 60 Missing ⚠️
app/components/UI/Tokens/index.tsx 48.27% 10 Missing and 5 partials ⚠️
...tings/NetworkSettings/withIsOriginalNativeToken.js 44.44% 5 Missing ⚠️
app/core/Engine.ts 16.66% 5 Missing ⚠️
app/components/UI/DrawerView/index.js 0.00% 3 Missing ⚠️
...omponents/Views/Settings/NetworksSettings/index.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8572      +/-   ##
==========================================
- Coverage   41.42%   41.39%   -0.03%     
==========================================
  Files        1270     1272       +2     
  Lines       30832    30950     +118     
  Branches     3046     3062      +16     
==========================================
+ Hits        12772    12812      +40     
- Misses      17296    17369      +73     
- Partials      764      769       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@salimtb salimtb added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed need-ux-ds-review A label to be used for design system and UX PR review Run Smoke E2E Triggers smoke e2e on Bitrise labels Feb 14, 2024
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments. Looks like there are failing unit tests as well

app/constants/network.js Outdated Show resolved Hide resolved
app/components/UI/Tokens/index.tsx Outdated Show resolved Hide resolved
app/components/UI/Tokens/index.tsx Outdated Show resolved Hide resolved
app/components/UI/Tokens/styles.ts Outdated Show resolved Hide resolved
app/components/UI/Tokens/index.tsx Show resolved Hide resolved
app/components/UI/Tokens/index.tsx Show resolved Hide resolved
@salimtb salimtb force-pushed the MMASSETS-123 branch 7 times, most recently from bf5a2ec to 40e84ae Compare February 20, 2024 11:17
@salimtb salimtb added the Run Smoke E2E Triggers smoke e2e on Bitrise label Feb 20, 2024
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7ce984a8-9156-4ef2-847b-8375d17bf34d
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@salimtb salimtb removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Feb 20, 2024
@salimtb salimtb requested a review from Cal-L February 20, 2024 16:23
@salimtb salimtb added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Feb 29, 2024
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: f9df61f
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ceb7f2d2-0bb8-4109-ba1e-e9a93c99ab78

Note

  • This comment will auto-update when build completes
  • 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

@salimtb salimtb removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Feb 29, 2024
@salimtb salimtb force-pushed the MMASSETS-123 branch 2 times, most recently from 6803f8d to 8228c4e Compare February 29, 2024 19:04
Copy link

sonarcloud bot commented Feb 29, 2024

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Feb 29, 2024
Copy link
Member

@cortisiko cortisiko 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 from a testing perspective

@salimtb salimtb merged commit 108da1c into main Mar 1, 2024
34 checks passed
@salimtb salimtb deleted the MMASSETS-123 branch March 1, 2024 09:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
@metamaskbot metamaskbot added the release-7.18.0 Issue or pull request that will be included in release 7.18.0 label Mar 1, 2024
@wachunei
Copy link
Member

Hey @salimtb I just realized there were additions to the Ramp folder, to be exact the files:

  • app/components/UI/Ramp/hooks/useIsOriginalNativeTokenSymbol.test.ts
  • app/components/UI/Ramp/hooks/useIsOriginalNativeTokenSymbol.ts

These must have been added before our CODEOWNERS folders were listed so I was not aware of this change. I want to kindly ask for these two files to be moved to the corresponding code domain since these are not used within our feature.

cc: @Cal-L @cortisiko

@salimtb
Copy link
Contributor Author

salimtb commented Apr 18, 2024

  • app/components/UI/Ramp/hooks/useIsOriginalNativeTokenSymbol.test.ts

hey @wachunei ,
hooks now moved on this PR , can i have a review pls

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.18.0 Issue or pull request that will be included in release 7.18.0 team-assets team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants